| Message ID | 20191114131751.26746-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/imx: parallel-display: Adjust bus_flags and bus_format handling | expand |
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.
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
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.
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).
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 ?
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.
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.
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.
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.
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.
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.
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.
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;
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(-)