Re: [PATCH v2 01/13] media: rockchip: rga: fix swizzling for RGB formats

From: Deborah Brouwer <deborah.brouwer@collabora.com>
To: Michael Tretter <m.tretter@pengutronix.de>
Cc: Jacob Chen <jacob-chen@iotwrt.com>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Shengyu Qu <wiagn233@outlook.com>,
	Nicolas Frattaroli <frattaroli.nicolas@gmail.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Diederik de Haas <didi.debian@cknow.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de,
	nicolas.dufresne@collabora.com
Subject: Re: [PATCH v2 01/13] media: rockchip: rga: fix swizzling for RGB formats
Date: Fri, 24 Nov 2023 16:39:57 -0800	[thread overview]
Message-ID: <ZWFCXaB5KpRL0Ssf@mz550> (raw)
In-Reply-To: <20230914-rockchip-rga-multiplanar-v2-1-bbfa6abf8bbf@pengutronix.de>

On Fri, Oct 13, 2023 at 01:00:22PM +0200, Michael Tretter wrote:

Hi Michael,

> When using 32 bit RGB formats, the RGA on the rk3568 produces wrong
> colors as the wrong color channels are read or written.  The reason is
> that the format description for the channel swizzeling is wrong and the
> wrong bits are configured. For example, when converting ARGB32 to NV12,
> the alpha channel is used as blue channel.. This doesn't happen if the
> color format is the same on both sides.
> 
> Fix the color_swap settings of the formats to correctly handle 32 bit
> RGB formats.

You mentioned in your cover letter that you only tested this on rk3568
so I tested it on an rk3399 as well.

I used gstreamer to convert each of the formats that you changed
(V4L2_PIX_FMT_ARGB32, V4L2_PIX_FMT_ABGR32, V4L2_PIX_FMT_XBGR32) into
each of:
BA24, AR24, XR24, RGB3, BGRI 3, NV21, YU12, 422P, YV12, and NM12.

All the colour conversions are working and much better than before which
I could see was definitely broken.

Tested-by: Deborah Brouwer <deborah.brouwer@collabora.com>

> 
> For RGA_COLOR_FMT_XBGR8888, the RGA_COLOR_ALPHA_SWAP bit doesn't have an
> effect. Thus, it isn't possible to handle the V4L2_PIX_FMT_XRGB32. Thus,
> it is removed from the list of supported formats.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> Changes in v2: None
> ---
>  drivers/media/platform/rockchip/rga/rga.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index f1c532a5802a..25f5b5eebf13 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -184,25 +184,16 @@ static int rga_setup_ctrls(struct rga_ctx *ctx)
>  static struct rga_fmt formats[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_ARGB32,
> -		.color_swap = RGA_COLOR_RB_SWAP,
> +		.color_swap = RGA_COLOR_ALPHA_SWAP,
>  		.hw_format = RGA_COLOR_FMT_ABGR8888,
>  		.depth = 32,
>  		.uv_factor = 1,
>  		.y_div = 1,
>  		.x_div = 1,
>  	},
> -	{
> -		.fourcc = V4L2_PIX_FMT_XRGB32,
> -		.color_swap = RGA_COLOR_RB_SWAP,
> -		.hw_format = RGA_COLOR_FMT_XBGR8888,
> -		.depth = 32,
> -		.uv_factor = 1,
> -		.y_div = 1,
> -		.x_div = 1,
> -	},
>  	{
>  		.fourcc = V4L2_PIX_FMT_ABGR32,
> -		.color_swap = RGA_COLOR_ALPHA_SWAP,
> +		.color_swap = RGA_COLOR_RB_SWAP,
>  		.hw_format = RGA_COLOR_FMT_ABGR8888,
>  		.depth = 32,
>  		.uv_factor = 1,
> @@ -211,7 +202,7 @@ static struct rga_fmt formats[] = {
>  	},
>  	{
>  		.fourcc = V4L2_PIX_FMT_XBGR32,
> -		.color_swap = RGA_COLOR_ALPHA_SWAP,
> +		.color_swap = RGA_COLOR_RB_SWAP,
>  		.hw_format = RGA_COLOR_FMT_XBGR8888,
>  		.depth = 32,
>  		.uv_factor = 1,
> 
> -- 
> 2.39.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel