Re: [PATCH 04/10] staging: media: cedrus: mpeg2: use the field height for field pictures

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

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

Le mardi 03 novembre 2026 à 19:12 +0100, Jernej Škrabec a écrit :
> Dne ponedeljek, 2. november 2026 ob 19:56:52 Srednjeevropski standardni čas je Nicolas Dufresne napisal(a):
> > Le lundi 02 novembre 2026 à 18:02 +0100, Jernej Skrabec a écrit :
> > > A field picture covers only half of the macroblock rows of a frame, but
> > > the driver programs the coded and the boundary size of the whole frame
> > > for it, as for a frame picture.
> > > 
> > > Program the height of a field for field pictures, like the vendor
> > > library does.
> > > 
> > > Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > index 10e98f08aafc..ca66a3c6b830 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > @@ -58,6 +58,7 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> > >  	struct vb2_queue *vq;
> > >  	const u8 *matrix;
> > >  	unsigned int i;
> > > +	bool field;
> > >  	u32 reg;
> > >  
> > >  	seq = run->mpeg2.sequence;
> > > @@ -108,13 +109,21 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> > >  
> > >  	/* Set frame dimensions. */
> > >  
> > > +	/*
> > > +	 * Field pictures cover half of the frame's macroblock rows; the
> > > +	 * vendor library halves both the coded and the boundary height.
> > > +	 */
> > > +	field = pic->picture_structure != V4L2_MPEG2_PIC_FRAME;
> > > +
> > >  	reg = VE_DEC_MPEG_PICCODEDSIZE_WIDTH(seq->horizontal_size);
> > > -	reg |= VE_DEC_MPEG_PICCODEDSIZE_HEIGHT(seq->vertical_size);
> > > +	reg |= VE_DEC_MPEG_PICCODEDSIZE_HEIGHT(field ?
> > > +			ALIGN(seq->vertical_size, 32) / 2 : seq->vertical_size);
> > >  
> > >  	cedrus_write(dev, VE_DEC_MPEG_PICCODEDSIZE, reg);
> > >  
> > >  	reg = VE_DEC_MPEG_PICBOUNDSIZE_WIDTH(ctx->src_fmt.width);
> > > -	reg |= VE_DEC_MPEG_PICBOUNDSIZE_HEIGHT(ctx->src_fmt.height);
> > > +	reg |= VE_DEC_MPEG_PICBOUNDSIZE_HEIGHT(field ? ctx->src_fmt.height / 2 :
> > > +						   ctx->src_fmt.height);
> > 
> > I'm not entirely sure, but I see:
> > 
> > 	fsize->stepwise.step_height = 1;
> 
> Where? I don't see this in Cedrus.

In the framesize enumeration, it returns a contiguous dimension set, with a step
of 1, which then requires no special treatment. Other drivers uses:

  struct v4l2_frmsize_stepwise frmsize

in a static per PIX_FMT configuration and helpers in try_fmt. Perhaps worth
reminding the following:

https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
width, height
    *coded* width and height parsed from the stream.

Since the dimension is coded, its quite unlikely Cedrus is correct. Most decoder
don't have postprocessor or croper, this is why stateless is like this. The
display dimension would be something set through S_SELECTION if this was needed
to crop (e.g. Hantro could support that, but currently don't, and the PP needs
16 pixels alignment both directions).

> 
> > 
> > But I don't think interlaced video can have odd height in the first place. But
> > it also have an implication here as you are not rounding up to a multiple of 2.
> > Not sure what is the proper fix, but I have this impression something is
> > missing.
> 
> Odd height shouldn't exist in practice, but I'll use
> DIV_ROUND_UP(ctx->src_fmt.height, 2) in v2 if you agree.

I guess it makes it "safe", but perhaps feed the todo?

> 
> Best regards,
> Jernej
> 
> > 
> > Nicolas
> > 
> > >  
> > >  	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> > >  
> > 
> 
> 
> 
> 

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