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

  This makes use of the new feature in the reference builder to program
up to 32 references when doing field decoding. It also signals the
parity (top of bottom) of the field to the hardware.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 48 ++++++++++------------
 1 file changed, 21 insertions(+), 27 deletions(-)
  

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

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

On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> 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.
> 

To be honest, I just saw that it was a negative idx and freaked out.  I
didn't look at any context...

You're right that we don't to allow the user to spam the dmesg.  Ideally
we would return an error.  A second best solution is to do a pr_err_once().

> > >  	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 ?

I just saw the name and wondered why it was a u8.  bool does make more
sense and works fine for the bitwise stuff.  But I don't really care at
all.

regards,
dan carpenter
  

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>This makes use of the new feature in the reference builder to program
>up to 32 references when doing field decoding. It also signals the
>parity (top of bottom) of the field to the hardware.

s/top of bottom/top or bottom/

>
>Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>

Greetings,
Sebastian

>---
> drivers/staging/media/rkvdec/rkvdec-h264.c | 48 ++++++++++------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>index ec52b195bbd7..891c48bf6a51 100644
>--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>@@ -97,13 +97,10 @@ struct rkvdec_h264_priv_tbl {
> 	u8 err_info[RKV_ERROR_INFO_SIZE];
> };
>
>-#define RKVDEC_H264_DPB_SIZE 16
>-
> struct rkvdec_h264_reflists {
> 	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
> 	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
> 	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
>-	u8 num_valid;
> };
>
> struct rkvdec_h264_run {
>@@ -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);
>+		}
>
> 		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;
>
> 			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);
> 		}
> 	}
> }
>@@ -990,10 +989,6 @@ static void config_registers(struct rkvdec_ctx *ctx,
> 				       rkvdec->regs + RKVDEC_REG_H264_BASE_REFER15);
> 	}
>
>-	/*
>-	 * Since support frame mode only
>-	 * top_field_order_cnt is the same as bottom_field_order_cnt
>-	 */
> 	reg = RKVDEC_CUR_POC(dec_params->top_field_order_cnt);
> 	writel_relaxed(reg, rkvdec->regs + RKVDEC_REG_CUR_POC0);
>
>@@ -1109,7 +1104,6 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> 	/* Build the P/B{0,1} ref lists. */
> 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
> 				       run.sps, run.decode_params->dpb);
>-	h264_ctx->reflists.num_valid = reflist_builder.num_valid;
> 	v4l2_h264_build_p_ref_list(&reflist_builder, h264_ctx->reflists.p);
> 	v4l2_h264_build_b_ref_lists(&reflist_builder, h264_ctx->reflists.b0,
> 				    h264_ctx->reflists.b1);
>@@ -1117,7 +1111,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> 	assemble_hw_scaling_list(ctx, &run);
> 	assemble_hw_pps(ctx, &run);
> 	lookup_ref_buf_idx(ctx, &run);
>-	assemble_hw_rps(ctx, &run);
>+	assemble_hw_rps(ctx, &reflist_builder, &run);
> 	config_registers(ctx, &run);
>
> 	rkvdec_run_postamble(ctx, &run.base);
>-- 
>2.34.1
>
  

Le mercredi 30 mars 2022 à 08:15 +0300, Dan Carpenter a écrit :
> On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> > 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.
> > 
> 
> To be honest, I just saw that it was a negative idx and freaked out.  I
> didn't look at any context...
> 
> You're right that we don't to allow the user to spam the dmesg.  Ideally
> we would return an error.  A second best solution is to do a pr_err_once().

There is two schools in the context of video stream decoding. I'm not saying
this driver is quite there in term of visual corruption reporting, this is
something I expect we'll improve in the long term. But here's the two schools:

- Freeze on the last non-corrupted image (Apple style)
- Display slightly distorted image with movement (the rest of the world)

In order to give users that choice, I must try decoding as much as I can
regardless if there is a missing a reference or not. That wasn't the goal in
this MR, but in the long run we'll remember this and mark the buffer as
corrupted (using the ERROR but setting a payload size to the picture size). This
leaves the users the option to drop or to keep the visually corrupted image. In
video streaming, corrupted stream could look relatively fine, this cannot be
judge noticing 1 reference frame missing (it could be referenced in only 1
macro-block).

