Hi
Am Mittwoch, den 09.02.2022, 00:52 +0100 schrieb Marek Vasut:
On 2/8/22 22:27, Christoph Niedermaier wrote:
From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com] Sent: Thursday, February 3, 2022 12:46 AM
Hi Christoph,
Hi Laurent,
On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
Without the data-mapping devicetree property my display won't work properly. It is flickering, because the bus flags won't be assigned without a defined bus format by the imx parallel display driver. There was a discussion about the removal [1] and an agreement that a better solution is needed, but it is missing so far. So what would be the better approach?
[1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
Signed-off-by: Christoph Niedermaier cniedermaier@dh-electronics.com Cc: Marek Vasut marex@denx.de Cc: Sam Ravnborg sam@ravnborg.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maxime Ripard mripard@kernel.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Shawn Guo shawnguo@kernel.org Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Pengutronix Kernel Team kernel@pengutronix.de Cc: Fabio Estevam festevam@gmail.com Cc: NXP Linux Team linux-imx@nxp.com Cc: linux-arm-kernel@lists.infradead.org To: dri-devel@lists.freedesktop.org
drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 3c08f9827acf..2c683d94a3f3 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm;
const char *mapping; int ret; np = dev->of_node;@@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", &desc->size.width); of_property_read_u32(np, "height-mm", &desc->size.height);
of_property_read_string(np, "data-mapping", &mapping);if (!strcmp(mapping, "rgb24"))desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;else if (!strcmp(mapping, "rgb565"))desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;else if (!strcmp(mapping, "bgr666"))desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;else if (!strcmp(mapping, "lvds666"))desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;You're right that there's an issue, but a revert isn't the right option. The commit you're reverting never made it in a stable release, because it was deemed to not be a good enough option.
First of all, any attempt to fix this should include an update to the DT binding. Second, as this is about DPI panels, the LVDS option should be dropped. Finally, I've shared some initial thoughts in [1], maybe you can reply to that e-mail to continue the discussion there ?
According to your thoughts in [1] you mean that the bus format should be build out of the devicetree properties bus-width and data-shift. It would be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18, but what about a bus like RGB565_1X16, where each color has different bus width. Also the order of the colors should be defined to differ between busses like RGB888_1X24 and GBR888_1X24. Are there any ideas how can this be covered?
Maybe with props like these ?
channel-width -- width of each color channel channel-shift -- shift of each color channel channel-map -- mapping of each color channel
So for RGB888 channel-width = <8 8 8>; channel-shift = <0 0 0>; channel-map = "RGB"; // or something ?
For BGR565 panel connected to RGB24 scanout channel-width = <5 6 5>; channel-shift = <3 2 3>; channel-map = "BGR"; // or something ?
For BGR565 panel connected to RGB565 scanout channel-width = <5 6 5>; channel-shift = <0 0 0>; channel-map = "BGR"; // or something ?
To me this looks like it goes into the wrong direction. The goal is to set bus_format to one of the possible enum values. Using such a generic approach it would be possible to request bus formats for which no enum value is currently defined, i.e. the BGR565 example does not have a matching bus_format in media-bus-format.h.
What about keeping the solution which the revert would give. Optionally with a changed property name s/data-mapping/bus-format/ and a changed list of allowed strings to better match between the dt string and the enum name, i.e. s/"rgb24"/"RGB888_1X24"/. This would allow to extend DT binding and code to possible newly required values like e.g. MEDIA_BUS_FMT_BGR888_1X24.
Alternatively one could use the bus-width and data-shift properties and allow for (bus-width + data-shift) to be one of 16, 18, 24 and assume RGB. I.e. one could only specify MEDIA_BUS_FMT_RGB565_1X16, MEDIA_BUS_FMT_RGB666_1X18, MEDIA_BUS_FMT_RGB888_1X24 respectively.
Max
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel