[PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Boris Brezillon]

Hello Christoph,

On Sat, 19 Feb 2022 09:28:44 +0000
Christoph Niedermaier <cniedermaier at dh-electronics.com> wrote:

> From: Max Krummenacher [mailto:max.oss.09 at gmail.com]
> Sent: Wednesday, February 9, 2022 10:38 AM
> >> If display timings were read from the devicetree using
> >> of_get_display_timing() and pixelclk-active is defined
> >> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
> >> automatically generated. Through the function
> >> drm_bus_flags_from_videomode() e.g. called in the
> >> panel-simple driver this flag got into the bus flags,
> >> but then in imx_pd_bridge_atomic_check() the bus flag
> >> check failed and will not initialize the display. The
> >> original commit fe141cedc433 does not explain why this
> >> check was introduced. So remove the bus flags check,
> >> because it stops the initialization of the display with
> >> valid bus flags.
> >>
> >> Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when available")
> >> Signed-off-by: Christoph Niedermaier <cniedermaier at dh-electronics.com>
> >> Cc: Marek Vasut <marex at denx.de>
> >> Cc: Boris Brezillon <boris.brezillon at collabora.com>
> >> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> >> Cc: David Airlie <airlied at linux.ie>
> >> Cc: Daniel Vetter <daniel at ffwll.ch>
> >> Cc: Shawn Guo <shawnguo at kernel.org>
> >> Cc: Sascha Hauer <s.hauer at pengutronix.de>
> >> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> >> Cc: Fabio Estevam <festevam at gmail.com>
> >> Cc: NXP Linux Team <linux-imx at nxp.com>
> >> Cc: linux-arm-kernel at lists.infradead.org
> >> To: dri-devel at lists.freedesktop.org
> >> ---
> >> V2: - Add Boris to the Cc list
> >> ---
> >>  drivers/gpu/drm/imx/parallel-display.c | 8 --------
> >>  1 file changed, 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> >> index a8aba0141ce7..06cb1a59b9bc 100644
> >> --- a/drivers/gpu/drm/imx/parallel-display.c
> >> +++ b/drivers/gpu/drm/imx/parallel-display.c
> >> @@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
> >>       if (!imx_pd_format_supported(bus_fmt))
> >>               return -EINVAL;
> >>
> >> -     if (bus_flags &
> >> -         ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
> >> -           DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
> >> -           DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
> >> -             dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
> >> -             return -EINVAL;
> >> -     }
> >> -
> >>       bridge_state->output_bus_cfg.flags = bus_flags;
> >>       bridge_state->input_bus_cfg.flags = bus_flags;
> >>       imx_crtc_state->bus_flags = bus_flags;  
> > 
> > Tested on a Colibri iMX6DL with a panel-dpi based panel.
> > 
> > Tested-by: Max Krummenacher <max.krummenacher at toradex.com>  
> 
> I still ask myself why this bus flag check is in the code.
> Is there a reason not to remove that?

The reasoning was that DE_{LOW,HIGH} and
FLAG_PIXDATA_DRIVE_{POS,NEG}EDGE were the only bus_flags taken into
account by the crtc logic, so anything else is simply ignored. This was
definitely wrong since the driver supports at least one of the VSYNC
polarity (perhaps both if there's a way to configure it that's not
hooked-up yet).

So I guess figuring out the default VSYNC polarity and accepting the
according DISPLAY_FLAGS_SYNC_XXXEDGE flag is what makes most sense here.

Regards,

Boris