Hi Marek,
On Thu, 2019-11-14 at 14:17 +0100, 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.
interface-pix-fmt is a legacy property that was never intended to be
used as an override for the panel bus format. The bus flags were
supposed to be set from the display-timings node, back when there was no
of-graph connected panel at all.
That being said, there isn't really a proper alternative that allows to
override the bus format requested by the panel driver in the device tree
to account for weird wiring. We could reuse the bus-width endpoint
property documented in [1], but that wouldn't completely specify how the
RGB components are to be mapped onto the parallel bus.
[1] Documentation/devicetree/bindings/media/video-interfaces.txt
I do wonder whether for your case it would be better to implement a
MEDIA_BUS_FMT_RGB666_1X24_CPADLO though, to leave the LSBs untouched
instead of risking to dump them into accidental PCB antennae.
> 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@xxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Fabio Estevam <festevam@xxxxxxxxx>
> Cc: NXP Linux Team <linux-imx@xxxxxxx>
> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> ---
> 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)
I see no reason to invert the logic here. Let's keep the common case
first.
> + 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;
So while I don't like this being used at all, I think the patch improves
consistency, as imx_pd_connector_get_modes doesn't allow to override the
panel's modes with DT display-timings either.
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
