Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le mardi 29 mars 2022 à 11:13 +0300, Dan Carpenter a écrit :
> On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> >  		int buf_idx = -1;
> >  
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> >  			buf_idx = vb2_find_timestamp(cap_q,
> >  						     dpb[i].reference_ts, 0);
> > +			if (buf_idx < 0)
> > +				pr_debug("No buffer for reference_ts %llu",
> > +					 dpb[i].reference_ts);
> 
> pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> leaving it as an error code.

Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
something that is not a driver error, but userland error. Perhaps you can
educate me on the policy in this regard, but malicous userland being able to
flood the logs very easily is my main concern here.

About the negative idx, it is being used set dpb_valid later on. H.264 error
resilience requires that these frames should be marked as "unexisting" but still
occupy space in the DPB, this is more or less what I'm trying to implement here.
Setting it to 0 would basically mean to refer to DPB index 0, which is
relatively random pick. I believe your suggestion is not taking into
consideration what the code is doing, but it would fall in some poor-man
concealment which I would rather leave to the userland.

> 
> > +		}
> >  
> >  		run->ref_buf_idx[i] = buf_idx;
> >  	}
> >  }
> >  
> >  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > +			    struct v4l2_h264_reflist_builder *builder,
> >  			    struct rkvdec_h264_run *run)
> >  {
> >  	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> >  	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
> >  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
> > -	const struct v4l2_ctrl_h264_sps *sps = run->sps;
> >  	struct rkvdec_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
> > -	u32 max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
> >  
> >  	u32 *hw_rps = priv_tbl->rps;
> >  	u32 i, j;
> > @@ -772,37 +772,36 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  		if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> >  			continue;
> >  
> > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> > -		    dpb[i].frame_num <= dec_params->frame_num) {
> > -			p[i] = dpb[i].frame_num;
> > -			continue;
> > -		}
> > -
> > -		p[i] = dpb[i].frame_num - max_frame_num;
> > +		p[i] = builder->refs[i].frame_num;
> >  	}
> >  
> >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > -			u8 idx = 0;
> > +		for (i = 0; i < builder->num_valid; i++) {
> > +			struct v4l2_h264_reference *ref;
> > +			u8 dpb_valid;
> > +			u8 bottom;
> 
> These would be better as type bool.

I never used a bool for bit operations before, but I guess that can work, thanks
for the suggestion. As this deviates from the original code, I suppose I should
make this a separate patch ?

> 
> regards,
> dan carpenter
> 
> >  
> >  			switch (j) {
> >  			case 0:
> > -				idx = h264_ctx->reflists.p[i].index;
> > +				ref = &h264_ctx->reflists.p[i];
> >  				break;
> >  			case 1:
> > -				idx = h264_ctx->reflists.b0[i].index;
> > +				ref = &h264_ctx->reflists.b0[i];
> >  				break;
> >  			case 2:
> > -				idx = h264_ctx->reflists.b1[i].index;
> > +				ref = &h264_ctx->reflists.b1[i];
> >  				break;
> >  			}
> >  
> > -			if (idx >= ARRAY_SIZE(dec_params->dpb))
> > +			if (WARN_ON(ref->index >= ARRAY_SIZE(dec_params->dpb)))
> >  				continue;
> >  
> > +			dpb_valid = run->ref_buf_idx[ref->index] >= 0;
> > +			bottom = ref->fields == V4L2_H264_BOTTOM_FIELD_REF;
> > +
> >  			set_ps_field(hw_rps, DPB_INFO(i, j),
> > -				     idx | dpb_valid << 4);
> > +				     ref->index | dpb_valid << 4);
> > +			set_ps_field(hw_rps, BOTTOM_FLAG(i, j), bottom);
> >  		}
> >  	}
> >  }
>