On Mon, 21 Feb 2022 12:56:43 +0100 Max Krummenacher <max.oss.09@xxxxxxxxx> wrote: > Am Montag, den 21.02.2022, 09:29 +0100 schrieb Boris Brezillon: > > Hello Christoph, > > > > On Sat, 19 Feb 2022 09:28:44 +0000 > > Christoph Niedermaier <cniedermaier@xxxxxxxxxxxxxxxxxx> wrote: > > > > > From: Max Krummenacher [mailto:max.oss.09@xxxxxxxxx] > > > 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@xxxxxxxxxxxxxxxxxx> > > > > > Cc: Marek Vasut <marex@xxxxxxx> > > > > > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> > > > > > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > > > > Cc: David Airlie <airlied@xxxxxxxx> > > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > > Cc: Shawn Guo <shawnguo@xxxxxxxxxx> > > > > > Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > > > > Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx> > > > > > Cc: Fabio Estevam <festevam@xxxxxxxxx> > > > > > Cc: NXP Linux Team <linux-imx@xxxxxxx> > > > > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > > > > To: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > > > > --- > > > > > 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@xxxxxxxxxxx> > > > > > > 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. > > Note that {HV}SYNC polarities are taken from the timings '.flag' field > The bus_flags do not carry > that information. > (drivers/gpu/ipu-v3/ipu-di.c:611: if (sig->mode.flags & > DISPLAY_FLAGS_HSYNC_HIGH)) > > The new flags DRM_BUS_FLAG_SYNC_DRIVE_{POS,NEG}EDGE are siblings to > DRM_BUS_FLAG_PIXDATA_DRIVE_{POS,NEG}EDGE and would allow to specify > on which pixelclock edge the sync signals are to be driven. > Before that addition it was implicitly assumed that the sync signals > and data signals would be driven on the same clock edge. Oh, ok. > The way I read the IPU driver it is not > foreseen that the data lines > and sync lines are driven by a different clock edge. > > I personally would just drop the sanity test on > bus_flags. This is what > this patch proposes. Sounds fine. Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>