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

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

[Marek Vasut]

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

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;
 
-- 
2.24.0.rc1

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

[Marek Vasut]

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

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;
 
-- 
2.24.0.rc1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stefan Agner]

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;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Marek Vasut]

On 11/14/19 2:50 PM, Stefan Agner wrote:
> 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. 

So, can this patch go in ? It's fixing an actual issue for me -- very
much what Stefan described above.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Philipp Zabel]

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@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
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Mon, 9 Mar 2020 21:23:06 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

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

I wonder if we shouldn't take more complex pin mappings into account
now and go directly for a data-mapping property describing those
mappings using a string. This way we'd have a single property that
would work for both fully parallel buses (DPI/RGB) and serial (or
partially parallel) ones (LVDS).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Boris,

On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:
> On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:
> > 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.
> 
> I wonder if we shouldn't take more complex pin mappings into account
> now and go directly for a data-mapping property describing those
> mappings using a string. This way we'd have a single property that
> would work for both fully parallel buses (DPI/RGB) and serial (or
> partially parallel) ones (LVDS).

I'm all for standardization, but I'm not sure data-mapping is the right
property, at least with its current definition. It's really meant to
describe how individual bits are mapped to the LVDS time slots. I'm fine
extending it, but we need to define it clearly. How would you envision
it being used in this case ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Marek Vasut]

On 3/9/20 11:50 AM, Philipp Zabel wrote:
> Hi Marek,

Hi,

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

So when this patch was posted half a year ago, it was needed. I rebased
on current next and this patch is no longer needed as some form of it
got in as part of other patches. This functionality is still broken in
e.g. 5.4 though.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Marek Vasut]

On 3/9/20 9:03 PM, Marek Vasut wrote:
> On 3/9/20 11:50 AM, Philipp Zabel wrote:
>> Hi Marek,
> 
> Hi,
> 
>> 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@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.
> 
> So when this patch was posted half a year ago, it was needed. I rebased
> on current next and this patch is no longer needed as some form of it
> got in as part of other patches. This functionality is still broken in
> e.g. 5.4 though.

Correction, a small subset of this patch is still needed, I'll send a V2.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Mon, 9 Mar 2020 21:59:26 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:
> > On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:  
> > > 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.  
> > 
> > I wonder if we shouldn't take more complex pin mappings into account
> > now and go directly for a data-mapping property describing those
> > mappings using a string. This way we'd have a single property that
> > would work for both fully parallel buses (DPI/RGB) and serial (or
> > partially parallel) ones (LVDS).  
> 
> I'm all for standardization, but I'm not sure data-mapping is the right
> property, at least with its current definition. It's really meant to
> describe how individual bits are mapped to the LVDS time slots. I'm fine
> extending it, but we need to define it clearly. How would you envision
> it being used in this case ?
> 

Well, clearly the data-width/data-shift approach does not solve all
problems: what do you do if the source R pins are connected to the sink
B pins? Well, the first answer would probably be 'have a serious
discussion with the HW designer responsible for this insanity' :-), but
once you've passed this 'WTF' stage, you'll have to find a way to tell
the source component it should use RGBxyx while the sink should use
BGRxyx (or vice-versa). This is something you can't extract that from
those width/shift props though. My suggestion would be to have one
string per MEDIA_BUS_FMT definition, so we can force things at the DT
level if we really have to. That's basically what the interface-pix-fmt
property was doing, except we would standardize the prop and values and
probably provide helpers so bridge elements don't have to parse this
prop manually.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Boris,

On Mon, Mar 09, 2020 at 09:22:18PM +0100, Boris Brezillon wrote:
> On Mon, 9 Mar 2020 21:59:26 +0200 Laurent Pinchart wrote:
> > On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:
> > > On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:  
> > > > 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.  
> > > 
> > > I wonder if we shouldn't take more complex pin mappings into account
> > > now and go directly for a data-mapping property describing those
> > > mappings using a string. This way we'd have a single property that
> > > would work for both fully parallel buses (DPI/RGB) and serial (or
> > > partially parallel) ones (LVDS).  
> > 
> > I'm all for standardization, but I'm not sure data-mapping is the right
> > property, at least with its current definition. It's really meant to
> > describe how individual bits are mapped to the LVDS time slots. I'm fine
> > extending it, but we need to define it clearly. How would you envision
> > it being used in this case ?
> 
> Well, clearly the data-width/data-shift approach does not solve all
> problems: what do you do if the source R pins are connected to the sink
> B pins? Well, the first answer would probably be 'have a serious
> discussion with the HW designer responsible for this insanity' :-), but
> once you've passed this 'WTF' stage, you'll have to find a way to tell
> the source component it should use RGBxyx while the sink should use
> BGRxyx (or vice-versa). This is something you can't extract that from
> those width/shift props though. My suggestion would be to have one
> string per MEDIA_BUS_FMT definition, so we can force things at the DT
> level if we really have to. That's basically what the interface-pix-fmt
> property was doing, except we would standardize the prop and values and
> probably provide helpers so bridge elements don't have to parse this
> prop manually.

