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

[Jernej Škrabec]

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@xxxxxxxxx>
> > ---
> >  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.

> 
> 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.

Best regards,
Jernej

> 
> Nicolas
> 
> >  
> >  	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> >  
>