Re: [PATCH 03/10] staging: media: cedrus: h264: mark long-term references in the frame info

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

[-- Attachment #1: Type: text/plain, Size: 4878 bytes --]

Le mardi 03 novembre 2026 à 19:06 +0100, Jernej Škrabec a écrit :
> Dne ponedeljek, 2. november 2026 ob 19:50:44 Srednjeevropski standardni čas je Nicolas Dufresne napisal(a):
> > Hi,
> > 
> > Le lundi 02 novembre 2026 à 18:02 +0100, Jernej Skrabec a écrit :
> > > The frame info of a reference has a reference type for each field, in
> > > bits 1:0 for the top field and 5:4 for the bottom one, which is 1 for a
> > > long-term reference. The driver never sets it, so the hardware handles
> > > long-term references like short-term ones, also where they differ, like
> > > in temporal direct and implicit weighted prediction. The vendor library
> > > sets it.
> > > 
> > > This fixes MR8_BT_B and MR9_BT_B of the H.264 conformance suite.
> > > 
> > > Fixes: 6eb9b758e307 ("media: cedrus: Add H264 decoding support")
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 12 +++++++++++-
> > >  drivers/staging/media/sunxi/cedrus/cedrus_regs.h |  8 ++++++++
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > index 892193c12b2f..a716955655f7 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -69,13 +69,21 @@ static void cedrus_fill_ref_pic(struct cedrus_ctx *ctx,
> > >  				struct cedrus_buffer *buf,
> > >  				unsigned int top_field_order_cnt,
> > >  				unsigned int bottom_field_order_cnt,
> > > +				bool long_term,
> > >  				struct cedrus_h264_sram_ref_pic *pic)
> > >  {
> > >  	struct vb2_buffer *vbuf = &buf->m2m_buf.vb.vb2_buf;
> > > +	u32 frame_info, ref_type;
> > >  
> > >  	pic->top_field_order_cnt = cpu_to_le32(top_field_order_cnt);
> > >  	pic->bottom_field_order_cnt = cpu_to_le32(bottom_field_order_cnt);
> > > -	pic->frame_info = cpu_to_le32(buf->codec.h264.pic_type << 8);
> > > +
> > > +	ref_type = long_term ? VE_H264_FRAME_INFO_REF_TYPE_LONG_TERM :
> > > +			       VE_H264_FRAME_INFO_REF_TYPE_SHORT_TERM;
> > > +	frame_info = VE_H264_FRAME_INFO_TOP_REF_TYPE(ref_type) |
> > > +		     VE_H264_FRAME_INFO_BOT_REF_TYPE(ref_type) |
> > > +		     VE_H264_FRAME_INFO_PIC_TYPE(buf->codec.h264.pic_type);
> > > +	pic->frame_info = cpu_to_le32(frame_info);
> > >  
> > >  	pic->luma_ptr = cpu_to_le32(cedrus_buf_addr(vbuf, &ctx->dst_fmt, 0));
> > >  	pic->chroma_ptr = cpu_to_le32(cedrus_buf_addr(vbuf, &ctx->dst_fmt, 1));
> > > @@ -128,6 +136,7 @@ static int cedrus_write_frame_list(struct cedrus_ctx *ctx,
> > >  		cedrus_fill_ref_pic(ctx, cedrus_buf,
> > >  				    dpb->top_field_order_cnt,
> > >  				    dpb->bottom_field_order_cnt,
> > > +				    dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM,
> > >  				    &pic_list[position]);
> > >  	}
> > >  
> > > @@ -174,6 +183,7 @@ static int cedrus_write_frame_list(struct cedrus_ctx *ctx,
> > >  	cedrus_fill_ref_pic(ctx, output_buf,
> > >  			    decode->top_field_order_cnt,
> > >  			    decode->bottom_field_order_cnt,
> > > +			    false,
> > 
> > nit: Not a hard objection, but I really hate boolean parameters. It could be a
> > simple as:
> > 
> > +			    false, /* short term reference */
> > 
> > Or using an enum. My preference being documented here, its yours to decide.
> 
> I'll use VE_H264_FRAME_INFO_REF_TYPE_* as parameter. I have to fix sashiko
> issue anyway.
> 
> I'll keep your Reviewed-by, unless you object.

wfm

> 
> Best regards,
> Jernej
> 
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > >  			    &pic_list[position]);
> > >  
> > >  	cedrus_h264_write_sram(dev, CEDRUS_SRAM_H264_FRAMEBUFFER_LIST,
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > index 05e6cbc548ab..961fe023bb43 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > @@ -616,6 +616,14 @@
> > >  #define VE_AVC_SRAM_PORT_OFFSET		0x2e0
> > >  #define VE_AVC_SRAM_PORT_DATA		0x2e4
> > >  
> > > +/* Frame info word of a frame buffer list entry in the AVC SRAM */
> > > +#define VE_H264_FRAME_INFO_PIC_TYPE(v)		SHIFT_AND_MASK_BITS(v, 9, 8)
> > > +#define VE_H264_FRAME_INFO_BOT_REF_TYPE(v)	SHIFT_AND_MASK_BITS(v, 5, 4)
> > > +#define VE_H264_FRAME_INFO_TOP_REF_TYPE(v)	SHIFT_AND_MASK_BITS(v, 1, 0)
> > > +
> > > +#define VE_H264_FRAME_INFO_REF_TYPE_SHORT_TERM	0
> > > +#define VE_H264_FRAME_INFO_REF_TYPE_LONG_TERM	1
> > > +
> > >  #define VE_VP8_PPS			0x214
> > >  #define VE_VP8_PPS_PIC_TYPE_P_FRAME		BIT(31)
> > >  #define VE_VP8_PPS_LAST_SHARPNESS_LEVEL(v)	SHIFT_AND_MASK_BITS(v, 30, 28)
> > 
> 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]