drm/imx: parallel-display: Adjust bus_flags and bus_format handling
diff mbox series

Message ID 20191114131751.26746-1-marex@denx.de
State New
Headers show
Series
  • drm/imx: parallel-display: Adjust bus_flags and bus_format handling
Related show

Commit Message

Marek Vasut Nov. 14, 2019, 1:17 p.m. UTC
The bus_flags and bus_format handling logic does not seem to cover
all potential usecases. Specifically, this seems to fail with an
"edt,etm0700g0edh6" display attached to an 24bit display interface,
with interface-pix-fmt = "rgb24" set in DT.

In this specific setup, the panel-simple.c driver entry for the display
sets .bus_flags to non-zero value. However, as imxpd->bus_format is set
from the DT property "interface-pix-fmt", imx_pd_encoder_atomic_check()
will set imx_crtc_state->bus_flags = imxpd->bus_flags even though the
imxpd->bus_flags is zero, while the di->bus_flags is correctly set by
the panel-simple.c and non-zero. The result is incorrect flags being
used for the display configuration and thus an image corruption.
(Specifically, DRM_BUS_FLAG_PIXDATA_POSEDGE is not propagated and thus
the ipuv3 clocks pixels on the wrong edge).

This patch fixes the problem by overriding the imx_crtc_state->bus_format
from the imxpd->bus_format only if the DT property "interface-pix-fmt" is
present or if the DI provides no formats. Similarly for bus_flags, which
are set from imxpd->bus_flags only if the DI provides no formats.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
To: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/imx/parallel-display.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Stefan Agner Nov. 14, 2019, 1:50 p.m. UTC | #1
On 2019-11-14 14:17, Marek Vasut wrote:
> The bus_flags and bus_format handling logic does not seem to cover
> all potential usecases. Specifically, this seems to fail with an
> "edt,etm0700g0edh6" display attached to an 24bit display interface,
> with interface-pix-fmt = "rgb24" set in DT.
> 
> In this specific setup, the panel-simple.c driver entry for the display
> sets .bus_flags to non-zero value. However, as imxpd->bus_format is set
> from the DT property "interface-pix-fmt", imx_pd_encoder_atomic_check()
> will set imx_crtc_state->bus_flags = imxpd->bus_flags even though the
> imxpd->bus_flags is zero, while the di->bus_flags is correctly set by
> the panel-simple.c and non-zero. The result is incorrect flags being
> used for the display configuration and thus an image corruption.
> (Specifically, DRM_BUS_FLAG_PIXDATA_POSEDGE is not propagated and thus
> the ipuv3 clocks pixels on the wrong edge).
> 
> This patch fixes the problem by overriding the imx_crtc_state->bus_format
> from the imxpd->bus_format only if the DT property "interface-pix-fmt" is
> present or if the DI provides no formats. Similarly for bus_flags, which
> are set from imxpd->bus_flags only if the DI provides no formats.

So this basically prioritizes imxpd->bus_format over what the display
provides? Is this correct in all situations?

I was thinking that interface-pix-fmt is the legacy way to define the
bus format and it should be provided by the display nowadays.

However, I guess there is the case where you connect a 18-bit display to
a 24-bit bus (leaving some bits unconnected). Depending on how the
colors/bits are distributed one cannot use 18-bit mode on SoC side but
has to use 24-bit. So the bus format becomes a connection specific
property... I guess the interface-pix-fmt can serve that role. 

--
Stefan


> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> To: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/imx/parallel-display.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/parallel-display.c
> b/drivers/gpu/drm/imx/parallel-display.c
> index 35518e5de356..92f00b12c068 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -113,13 +113,16 @@ static int imx_pd_encoder_atomic_check(struct
> drm_encoder *encoder,
>  	struct drm_display_info *di = &conn_state->connector->display_info;
>  	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
>  
> -	if (!imxpd->bus_format && di->num_bus_formats) {
> -		imx_crtc_state->bus_flags = di->bus_flags;
> +	if (imxpd->bus_format || !di->num_bus_formats)
> +		imx_crtc_state->bus_format = imxpd->bus_format;
> +	else
>  		imx_crtc_state->bus_format = di->bus_formats[0];
> -	} else {
> +
> +	if (di->num_bus_formats)
> +		imx_crtc_state->bus_flags = di->bus_flags;
> +	else
>  		imx_crtc_state->bus_flags = imxpd->bus_flags;
> -		imx_crtc_state->bus_format = imxpd->bus_format;
> -	}
> +
>  	imx_crtc_state->di_hsync_pin = 2;
>  	imx_crtc_state->di_vsync_pin = 3;

Patch
diff mbox series

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 35518e5de356..92f00b12c068 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -113,13 +113,16 @@  static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder,
 	struct drm_display_info *di = &conn_state->connector->display_info;
 	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
 
-	if (!imxpd->bus_format && di->num_bus_formats) {
-		imx_crtc_state->bus_flags = di->bus_flags;
+	if (imxpd->bus_format || !di->num_bus_formats)
+		imx_crtc_state->bus_format = imxpd->bus_format;
+	else
 		imx_crtc_state->bus_format = di->bus_formats[0];
-	} else {
+
+	if (di->num_bus_formats)
+		imx_crtc_state->bus_flags = di->bus_flags;
+	else
 		imx_crtc_state->bus_flags = imxpd->bus_flags;
-		imx_crtc_state->bus_format = imxpd->bus_format;
-	}
+
 	imx_crtc_state->di_hsync_pin = 2;
 	imx_crtc_state->di_vsync_pin = 3;