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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

> +		}
>  
>  		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.

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);
>  		}
>  	}
>  }