diff mbox series

[v2,13/23] media: rkvdec: h264: Fix dpb_valid implementation

Message ID 20220331193726.289559-14-nicolas.dufresne@collabora.com (mailing list archive)
State Superseded
Delegated to: Hans Verkuil
Headers
Series [v2,01/23] media: doc: Document dual use of H.264 pic_num/frame_num |

Commit Message

Nicolas Dufresne March 31, 2022, 7:37 p.m. UTC 
  The ref builder only provided references that are marked as valid in the
dpb. Thus the current implementation of dpb_valid would always set the
flag to 1. This is not representing missing frames (this is called
'non-existing' pictures in the spec). In some context, these non-existing
pictures still need to occupy a slot in the reference list according to
the spec.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Ezequiel Garcia April 2, 2022, 11:16 a.m. UTC | #1 
On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> The ref builder only provided references that are marked as valid in the
> dpb. Thus the current implementation of dpb_valid would always set the
> flag to 1. This is not representing missing frames (this is called
> 'non-existing' pictures in the spec). In some context, these non-existing
> pictures still need to occupy a slot in the reference list according to
> the spec.
> 

Good catch! OOC, did you find this because it was failing a test vector?

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

Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks,
Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index dff89732ddd0..bcde37d72244 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
>  	const struct v4l2_ctrl_h264_sps *sps;
>  	const struct v4l2_ctrl_h264_pps *pps;
>  	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> +	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
>  };
>  
>  struct rkvdec_h264_ctx {
> @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>  	}
>  }
>  
> +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> +			       struct rkvdec_h264_run *run)
> +{
> +	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> +	u32 i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> +		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> +		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> +		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)
> +			buf_idx = vb2_find_timestamp(cap_q,
> +						     dpb[i].reference_ts, 0);
> +
> +		run->ref_buf_idx[i] = buf_idx;
> +	}
> +}
> +
>  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  			    struct rkvdec_h264_run *run)
>  {
> @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  
>  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
>  		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> -			u8 dpb_valid = 0;
> +			bool dpb_valid = run->ref_buf_idx[i] >= 0;
>  			u8 idx = 0;
>  
>  			switch (j) {
> @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  
>  			if (idx >= ARRAY_SIZE(dec_params->dpb))
>  				continue;
> -			dpb_valid = !!(dpb[idx].flags &
> -				       V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
>  
>  			set_ps_field(hw_rps, DPB_INFO(i, j),
>  				     idx | dpb_valid << 4);
> @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
>  	    unsigned int dpb_idx)
>  {
>  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> -	const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
>  	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> -	int buf_idx = -1;
> -
> -	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> -		buf_idx = vb2_find_timestamp(cap_q,
> -					     dpb[dpb_idx].reference_ts, 0);
> +	int buf_idx = run->ref_buf_idx[dpb_idx];
>  
>  	/*
>  	 * If a DPB entry is unused or invalid, address of current destination
> @@ -1102,6 +1116,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);
>  	config_registers(ctx, &run);
>  
> -- 
> 2.34.1
>
Nicolas Dufresne April 5, 2022, 3:10 p.m. UTC | #2 
Le samedi 02 avril 2022 à 08:16 -0300, Ezequiel Garcia a écrit :
> On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> > The ref builder only provided references that are marked as valid in the
> > dpb. Thus the current implementation of dpb_valid would always set the
> > flag to 1. This is not representing missing frames (this is called
> > 'non-existing' pictures in the spec). In some context, these non-existing
> > pictures still need to occupy a slot in the reference list according to
> > the spec.
> > 
> 
> Good catch! OOC, did you find this because it was failing a test vector?

The effect is complex, so I could not correlate to specific tests. Also, what I
wanted to fix isn't covered by the ITU conformance, its mostly resiliance
requirement. But this should remove some of the IOMMU fault on broken streams
and make it less likely to use references that don't exists or aren't set what
we expect. After this change, the driver was getting more stable, and results
was also more reproducible (specially in parallel decode case, which I use to
speed up testing).

> 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks for the review.

> 
> Thanks,
> Ezequiel
> 
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > index dff89732ddd0..bcde37d72244 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> >  	const struct v4l2_ctrl_h264_sps *sps;
> >  	const struct v4l2_ctrl_h264_pps *pps;
> >  	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> > +	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> >  };
> >  
> >  struct rkvdec_h264_ctx {
> > @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> >  	}
> >  }
> >  
> > +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > +			       struct rkvdec_h264_run *run)
> > +{
> > +	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> > +	u32 i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> > +		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > +		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > +		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)
> > +			buf_idx = vb2_find_timestamp(cap_q,
> > +						     dpb[i].reference_ts, 0);
> > +
> > +		run->ref_buf_idx[i] = buf_idx;
> > +	}
> > +}
> > +
> >  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  			    struct rkvdec_h264_run *run)
> >  {
> > @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  
> >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> >  		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > -			u8 dpb_valid = 0;
> > +			bool dpb_valid = run->ref_buf_idx[i] >= 0;
> >  			u8 idx = 0;
> >  
> >  			switch (j) {
> > @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> >  
> >  			if (idx >= ARRAY_SIZE(dec_params->dpb))
> >  				continue;
> > -			dpb_valid = !!(dpb[idx].flags &
> > -				       V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
> >  
> >  			set_ps_field(hw_rps, DPB_INFO(i, j),
> >  				     idx | dpb_valid << 4);
> > @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> >  	    unsigned int dpb_idx)
> >  {
> >  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > -	const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> >  	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > -	int buf_idx = -1;
> > -
> > -	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > -		buf_idx = vb2_find_timestamp(cap_q,
> > -					     dpb[dpb_idx].reference_ts, 0);
> > +	int buf_idx = run->ref_buf_idx[dpb_idx];
> >  
> >  	/*
> >  	 * If a DPB entry is unused or invalid, address of current destination
> > @@ -1102,6 +1116,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);
> >  	config_registers(ctx, &run);
> >  
> > -- 
> > 2.34.1
> >
Ezequiel Garcia April 5, 2022, 3:34 p.m. UTC | #3 
On Tue, Apr 5, 2022 at 12:11 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le samedi 02 avril 2022 à 08:16 -0300, Ezequiel Garcia a écrit :
> > On Thu, Mar 31, 2022 at 03:37:15PM -0400, Nicolas Dufresne wrote:
> > > The ref builder only provided references that are marked as valid in the
> > > dpb. Thus the current implementation of dpb_valid would always set the
> > > flag to 1. This is not representing missing frames (this is called
> > > 'non-existing' pictures in the spec). In some context, these non-existing
> > > pictures still need to occupy a slot in the reference list according to
> > > the spec.
> > >
> >
> > Good catch! OOC, did you find this because it was failing a test vector?
>
> The effect is complex, so I could not correlate to specific tests. Also, what I
> wanted to fix isn't covered by the ITU conformance, its mostly resiliance
> requirement. But this should remove some of the IOMMU fault on broken streams
> and make it less likely to use references that don't exists or aren't set what
> we expect. After this change, the driver was getting more stable, and results
> was also more reproducible (specially in parallel decode case, which I use to
> speed up testing).
>

Thanks for the details. This sounds like something that could
be added to the commit description itself.

> >
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> >
> > Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Thanks for the review.
>

No problem :)

> >
> > Thanks,
> > Ezequiel
> >
> > > ---
> > >  drivers/staging/media/rkvdec/rkvdec-h264.c | 33 ++++++++++++++++------
> > >  1 file changed, 24 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > index dff89732ddd0..bcde37d72244 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > @@ -112,6 +112,7 @@ struct rkvdec_h264_run {
> > >     const struct v4l2_ctrl_h264_sps *sps;
> > >     const struct v4l2_ctrl_h264_pps *pps;
> > >     const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
> > > +   int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
> > >  };
> > >
> > >  struct rkvdec_h264_ctx {
> > > @@ -725,6 +726,26 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
> > >     }
> > >  }
> > >
> > > +static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > > +                          struct rkvdec_h264_run *run)
> > > +{
> > > +   const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
> > > +   u32 i;
> > > +
> > > +   for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
> > > +           struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > +           const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > > +           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)
> > > +                   buf_idx = vb2_find_timestamp(cap_q,
> > > +                                                dpb[i].reference_ts, 0);
> > > +
> > > +           run->ref_buf_idx[i] = buf_idx;
> > > +   }
> > > +}
> > > +
> > >  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > >                         struct rkvdec_h264_run *run)
> > >  {
> > > @@ -762,7 +783,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > >
> > >     for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > >             for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > -                   u8 dpb_valid = 0;
> > > +                   bool dpb_valid = run->ref_buf_idx[i] >= 0;
> > >                     u8 idx = 0;
> > >
> > >                     switch (j) {
> > > @@ -779,8 +800,6 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
> > >
> > >                     if (idx >= ARRAY_SIZE(dec_params->dpb))
> > >                             continue;
> > > -                   dpb_valid = !!(dpb[idx].flags &
> > > -                                  V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
> > >
> > >                     set_ps_field(hw_rps, DPB_INFO(i, j),
> > >                                  idx | dpb_valid << 4);
> > > @@ -859,13 +878,8 @@ get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
> > >         unsigned int dpb_idx)
> > >  {
> > >     struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > -   const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
> > >     struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > > -   int buf_idx = -1;
> > > -
> > > -   if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > -           buf_idx = vb2_find_timestamp(cap_q,
> > > -                                        dpb[dpb_idx].reference_ts, 0);
> > > +   int buf_idx = run->ref_buf_idx[dpb_idx];
> > >
> > >     /*
> > >      * If a DPB entry is unused or invalid, address of current destination
> > > @@ -1102,6 +1116,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);
> > >     config_registers(ctx, &run);
> > >
> > > --
> > > 2.34.1
> > >
>
diff mbox series

Patch

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index dff89732ddd0..bcde37d72244 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -112,6 +112,7 @@  struct rkvdec_h264_run {
 	const struct v4l2_ctrl_h264_sps *sps;
 	const struct v4l2_ctrl_h264_pps *pps;
 	const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix;
+	int ref_buf_idx[V4L2_H264_NUM_DPB_ENTRIES];
 };
 
 struct rkvdec_h264_ctx {
@@ -725,6 +726,26 @@  static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 	}
 }
 
+static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
+			       struct rkvdec_h264_run *run)
+{
+	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
+	u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
+		struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
+		const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
+		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)
+			buf_idx = vb2_find_timestamp(cap_q,
+						     dpb[i].reference_ts, 0);
+
+		run->ref_buf_idx[i] = buf_idx;
+	}
+}
+
 static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			    struct rkvdec_h264_run *run)
 {
@@ -762,7 +783,7 @@  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 
 	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
 		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
-			u8 dpb_valid = 0;
+			bool dpb_valid = run->ref_buf_idx[i] >= 0;
 			u8 idx = 0;
 
 			switch (j) {
@@ -779,8 +800,6 @@  static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 
 			if (idx >= ARRAY_SIZE(dec_params->dpb))
 				continue;
-			dpb_valid = !!(dpb[idx].flags &
-				       V4L2_H264_DPB_ENTRY_FLAG_ACTIVE);
 
 			set_ps_field(hw_rps, DPB_INFO(i, j),
 				     idx | dpb_valid << 4);
@@ -859,13 +878,8 @@  get_ref_buf(struct rkvdec_ctx *ctx, struct rkvdec_h264_run *run,
 	    unsigned int dpb_idx)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
-	const struct v4l2_h264_dpb_entry *dpb = run->decode_params->dpb;
 	struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
-	int buf_idx = -1;
-
-	if (dpb[dpb_idx].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
-		buf_idx = vb2_find_timestamp(cap_q,
-					     dpb[dpb_idx].reference_ts, 0);
+	int buf_idx = run->ref_buf_idx[dpb_idx];
 
 	/*
 	 * If a DPB entry is unused or invalid, address of current destination
@@ -1102,6 +1116,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);
 	config_registers(ctx, &run);