Educational bit behind, I think we should keep going and not "error out". Also,
for debugging purpose, it is nicer if we can get a complete report of non-memory
backed references. As a missing reference in 1 frame, may have implication in
later frame, or may cause other frames to be missed.

One thing I was thinking though, is that through using raw pr_debug() I'm not
giving much context to my trace, so it would be hard to associate it with the
driver instance (m2m are multi-instance) it was running against. But I didn't
want to add a new tracing wrapper for that driver in this patchset as it was
already relatively large patchset. Though, these traces have been previous to
make the driver work (as long as you test with a single instance).

If its all right with everyone, I'd leave it like this for this round, we can
dedicated a patchset on improve this driver tracing in the future.

> 
> > > >  	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 ?
> 
> I just saw the name and wondered why it was a u8.  bool does make more
> sense and works fine for the bitwise stuff.  But I don't really care at
> all.

I'll do that in v2, in same patch, looks minor enough. I think if using bool
could guaranty that only 1 or 0 is  possible, it would be even better, but don't
think C works like this.

Thanks again for your comments.

> 
> regards,
> dan carpenter
>
  

Yeah.  I'm aboslutely fine with whatever you do.  Some of the questions
you're asking occurred to me too but I don't have the answers.

> > > > > +		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 ?
> > 
> > I just saw the name and wondered why it was a u8.  bool does make more
> > sense and works fine for the bitwise stuff.  But I don't really care at
> > all.
> 
> I'll do that in v2, in same patch, looks minor enough. I think if using bool
> could guaranty that only 1 or 0 is  possible, it would be even better, but don't
> think C works like this.

I'm not sure I understand.  If you assign "bool x = <any non-zero>;"
then x is set to true.  Do you want a static checker warning for if
<any non-zero> can be something other than one or zero?  The problem is
that people sometimes deliberately do stuff like "bool x = var & 0xf0;".
Smatch will complain if you assign a negative value to x.

test.c:8 test() warn: assigning (-3) to unsigned variable 'x'

It's supposed to print a warning if you used it to save error codes like:

	x = some_kernel_function();

But it does not.  :/  Something to investigate.

regards,
dan carpenter
  

Le mercredi 30 mars 2022 à 18:16 +0300, Dan Carpenter a écrit :
> Yeah.  I'm aboslutely fine with whatever you do.  Some of the questions
> you're asking occurred to me too but I don't have the answers.
> 
> > > > > > +		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 ?
> > > 
> > > I just saw the name and wondered why it was a u8.  bool does make more
> > > sense and works fine for the bitwise stuff.  But I don't really care at
> > > all.
> > 
> > I'll do that in v2, in same patch, looks minor enough. I think if using bool
> > could guaranty that only 1 or 0 is  possible, it would be even better, but don't
> > think C works like this.
> 
> I'm not sure I understand.  If you assign "bool x = <any non-zero>;"
> then x is set to true.  Do you want a static checker warning for if
> <any non-zero> can be something other than one or zero?  The problem is
> that people sometimes deliberately do stuff like "bool x = var & 0xf0;".
> Smatch will complain if you assign a negative value to x.
> 
> test.c:8 test() warn: assigning (-3) to unsigned variable 'x'
> 
> It's supposed to print a warning if you used it to save error codes like:
> 
> 	x = some_kernel_function();
> 
> But it does not.  :/  Something to investigate.

That would be an amazing catch, you might have seen a lot of:

  x = !!(var & 0xf0)

For branches, it does no matter, but if you use x it like this dpb_valid
variable is used, not having 0 or 1 can lead to very surprising results. In the
end its used like this

  set_reg(reg0, val | (x << N))

So using bool type can hint the analyzer that 0 or 1 was likely expected, while
currently an u8 would be ambiguous and lead to false positive if we were to
warn.

> 
> regards,
> dan carpenter
>