I don't think that would work in the general case though. We may want to
use different formats and pick one of them at runtime based on external
information (for instance when the sink can accept both RGB and YUV),
hardcoding formats in DT isn't a good option. We instead need to add
information to DT to specify how lines are connected, and deduce formats
based on that.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Mon, 9 Mar 2020 22:32:11 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Mon, Mar 09, 2020 at 09:22:18PM +0100, Boris Brezillon wrote:
> > On Mon, 9 Mar 2020 21:59:26 +0200 Laurent Pinchart wrote:  
> > > On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:  
> > > > On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:    
> > > > > 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.    
> > > > 
> > > > I wonder if we shouldn't take more complex pin mappings into account
> > > > now and go directly for a data-mapping property describing those
> > > > mappings using a string. This way we'd have a single property that
> > > > would work for both fully parallel buses (DPI/RGB) and serial (or
> > > > partially parallel) ones (LVDS).    
> > > 
> > > I'm all for standardization, but I'm not sure data-mapping is the right
> > > property, at least with its current definition. It's really meant to
> > > describe how individual bits are mapped to the LVDS time slots. I'm fine
> > > extending it, but we need to define it clearly. How would you envision
> > > it being used in this case ?  
> > 
> > Well, clearly the data-width/data-shift approach does not solve all
> > problems: what do you do if the source R pins are connected to the sink
> > B pins? Well, the first answer would probably be 'have a serious
> > discussion with the HW designer responsible for this insanity' :-), but
> > once you've passed this 'WTF' stage, you'll have to find a way to tell
> > the source component it should use RGBxyx while the sink should use
> > BGRxyx (or vice-versa). This is something you can't extract that from
> > those width/shift props though. My suggestion would be to have one
> > string per MEDIA_BUS_FMT definition, so we can force things at the DT
> > level if we really have to. That's basically what the interface-pix-fmt
> > property was doing, except we would standardize the prop and values and
> > probably provide helpers so bridge elements don't have to parse this
> > prop manually.  
> 
> I don't think that would work in the general case though. We may want to
> use different formats and pick one of them at runtime based on external
> information (for instance when the sink can accept both RGB and YUV),
> hardcoding formats in DT isn't a good option. We instead need to add
> information to DT to specify how lines are connected, and deduce formats
> based on that.
> 

If we start describing the role of each pin, we're not that far from a
pinmux definition, the only difference being that we want pin configs
to match between the source and sink, where actual pinmux configs are
only controlled by one element (the HW block requesting exclusive
access to those pins).

Note that none of those things actually solve Marek's issue, which was
related to bus-flags, not bus-format. But I'm glad we have this
discussion, since that's something I need to solve for an imx setup
with a lvds-codec encoder connected to the imx-pd block.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Boris,

On Mon, Mar 09, 2020 at 09:42:44PM +0100, Boris Brezillon wrote:
> On Mon, 9 Mar 2020 22:32:11 +0200 Laurent Pinchart wrote:
> > On Mon, Mar 09, 2020 at 09:22:18PM +0100, Boris Brezillon wrote:
> >> On Mon, 9 Mar 2020 21:59:26 +0200 Laurent Pinchart wrote:  
> >>> On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:  
> >>>> On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:    
> >>>>> 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.    
> >>>> 
> >>>> I wonder if we shouldn't take more complex pin mappings into account
> >>>> now and go directly for a data-mapping property describing those
> >>>> mappings using a string. This way we'd have a single property that
> >>>> would work for both fully parallel buses (DPI/RGB) and serial (or
> >>>> partially parallel) ones (LVDS).    
> >>> 
> >>> I'm all for standardization, but I'm not sure data-mapping is the right
> >>> property, at least with its current definition. It's really meant to
> >>> describe how individual bits are mapped to the LVDS time slots. I'm fine
> >>> extending it, but we need to define it clearly. How would you envision
> >>> it being used in this case ?  
> >> 
> >> Well, clearly the data-width/data-shift approach does not solve all
> >> problems: what do you do if the source R pins are connected to the sink
> >> B pins? Well, the first answer would probably be 'have a serious
> >> discussion with the HW designer responsible for this insanity' :-), but
> >> once you've passed this 'WTF' stage, you'll have to find a way to tell
> >> the source component it should use RGBxyx while the sink should use
> >> BGRxyx (or vice-versa). This is something you can't extract that from
> >> those width/shift props though. My suggestion would be to have one
> >> string per MEDIA_BUS_FMT definition, so we can force things at the DT
> >> level if we really have to. That's basically what the interface-pix-fmt
> >> property was doing, except we would standardize the prop and values and
> >> probably provide helpers so bridge elements don't have to parse this
> >> prop manually.  
> > 
> > I don't think that would work in the general case though. We may want to
> > use different formats and pick one of them at runtime based on external
> > information (for instance when the sink can accept both RGB and YUV),
> > hardcoding formats in DT isn't a good option. We instead need to add
> > information to DT to specify how lines are connected, and deduce formats
> > based on that.
> 
> If we start describing the role of each pin, we're not that far from a
> pinmux definition, the only difference being that we want pin configs
> to match between the source and sink, where actual pinmux configs are
> only controlled by one element (the HW block requesting exclusive
> access to those pins).

