linux-kernel - Re: [PATCH v2 13/23] media: rkvdec: h264: Fix dpb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <92001085580ac1005596b34f79f1dadcc4b3f3c9.camel@collabora.com>
Date:   Tue, 05 Apr 2022 11:10:57 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...labora.com,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/23] media: rkvdec: h264: Fix dpb_valid
 implementation

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@...labora.com>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke@...labora.com>
> 
> Fixes: cd33c830448ba ("media: rkvdec: Add the rkvdec driver")
> Reviewed-by: Ezequiel Garcia <ezequiel@...guardiasur.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
> >