Re: [PATCH 09/10] staging: media: cedrus: h616: use the 256-bit DDR mode

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

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

Hi,

Le lundi 02 novembre 2026 à 18:03 +0100, Jernej Skrabec a écrit :
> With the DDR mode in VE_MODE set to 128-bit, which the driver uses on
> all SoCs, the H616 H.264 engine reads corrupted reference chroma, while
> luma is fine. H.264 and VP8 P frames come out with bit-exact luma, but
> with about 22 dB PSNR chroma. H.265 and MPEG-2 are unaffected.
> 
> The vendor library selects the DDR mode based on the VE IC version. It
> uses the 256-bit mode without the 2 MiB reconstruction write mode on
> the H616, and keeps the 128-bit mode with the 2 MiB reconstruction
> write mode on older SoCs, like the H6. Doing the same on the H616 makes
> all codecs bit-exact (tested with BA_MW_D, CABA2_Sony_E,
> vp80-00-comprehensive-002, AMP_A_Samsung_7 and MEI.stream16).
> 
> Make the VE_MODE DDR settings a variant property and add an H616
> variant, which uses the 256-bit mode. Replace the FIXME about the
> DDR mode with a description of what the vendor library does.
> 
> Fixes: 1f24d4307515 ("staging: media: sunxi: cedrus: add H616 variant")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

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

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c    | 14 +++++++++++++-
>  drivers/staging/media/sunxi/cedrus/cedrus.h    |  2 ++
>  drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 14 ++++++++++----
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index 756243795de8..2bbc79b44f99 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -27,6 +27,7 @@
>  #include "cedrus_video.h"
>  #include "cedrus_dec.h"
>  #include "cedrus_hw.h"
> +#include "cedrus_regs.h"
>  
>  /*
>   * The hardware decodes whatever picture size the bitstream headers describe.
> @@ -697,6 +698,17 @@ static const struct cedrus_variant sun50i_h6_cedrus_variant = {
>  	.mod_rate	= 600000000,
>  };
>  
> +static const struct cedrus_variant sun50i_h616_cedrus_variant = {
> +	.capabilities	= CEDRUS_CAPABILITY_UNTILED |
> +			  CEDRUS_CAPABILITY_MPEG2_DEC |
> +			  CEDRUS_CAPABILITY_H264_DEC |
> +			  CEDRUS_CAPABILITY_H265_DEC |
> +			  CEDRUS_CAPABILITY_H265_10_DEC |
> +			  CEDRUS_CAPABILITY_VP8_DEC,
> +	.mod_rate	= 600000000,
> +	.ve_mode_ddr	= VE_MODE_DDR_MODE_BW_256,
> +};
> +
>  static const struct of_device_id cedrus_dt_match[] = {
>  	{
>  		.compatible = "allwinner,sun4i-a10-video-engine",
> @@ -744,7 +756,7 @@ static const struct of_device_id cedrus_dt_match[] = {
>  	},
>  	{
>  		.compatible = "allwinner,sun50i-h616-video-engine",
> -		.data = &sun50i_h6_cedrus_variant,
> +		.data = &sun50i_h616_cedrus_variant,
>  	},
>  	{ /* sentinel */ }
>  };
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index a523b56a9b8c..d28fe91562d1 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -181,6 +181,7 @@ struct cedrus_dec_ops {
>  struct cedrus_variant {
>  	unsigned int	capabilities;
>  	unsigned int	mod_rate;
> +	u32		ve_mode_ddr;
>  };
>  
>  struct cedrus_dev {
> @@ -204,6 +205,7 @@ struct cedrus_dev {
>  	struct reset_control	*rstc;
>  
>  	unsigned int		capabilities;
> +	u32			ve_mode_ddr;
>  
>  	struct delayed_work	watchdog_work;
>  };
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index 444fb53878d1..b66d855daaf8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -36,11 +36,16 @@ int cedrus_engine_enable(struct cedrus_ctx *ctx)
>  	u32 reg = 0;
>  
>  	/*
> -	 * FIXME: This is only valid on 32-bits DDR's, we should test
> -	 * it on the A13/A33.
> +	 * The DDR mode depends on the SoC. Like the vendor library, use the
> +	 * 128-bit mode with the 2 MiB reconstruction write mode, unless the
> +	 * variant says otherwise.
>  	 */
> -	reg |= VE_MODE_REC_WR_MODE_2MB;
> -	reg |= VE_MODE_DDR_MODE_BW_128;
> +	if (ctx->dev->ve_mode_ddr) {
> +		reg |= ctx->dev->ve_mode_ddr;
> +	} else {
> +		reg |= VE_MODE_REC_WR_MODE_2MB;
> +		reg |= VE_MODE_DDR_MODE_BW_128;
> +	}
>  
>  	switch (ctx->src_fmt.pixelformat) {
>  	case V4L2_PIX_FMT_MPEG2_SLICE:
> @@ -254,6 +259,7 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  		return -EINVAL;
>  
>  	dev->capabilities = variant->capabilities;
> +	dev->ve_mode_ddr = variant->ve_mode_ddr;
>  
>  	irq_dec = platform_get_irq(dev->pdev, 0);
>  	if (irq_dec <= 0)

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