The trick here will be to find an appropriate middle-ground. I don't
think we need to describe the role of each pin, but only to take into
account the parallel bus routing configurations that are likely to
happen in practice. Connecting MSBs to LSBs when decreasing the bus
width (or the other way around when increasing it) is a common issue.
Flipping R and B should be less common, but I suppose it can happen in
practice if the display controller supports both RGB and BGR formats (it
will just need to adjust the format internally if there's no dedicated
R<->B flipping hardware option). What else do we have ?

> Note that none of those things actually solve Marek's issue, which was
> related to bus-flags, not bus-format. But I'm glad we have this
> discussion, since that's something I need to solve for an imx setup
> with a lvds-codec encoder connected to the imx-pd block.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Boris Brezillon]

On Mon, 9 Mar 2020 22:48:05 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Mon, Mar 09, 2020 at 09:42:44PM +0100, Boris Brezillon wrote:
> > On Mon, 9 Mar 2020 22:32:11 +0200 Laurent Pinchart wrote:  
> > > On Mon, Mar 09, 2020 at 09:22:18PM +0100, Boris Brezillon wrote:  
> > >> On Mon, 9 Mar 2020 21:59:26 +0200 Laurent Pinchart wrote:    
> > >>> On Mon, Mar 09, 2020 at 08:55:59PM +0100, Boris Brezillon wrote:    
> > >>>> On Mon, 9 Mar 2020 21:23:06 +0200 Laurent Pinchart wrote:      
> > >>>>> 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.      
> > >>>> 
> > >>>> I wonder if we shouldn't take more complex pin mappings into account
> > >>>> now and go directly for a data-mapping property describing those
> > >>>> mappings using a string. This way we'd have a single property that
> > >>>> would work for both fully parallel buses (DPI/RGB) and serial (or
> > >>>> partially parallel) ones (LVDS).      
> > >>> 
> > >>> I'm all for standardization, but I'm not sure data-mapping is the right
> > >>> property, at least with its current definition. It's really meant to
> > >>> describe how individual bits are mapped to the LVDS time slots. I'm fine
> > >>> extending it, but we need to define it clearly. How would you envision
> > >>> it being used in this case ?    
> > >> 
> > >> Well, clearly the data-width/data-shift approach does not solve all
> > >> problems: what do you do if the source R pins are connected to the sink
> > >> B pins? Well, the first answer would probably be 'have a serious
> > >> discussion with the HW designer responsible for this insanity' :-), but
> > >> once you've passed this 'WTF' stage, you'll have to find a way to tell
> > >> the source component it should use RGBxyx while the sink should use
> > >> BGRxyx (or vice-versa). This is something you can't extract that from
> > >> those width/shift props though. My suggestion would be to have one
> > >> string per MEDIA_BUS_FMT definition, so we can force things at the DT
> > >> level if we really have to. That's basically what the interface-pix-fmt
> > >> property was doing, except we would standardize the prop and values and
> > >> probably provide helpers so bridge elements don't have to parse this
> > >> prop manually.    
> > > 
> > > I don't think that would work in the general case though. We may want to
> > > use different formats and pick one of them at runtime based on external
> > > information (for instance when the sink can accept both RGB and YUV),
> > > hardcoding formats in DT isn't a good option. We instead need to add
> > > information to DT to specify how lines are connected, and deduce formats
> > > based on that.  
> > 
> > If we start describing the role of each pin, we're not that far from a
> > pinmux definition, the only difference being that we want pin configs
> > to match between the source and sink, where actual pinmux configs are
> > only controlled by one element (the HW block requesting exclusive
> > access to those pins).  
> 
> The trick here will be to find an appropriate middle-ground. I don't
> think we need to describe the role of each pin, but only to take into
> account the parallel bus routing configurations that are likely to
> happen in practice. Connecting MSBs to LSBs when decreasing the bus
> width (or the other way around when increasing it) is a common issue.
> Flipping R and B should be less common, but I suppose it can happen in
> practice if the display controller supports both RGB and BGR formats (it
> will just need to adjust the format internally if there's no dedicated
> R<->B flipping hardware option). What else do we have ?

That's all I can think of, at least for DPI/RGB buses. So your
recommendation would be to keep data-width/data-shift props and
complement them with dpi-swap-red-blue-signals, right? Not entirely sure
data-width is enough to describe all kind of bus-width
increase/shrinking (don't we have RGB combinations taking the same
number of signals but with different layouts)?

Alternatively we could describe the 'out-src-format <-> in-sink-format'
mappings using pair of strings:

	port {
	...
		endpoint {
			bus-formats = <out-src-format1>,
				      <in-sink-format1>,
				      <out-src-format2>,
				      <in-sink-format2>,
					...;
		};
	};

but I'm almost sure you won't like this idea (actually, I don't like it
either :-)).

Anyway, I don't have much time to spend on this, so I'll probably
respin the lvds-codec patches, extending it to support bus-width on the
input side of an encoder element.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel