Re: [PATCH] media: verisilicon: HEVC: Initialize start_bit field

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

Le lundi 20 janvier 2025 à 08:15 -0600, Adam Ford a écrit :
> On Mon, Jan 20, 2025 at 8:10 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > Le lundi 20 janvier 2025 à 11:00 +0100, Benjamin Gaignard a écrit :
> > > Le 20/01/2025 à 09:10, Benjamin Gaignard a écrit :
> > > > Always set start_bit field to 0, if not it could lead to corrupted frames
> > > > specially when decoding VP9 bitstreams at the same time since VP9 driver
> > > > set it for it own purpose.
> > 
> >                its
> > 
> 
> Does this impact the Fluster score?

Only if you concurrently test VP9 and HEVC at the same time. Fluster does not
parallelized across codecs, could be a nice addition to catch more issues.

Nicolas

> 
> > > > 
> > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > 
> > > I could add this tag:
> > > 
> > > Fixes: cb5dd5a0fa518 ("media: hantro: Introduce G2/HEVC decoder")
> > 
> > I have tested this on IMX8MQ board using the following GStreamer pipeline.
> > Before the change, the HEVC window was entirely corrupted. The streams don't
> > matter as long as they use both HEVC and VP9 codec.
> > 
> > gst-launch-1.0 \
> >   filesrc location=hevc.mp4 ! parsebin ! v4l2slh265dec ! fakevideosink \
> >   filesrc location=vp9.mkv ! parsebin ! v4l2slvp9dec ! fakevideosink
> > 
> > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > 
> > > 
> > > > ---
> > > >   drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> > > > index 85a44143b378..0e212198dd65 100644
> > > > --- a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> > > > +++ b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> > > > @@ -518,6 +518,7 @@ static void set_buffers(struct hantro_ctx *ctx)
> > > >     hantro_reg_write(vpu, &g2_stream_len, src_len);
> > > >     hantro_reg_write(vpu, &g2_strm_buffer_len, src_buf_len);
> > > >     hantro_reg_write(vpu, &g2_strm_start_offset, 0);
> > > > +   hantro_reg_write(vpu, &g2_start_bit, 0);
> > > >     hantro_reg_write(vpu, &g2_write_mvs_e, 1);
> > 
> > I've also crossed against "decoder_swreg_map_g2.xlsx" documentation, if you are
> > lucky to have access to that, and within swreg5 there is only g2_tempor_mvp_e
> > that is also shared, and its already being set in both drivers.
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > 
> > > > 
> > > >     hantro_write_addr(vpu, G2_TILE_SIZES_ADDR, ctx->hevc_dec.tile_sizes.dma);
> > > 
> > 
> >