Coda: imx53 plays video with incorrect colors

[Philipp Zabel]

Hi Fabio,

On Mon, 2021-01-18 at 07:57 -0300, Fabio Estevam wrote:
> Hi Philipp,
> 
> On Sun, Jan 17, 2021 at 10:18 PM Nicolas Dufresne <nicolas at ndufresne.ca> wrote:
> 
> > At first sight there is nothing wrong from GStreamer happening. It
> > picks i420, pass it to KMS and it comes out wrong, first suspect
> > would be the display driver. Understand that yuv formats are often
> > unused and untested as most display server / compositer uses rgb
> > type of formats converting with GPU.

This is both a bug and a missing feature in the imx-drm driver. The
driver doesn't support color space conversion on the VGA output and
doesn't report that correctly.

> On i.MX53 I see this 'wrong color' behavior when playing videos into
> TVE as well as parallel display.
>
> Are you able to playback video successfully on i.MX53? If so, could
> you please share your Gstreamer pipeline?

I can reproduce this with

  gst-launch-1.0 videotestsrc ! video/x-raw,format=I420 ! kmssink

and the same for NV12 or YUY2. We are missing color space conversion on
the VGA output.

The IPU only supports color space conversion on one output via the DP.
This path is hard-coded to DI0 in the driver. The direct DC path to DI1,
which TVE/VGA is connected to, does not support CSC.

The driver could be modified to switch the DP->DI0/DC->DI1 mapping
around to DP->DI1/DC->DI0 when required. As a simple test, you can
switch statically with:

----------8<----------
diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index d166ee262ce4..f7c86ef0bed7 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1118,18 +1118,18 @@ static struct ipu_platform_reg client_reg[] = {
 	}, {
 		.pdata = {
 			.di = 0,
-			.dc = 5,
-			.dp = IPU_DP_FLOW_SYNC_BG,
-			.dma[0] = IPUV3_CHANNEL_MEM_BG_SYNC,
+			.dc = 1,
+			.dp = -EINVAL,
+			.dma[0] = IPUV3_CHANNEL_MEM_DC_SYNC,
 			.dma[1] = IPUV3_CHANNEL_MEM_FG_SYNC,
 		},
 		.name = "imx-ipuv3-crtc",
 	}, {
 		.pdata = {
 			.di = 1,
-			.dc = 1,
-			.dp = -EINVAL,
-			.dma[0] = IPUV3_CHANNEL_MEM_DC_SYNC,
+			.dc = 5,
+			.dp = IPU_DP_FLOW_SYNC_BG,
+			.dma[0] = IPUV3_CHANNEL_MEM_BG_SYNC,
 			.dma[1] = -EINVAL,
 		},
 		.name = "imx-ipuv3-crtc",
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 34b4075a6a8e..0a7f48e4aa37 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -364,9 +364,9 @@ int ipu_dc_init(struct ipu_soc *ipu, struct device *dev,
 
 	writel(DC_WR_CH_CONF_WORD_SIZE_24 | DC_WR_CH_CONF_DISP_ID_PARALLEL(1) |
 			DC_WR_CH_CONF_PROG_DI_ID,
-			priv->channels[1].base + DC_WR_CH_CONF);
-	writel(DC_WR_CH_CONF_WORD_SIZE_24 | DC_WR_CH_CONF_DISP_ID_PARALLEL(0),
 			priv->channels[5].base + DC_WR_CH_CONF);
+	writel(DC_WR_CH_CONF_WORD_SIZE_24 | DC_WR_CH_CONF_DISP_ID_PARALLEL(0),
+			priv->channels[1].base + DC_WR_CH_CONF);
 
 	writel(DC_GEN_SYNC_1_6_SYNC | DC_GEN_SYNC_PRIORITY_1,
 		priv->dc_reg + DC_GEN);
---------->8----------

That should enable CSC (and overlay plane) on DI1, and lose it on DI0.

Or, as a workaround, add a v4l2convert element and use the IC to convert
to BGRx between decoder and kmssink.

regards
Philipp