Re: [PATCH] drm/imx: parallel-display: Adjust bus_flags and bus_format handling

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philipp,

(CC'ing Boris)

On Mon, Mar 09, 2020 at 11:50:59AM +0100, Philipp Zabel wrote:
> 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

Things are funny sometimes, I've run into the exact same problem with a
different display controller today.

Shouldn't we use the data-shift property from [1] to specify this ?
Combined with Boris' bus format negotiation for bridges, I think we
would have all the components in place to solve this problem properly.
Bonus points if we can get a helper function that CRTC code can call to
get the bus format they should use without having to care about the
details (and just to be clear, no yak shaving here, I'm not asking Marek
to implement such a helper, it's not a blocking issue).

> 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.

That's a good point I haven't thought about, and I agree it makes sense.
It means that display controller drivers will have to explicitly support
MEDIA_BUS_FMT_RGB666_1X24_CPADLO and map it to MEDIA_BUS_FMT_RGB888_1X24
if the hardware doesn't support that feature, but I don't think that's a
big issue.

> > 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(-)
> > 
> > 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,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel