Re: [PATCH 05/10] staging: media: cedrus: mpeg2: derive the field order of field pictures

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

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

Le lundi 02 novembre 2026 à 18:02 +0100, Jernej Skrabec a écrit :
> The hardware uses the top field first bit of the picture header to tell
> the first field of a frame from the second one. However,
> top_field_first is always 0 in field pictures, so userspace passes
> V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST cleared, and the second field of
> every top field first pair is predicted from the wrong reference.
> 
> Detect the second field in the driver instead: it has the opposite
> parity of the previous field and is decoded into the same capture
> buffer. This works with unmodified userspace.
> 
> Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Its quite likely other MPEG-2 decoder are affected. Would have been nice to pass
the picture structure explicitly in the decode params in the first place. Not
your fault.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  5 +++
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 32 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 54f867949fba..a523b56a9b8c 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -144,6 +144,11 @@ struct cedrus_ctx {
>  			void		*entry_points_buf;
>  			dma_addr_t	entry_points_buf_addr;
>  		} h265;
> +		struct {
> +			bool		pair_pending;
> +			unsigned int	pair_dst;
> +			u8		pair_structure;
> +		} mpeg2;
>  		struct {
>  			unsigned int	last_frame_p_type;
>  			unsigned int	last_filter_type;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index ca66a3c6b830..4c8accee5389 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -58,7 +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;
> +	bool field, tff;
>  	u32 reg;
>  
>  	seq = run->mpeg2.sequence;
> @@ -87,6 +87,26 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  		cedrus_write(dev, VE_DEC_MPEG_IQMINPUT, reg);
>  	}
>  
> +	/*
> +	 * top_field_first is always 0 in field pictures, but the hardware
> +	 * uses this bit to tell the first field of a frame from the second.
> +	 * A field is the second one when it has the opposite parity of the
> +	 * previous field and is decoded into the same capture buffer.
> +	 */
> +	if (pic->picture_structure == V4L2_MPEG2_PIC_FRAME) {
> +		tff = !!(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
> +		ctx->codec.mpeg2.pair_pending = false;
> +	} else {
> +		bool second = ctx->codec.mpeg2.pair_pending &&
> +			      ctx->codec.mpeg2.pair_structure != pic->picture_structure &&
> +			      ctx->codec.mpeg2.pair_dst == run->dst->vb2_buf.index;
> +
> +		tff = (pic->picture_structure == V4L2_MPEG2_PIC_TOP_FIELD) != second;
> +		ctx->codec.mpeg2.pair_pending = !second;
> +		ctx->codec.mpeg2.pair_structure = pic->picture_structure;
> +		ctx->codec.mpeg2.pair_dst = run->dst->vb2_buf.index;
> +	}
> +
>  	/* Set MPEG picture header. */
>  
>  	reg = VE_DEC_MPEG_MP12HDR_SLICE_TYPE(pic->picture_coding_type);
> @@ -96,7 +116,7 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	reg |= VE_DEC_MPEG_MP12HDR_F_CODE(1, 1, pic->f_code[1][1]);
>  	reg |= VE_DEC_MPEG_MP12HDR_INTRA_DC_PRECISION(pic->intra_dc_precision);
>  	reg |= VE_DEC_MPEG_MP12HDR_INTRA_PICTURE_STRUCTURE(pic->picture_structure);
> -	reg |= VE_DEC_MPEG_MP12HDR_TOP_FIELD_FIRST(pic->flags & V4L2_MPEG2_PIC_FLAG_TOP_FIELD_FIRST);
> +	reg |= VE_DEC_MPEG_MP12HDR_TOP_FIELD_FIRST(tff);
>  	reg |= VE_DEC_MPEG_MP12HDR_FRAME_PRED_FRAME_DCT(pic->flags & V4L2_MPEG2_PIC_FLAG_FRAME_PRED_DCT);
>  	reg |= VE_DEC_MPEG_MP12HDR_CONCEALMENT_MOTION_VECTORS(pic->flags & V4L2_MPEG2_PIC_FLAG_CONCEALMENT_MV);
>  	reg |= VE_DEC_MPEG_MP12HDR_Q_SCALE_TYPE(pic->flags & V4L2_MPEG2_PIC_FLAG_Q_SCALE_TYPE);
> @@ -186,6 +206,13 @@ static int cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	return 0;
>  }
>  
> +static int cedrus_mpeg2_start(struct cedrus_ctx *ctx)
> +{
> +	memset(&ctx->codec.mpeg2, 0, sizeof(ctx->codec.mpeg2));
> +
> +	return 0;
> +}
> +
>  static void cedrus_mpeg2_trigger(struct cedrus_ctx *ctx)
>  {
>  	struct cedrus_dev *dev = ctx->dev;
> @@ -203,5 +230,6 @@ struct cedrus_dec_ops cedrus_dec_ops_mpeg2 = {
>  	.irq_disable	= cedrus_mpeg2_irq_disable,
>  	.irq_status	= cedrus_mpeg2_irq_status,
>  	.setup		= cedrus_mpeg2_setup,
> +	.start		= cedrus_mpeg2_start,
>  	.trigger	= cedrus_mpeg2_trigger,
>  };

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