[PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Ahmad Fatoum]

Hello,

I got a board with the RED[0:7]/BLUE[0:7] lanes originating from the
LCDIF swapped and would like to describe this in the device tree:

This first patch extends the mxsfb driver to support
following bus formats:
	MEDIA_BUS_FMT_BGR888_1X24
	MEDIA_BUS_FMT_RBG888_1X24
	MEDIA_BUS_FMT_GBR888_1X24

The latter two patches add a new interface-pix-fmt property
(named so because fsl,imx-parallel-display has one),
which allows a device tree to override the bus format to account
for swapped signal lanes.

Thoughts?

Cheers
Ahmad

--

Ahmad Fatoum (3):
  drm/mxsfb: use bus_format to determine pixel RGB component order
  drm/mxsfb: implement interface-pix-fmt of_property to override bus
    format
  dt-bindings: mxsfb: document new interface-pix-fmt property

 .../devicetree/bindings/display/mxsfb.txt     |  5 +++
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c            | 45 +++++++++++++++----
 drivers/gpu/drm/mxsfb/mxsfb_drv.c             | 13 ++++++
 drivers/gpu/drm/mxsfb/mxsfb_drv.h             |  7 +++
 drivers/gpu/drm/mxsfb/mxsfb_regs.h            | 17 +++++++
 5 files changed, 79 insertions(+), 8 deletions(-)

-- 
2.19.1

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

[PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order

[Ahmad Fatoum]

The LCDIF controller v4, like the one included in the i.MX6UL,
can be configured to accept other RGB pixel formats. Extend
the driver to configure the controller to do so.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 41 +++++++++++++++++++++++++-----
 drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  6 +++++
 drivers/gpu/drm/mxsfb/mxsfb_regs.h | 17 +++++++++++++
 3 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 24b1f0c1432e..1b5b1fddd691 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -97,29 +97,56 @@ static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb)
 	struct drm_crtc *crtc = &mxsfb->pipe.crtc;
 	struct drm_device *drm = crtc->dev;
 	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-	u32 reg;
+	u32 ctrl, pattern;
 
-	reg = readl(mxsfb->base + LCDC_CTRL);
+	ctrl = readl(mxsfb->base + LCDC_CTRL);
 
 	if (mxsfb->connector.display_info.num_bus_formats)
 		bus_format = mxsfb->connector.display_info.bus_formats[0];
 
-	reg &= ~CTRL_BUS_WIDTH_MASK;
+	ctrl &= ~CTRL_BUS_WIDTH_MASK;
 	switch (bus_format) {
 	case MEDIA_BUS_FMT_RGB565_1X16:
-		reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
+		pattern = CTRL2_PATTERN_RGB;
 		break;
 	case MEDIA_BUS_FMT_RGB666_1X18:
-		reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
+		pattern = CTRL2_PATTERN_RGB;
 		break;
 	case MEDIA_BUS_FMT_RGB888_1X24:
-		reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+		pattern = CTRL2_PATTERN_RGB;
+		break;
+	case MEDIA_BUS_FMT_BGR888_1X24:
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+		pattern = CTRL2_PATTERN_BGR;
+		break;
+	case MEDIA_BUS_FMT_RBG888_1X24:
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+		pattern = CTRL2_PATTERN_RBG;
+		break;
+	case MEDIA_BUS_FMT_GBR888_1X24:
+		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+		pattern = CTRL2_PATTERN_GBR;
 		break;
 	default:
+		pattern = CTRL2_PATTERN_RGB;
 		dev_err(drm->dev, "Unknown media bus format %d\n", bus_format);
 		break;
 	}
-	writel(reg, mxsfb->base + LCDC_CTRL);
+	writel(ctrl, mxsfb->base + LCDC_CTRL);
+
+	if (mxsfb_is_v4(mxsfb)) {
+		u32 ctrl2 = readl(mxsfb->base + LCDC_V4_CTRL2);
+		ctrl2 &= ~CTRL2_PATTERN_MASK;
+		ctrl2 |= CTRL2_SET_PATTERN(pattern);
+		writel(ctrl2, mxsfb->base + LCDC_V4_CTRL2);
+	} else if (pattern != CTRL2_PATTERN_RGB) {
+		/* RGB is default, so only warn for other patterns */
+		dev_err(drm->dev, "Unsupported media bus format %d\n",
+			bus_format);
+	}
 }
 
 static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index 5d0883fc805b..89fa2076acaf 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -16,6 +16,8 @@
 #ifndef __MXSFB_DRV_H__
 #define __MXSFB_DRV_H__
 
+#include <linux/types.h>
+
 struct mxsfb_devdata {
 	unsigned int	 transfer_count;
 	unsigned int	 cur_buf;
@@ -50,5 +52,9 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private *mxsfb);
 void mxsfb_crtc_disable(struct mxsfb_drm_private *mxsfb);
 void mxsfb_plane_atomic_update(struct mxsfb_drm_private *mxsfb,
 			       struct drm_plane_state *state);
+static inline bool mxsfb_is_v4(const struct mxsfb_drm_private *mxsfb)
+{
+	return mxsfb->devdata->ipversion == 4;
+}
 
 #endif /* __MXSFB_DRV_H__ */
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
index 66a6ba9ec533..c5f4dea80093 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
@@ -22,6 +22,7 @@
 
 #define LCDC_CTRL			0x00
 #define LCDC_CTRL1			0x10
+#define LCDC_V4_CTRL2			0x20
 #define LCDC_V3_TRANSFER_COUNT		0x20
 #define LCDC_V4_TRANSFER_COUNT		0x30
 #define LCDC_V4_CUR_BUF			0x40
@@ -59,6 +60,15 @@
 #define CTRL1_CUR_FRAME_DONE_IRQ_EN	(1 << 13)
 #define CTRL1_CUR_FRAME_DONE_IRQ	(1 << 9)
 
+#define CTRL2_EVEN_LINE_PATTERN_MASK	(0x7 << 12)
+#define CTRL2_ODD_LINE_PATTERN_MASK	(0x7 << 16)
+#define CTRL2_PATTERN_MASK		(CTRL2_ODD_LINE_PATTERN_MASK |\
+					 CTRL2_EVEN_LINE_PATTERN_MASK)
+#define CTRL2_SET_EVEN_LINE_PATTERN(x)	(((x) & 0x7) << 12)
+#define CTRL2_SET_ODD_LINE_PATTERN(x)	(((x) & 0x7) << 16)
+#define CTRL2_SET_PATTERN(x)		(CTRL2_SET_EVEN_LINE_PATTERN(x) |\
+					 CTRL2_SET_ODD_LINE_PATTERN(x))
+
 #define TRANSFER_COUNT_SET_VCOUNT(x)	(((x) & 0xffff) << 16)
 #define TRANSFER_COUNT_GET_VCOUNT(x)	(((x) >> 16) & 0xffff)
 #define TRANSFER_COUNT_SET_HCOUNT(x)	((x) & 0xffff)
@@ -112,4 +122,11 @@
 #define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
 #define MXSFB_SYNC_DOTCLK_FALLING_ACT	(1 << 7) /* negative edge sampling */
 
+#define CTRL2_PATTERN_RGB		0x0
+#define CTRL2_PATTERN_RBG		0x1
+#define CTRL2_PATTERN_GBR		0x2
+#define CTRL2_PATTERN_GRB		0x3
+#define CTRL2_PATTERN_BRG		0x4
+#define CTRL2_PATTERN_BGR		0x5
+
 #endif /* __MXSFB_REGS_H__ */
-- 
2.19.1

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

[PATCH 2/3] drm/mxsfb: implement interface-pix-fmt of_property to override bus format

[Ahmad Fatoum]

This way hardware that has the LCD signal lines swapped can express
so in the device tree and existing panel support can be reused
unmodified.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c |  4 +++-
 drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 13 +++++++++++++
 drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index 1b5b1fddd691..0de48384054d 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -101,7 +101,9 @@ static void mxsfb_set_bus_fmt(struct mxsfb_drm_private *mxsfb)
 
 	ctrl = readl(mxsfb->base + LCDC_CTRL);
 
-	if (mxsfb->connector.display_info.num_bus_formats)
+	if (mxsfb->bus_format_override)
+		bus_format = mxsfb->bus_format_override;
+	else if (mxsfb->connector.display_info.num_bus_formats)
 		bus_format = mxsfb->connector.display_info.bus_formats[0];
 
 	ctrl &= ~CTRL_BUS_WIDTH_MASK;
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 2393e6d16ffd..169b458691e4 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -182,6 +182,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct mxsfb_drm_private *mxsfb;
 	struct resource *res;
+	const char *fmt;
 	int ret;
 
 	mxsfb = devm_kzalloc(&pdev->dev, sizeof(*mxsfb), GFP_KERNEL);
@@ -208,6 +209,18 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags)
 	if (IS_ERR(mxsfb->clk_disp_axi))
 		mxsfb->clk_disp_axi = NULL;
 
+	ret = of_property_read_string(drm->dev->of_node, "interface-pix-fmt", &fmt);
+	if (!ret) {
+		if (!strcmp(fmt, "rgb24"))
+			mxsfb->bus_format_override = MEDIA_BUS_FMT_RGB888_1X24;
+		else if (!strcmp(fmt, "bgr24"))
+			mxsfb->bus_format_override = MEDIA_BUS_FMT_BGR888_1X24;
+		else if (!strcmp(fmt, "rbg24"))
+			mxsfb->bus_format_override = MEDIA_BUS_FMT_RBG888_1X24;
+		else if (!strcmp(fmt, "gbr24"))
+			mxsfb->bus_format_override = MEDIA_BUS_FMT_GBR888_1X24;
+	}
+
 	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
index 89fa2076acaf..63d30dd4dc36 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
@@ -40,6 +40,7 @@ struct mxsfb_drm_private {
 	struct drm_connector		connector;
 	struct drm_panel		*panel;
 	struct drm_fbdev_cma		*fbdev;
+	unsigned int			bus_format_override;
 };
 
 int mxsfb_setup_crtc(struct drm_device *dev);
-- 
2.19.1

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

[PATCH 3/3] dt-bindings: mxsfb: document new interface-pix-fmt property

[Ahmad Fatoum]

This newly implemented binding allows expressing swapped signal
lanes in the device tree so existing panel support can be reused.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/devicetree/bindings/display/mxsfb.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/mxsfb.txt b/Documentation/devicetree/bindings/display/mxsfb.txt
index 472e1ea6c591..aa0c2c32ee1e 100644
--- a/Documentation/devicetree/bindings/display/mxsfb.txt
+++ b/Documentation/devicetree/bindings/display/mxsfb.txt
@@ -14,6 +14,11 @@ Required properties:
     - "pix" for the LCDIF block clock
     - (MX6SX-only) "axi", "disp_axi" for the bus interface clock
 
+optional properties:
+- interface-pix-fmt:	overrides the bus format in use for the display
+			interface. This is to account for swapped signal lanes.
+			supported types: "rgb24", "bgr24", "rbg24", "gbr24"
+
 Required sub-nodes:
   - port: The connection to an encoder chip.
 
-- 
2.19.1

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

Re: [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order

[Ahmad Fatoum]

On 2/1/19 18:02, Ahmad Fatoum wrote:

> -	reg &= ~CTRL_BUS_WIDTH_MASK;
> +	ctrl &= ~CTRL_BUS_WIDTH_MASK;
>  	switch (bus_format) {

>  	default:
> +		pattern = CTRL2_PATTERN_RGB;
>  		dev_err(drm->dev, "Unknown media bus format %d\n", bus_format);
>  		break;
>  	}
> -	writel(reg, mxsfb->base + LCDC_CTRL);
> +	writel(ctrl, mxsfb->base + LCDC_CTRL);

Should the default case explicitly set a value for ctrl?
So far, the appropriate bits stay at zero after the &= ~CTRL_BUS_WIDTH_MASK
which is STMLCDIF_16BIT.

Or maybe skip the LCDC_CTRL register write altogether if dev_err is reached?

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Stefan Agner]

On 02.01.2019 18:02, Ahmad Fatoum wrote:
> Hello,
> 
> I got a board with the RED[0:7]/BLUE[0:7] lanes originating from the
> LCDIF swapped and would like to describe this in the device tree:
> 
> This first patch extends the mxsfb driver to support
> following bus formats:
> 	MEDIA_BUS_FMT_BGR888_1X24
> 	MEDIA_BUS_FMT_RBG888_1X24
> 	MEDIA_BUS_FMT_GBR888_1X24
> 
> The latter two patches add a new interface-pix-fmt property
> (named so because fsl,imx-parallel-display has one),
> which allows a device tree to override the bus format to account
> for swapped signal lanes.
> 
> Thoughts?

On a quick glance patch 1 looks good.

However, patch 2/3 are probably unnecessary when using of graph/panel
support. E.g. panel-simple.c supports bus formats.

Is the display you are using regular RGB and only the board/connectors
happen to swap colors?

--
Stefan

> 
> Cheers
> Ahmad
> 
> --
> 
> Ahmad Fatoum (3):
>   drm/mxsfb: use bus_format to determine pixel RGB component order
>   drm/mxsfb: implement interface-pix-fmt of_property to override bus
>     format
>   dt-bindings: mxsfb: document new interface-pix-fmt property
> 
>  .../devicetree/bindings/display/mxsfb.txt     |  5 +++
>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c            | 45 +++++++++++++++----
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c             | 13 ++++++
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h             |  7 +++
>  drivers/gpu/drm/mxsfb/mxsfb_regs.h            | 17 +++++++
>  5 files changed, 79 insertions(+), 8 deletions(-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Sam Ravnborg]

Hi Ahmad.

On Wed, Jan 02, 2019 at 10:05:31PM +0100, Stefan Agner wrote:
> On 02.01.2019 18:02, Ahmad Fatoum wrote:
> > Hello,
> > 
> > I got a board with the RED[0:7]/BLUE[0:7] lanes originating from the
> > LCDIF swapped and would like to describe this in the device tree:
> > 
> > This first patch extends the mxsfb driver to support
> > following bus formats:
> > 	MEDIA_BUS_FMT_BGR888_1X24
> > 	MEDIA_BUS_FMT_RBG888_1X24
> > 	MEDIA_BUS_FMT_GBR888_1X24
> > 
> > The latter two patches add a new interface-pix-fmt property
> > (named so because fsl,imx-parallel-display has one),
> > which allows a device tree to override the bus format to account
> > for swapped signal lanes.
> > 
> > Thoughts?
I have not seen the original mail, so a reply to this mail.
The problem with the RED/BLUE lines swapped is something I
have encountered while working with DRM support for Atmel at91sam9263 too.

The solution selected is to extend the endpoint with
a new optional property:

- wiring: Wiring of data lines to display.
  "straight" - normal wiring.
  "red-blue-reversed" - red and blue lines reversed.

(media/video-interfaces.txt)


The DT node looks like this:

               port@0 {
                        reg = <0>;
                        #address-cells = <1>;
                        #size-cells = <0>;
                        lcdc_panel_output: endpoint@0 {
                                reg = <0>;
                                wiring = "red-blue-reversed";
                                remote-endpoint = <&panel_input>;
                        };
                };

This allows us to specify the swapping in the endpoint and
not in the panel.
So we can use the same panel, with the same bus_format, in several
designs some with red-blue swapped (reversed), and some not.

This above is inspired by some earlier thread on dri-devel.
I recall Peter Rosin was one of the main source of inspiration.

Patches I refer to are not yet posted, this is work-in-progess.
They need more polishing and testing before they are dri-devel ready.

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

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Ahmad Fatoum]

On 1/2/19 10:05 PM, Stefan Agner wrote:

> On a quick glance patch 1 looks good.
> 
> However, patch 2/3 are probably unnecessary when using of graph/panel
> support. E.g. panel-simple.c supports bus formats.
> 
> Is the display you are using regular RGB and only the board/connectors
> happen to swap colors?

Exactly, panel has RGB bus format, but red and blue are crossed on PCB.

Cheers
Ahmad
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Ahmad Fatoum]

Hello Sam,

On 2/1/19 22:37, Sam Ravnborg wrote:
> The problem with the RED/BLUE lines swapped is something I
> have encountered while working with DRM support for Atmel at91sam9263 too.
> 
> The solution selected is to extend the endpoint with
> a new optional property:
> 
> - wiring: Wiring of data lines to display.
>   "straight" - normal wiring.
>   "red-blue-reversed" - red and blue lines reversed.
> 
> (media/video-interfaces.txt)
> 
> 
> The DT node looks like this:
> 
>                port@0 {
>                         reg = <0>;
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         lcdc_panel_output: endpoint@0 {
>                                 reg = <0>;
>                                 wiring = "red-blue-reversed";
>                                 remote-endpoint = <&panel_input>;
>                         };
>                 };
> 
> This allows us to specify the swapping in the endpoint and
> not in the panel.
> So we can use the same panel, with the same bus_format, in several
> designs some with red-blue swapped (reversed), and some not.

A colleague suggested a property in the endpoint as well, but I shied
away because of the extra hassle. Seems there's won't be a way around it ^^'..

How do you intend to propagate this different wiring setting?
How about having drm_of_find_panel_or_bridge adjust the
(*panel)->connector->display_info.bus_formats array to account for the
different wiring? That way there shouldn't be any need to adjust drivers.

> 
> This above is inspired by some earlier thread on dri-devel.
> I recall Peter Rosin was one of the main source of inspiration.

Thanks for the interesting read.

> 
> Patches I refer to are not yet posted, this is work-in-progess.
> They need more polishing and testing before they are dri-devel ready

Cheers
Ahmad

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Sam Ravnborg]

Hi Ahmad.

> On 2/1/19 22:37, Sam Ravnborg wrote:
> > The problem with the RED/BLUE lines swapped is something I
> > have encountered while working with DRM support for Atmel at91sam9263 too.
> > 
> > The solution selected is to extend the endpoint with
> > a new optional property:
> > 
> > - wiring: Wiring of data lines to display.
> >   "straight" - normal wiring.
> >   "red-blue-reversed" - red and blue lines reversed.
> > 
> > (media/video-interfaces.txt)
> > 
> > 
> > The DT node looks like this:
> > 
> >                port@0 {
> >                         reg = <0>;
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         lcdc_panel_output: endpoint@0 {
> >                                 reg = <0>;
> >                                 wiring = "red-blue-reversed";
> >                                 remote-endpoint = <&panel_input>;
> >                         };
> >                 };
> > 
> > This allows us to specify the swapping in the endpoint and
> > not in the panel.
> > So we can use the same panel, with the same bus_format, in several
> > designs some with red-blue swapped (reversed), and some not.
> 
> A colleague suggested a property in the endpoint as well, but I shied
> away because of the extra hassle. Seems there's won't be a way around it ^^'..
> 
> How do you intend to propagate this different wiring setting?

The way I have it implmented is more or less like this:

First find the wiring property:
1) Look up endpoint using of_graph_get_endpoint_by_regs()
2) Get wiring property
3) of_node_put(endpoint);

And then find and attach the panel:
4) drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
5) devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_DPI);
6) Then based on the wiring property I adjust bus_format
7) drm_simple_display_pipe_init()
8) drm_simple_display_pipe_attach_bridge()

But this is all virgin code that for now can build,
but has not yet seen any testing.
It is a lot of boilerplate for something relatively simple
and I hope there are ways to simplify this.
Relevant parts of the file pasted below.

But the translation of bus_format in a central place may prove a bit
difficult and I assume this as something that can differ
a lot between different HW solutions.

> How about having drm_of_find_panel_or_bridge adjust the
> (*panel)->connector->display_info.bus_formats array to account for the
> different wiring? That way there shouldn't be any need to adjust drivers.
But if you prove me wrong and this fly I am all for it.

Keep in mind that I am novice in the DRM land. So there may be better ways to do it.

	Sam


static int lcdc_get_of_wiring(struct lcdc *lcdc,
			      const struct device_node *ep)
{
	const char *str;
	int ret;

	ret = of_property_read_string(ep, "wiring", &str);
	if (ret)
		return ret;

	if (strcmp(str, "red-green-reversed") == 0) {
		lcdc->wiring_reversed = true;
	} else if (strcmp(str, "straight") == 0) {
		/* Use default format */
	} else {
		DRM_DEV_ERROR(lcdc->dev, "unknown \"wiring\" property: %s",
			      str);
		return -EINVAL;
	}

	return 0;
}

static int lcdc_display_init(struct lcdc *lcdc, struct drm_device *drm)
{
	struct drm_display_info *display_info;
	const u32 *formats;
	size_t nformats;
	int ret;

	display_info = &lcdc->panel->connector->display_info;

	if (!display_info->num_bus_formats || !display_info->bus_formats) {
		DRM_DEV_ERROR(lcdc->dev, "missing bus_format from panel");
		return -EINVAL;
	}

	switch (display_info->bus_formats[0]) {
		case MEDIA_BUS_FMT_RGB888_1X24:
		case MEDIA_BUS_FMT_RGB565_1X16:
			lcdc->bus_format = display_info->bus_formats[0];
			break;
		default:
			DRM_DEV_ERROR(lcdc->dev, "unsupported bus_format: %d",
				      display_info->bus_formats[0]);
			return -EINVAL;
	}

	/* Select formats depending on wiring (from bus_formats + from DT) */
	if (lcdc->bus_format == MEDIA_BUS_FMT_RGB888_1X24) {
		if (!lcdc->wiring_reversed) {
			formats = bgr_formats_24;
			nformats = ARRAY_SIZE(bgr_formats_24);
		} else {
			formats = rgb_formats_24;
			nformats = ARRAY_SIZE(rgb_formats_24);
		}
	} else {
		if (!lcdc->wiring_reversed) {
			formats = bgr_formats_16;
			nformats = ARRAY_SIZE(bgr_formats_16);
		} else {
			formats = rgb_formats_16;
			nformats = ARRAY_SIZE(rgb_formats_16);
		}
	}

	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
					   &lcdc_display_funcs,
					   formats, nformats,
					   NULL, &lcdc->connector);
	if (ret < 0)
		DRM_DEV_ERROR(lcdc->dev, "failed to init display pipe: %d",
			      ret);

	return ret;
}

static int lcdc_attach_panel(struct lcdc *lcdc, struct drm_device *drm)
{
	struct drm_bridge *bridge;
	struct drm_panel *panel;
	struct device *dev;
	int ret;

	dev = lcdc->dev;

	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
	if (ret < 0)
		return ret;

	if (panel) {
		lcdc->panel = panel;
		bridge = devm_drm_panel_bridge_add(dev, panel,
						   DRM_MODE_CONNECTOR_DPI);
		ret = PTR_ERR_OR_ZERO(bridge);
		if (ret < 0) {
			DRM_DEV_ERROR(dev, "failed to add bridge: %d", ret);
			return ret;
		}

	} else {
		DRM_DEV_ERROR(dev, "the bridge is not a panel");
		return -ENODEV;
	}

	ret = lcdc_display_init(lcdc, drm);
	if (ret < 0)
		return ret;

	ret = drm_simple_display_pipe_attach_bridge(&lcdc->pipe, bridge);
	if (ret < 0)
		DRM_DEV_ERROR(dev, "failed to attach bridge: %d", ret);

	return ret;
}

static int lcdc_create_output(struct lcdc *lcdc, struct drm_device *drm)
{
	struct device_node *endpoint;
	int ret;

	/* port@0/endpoint@0 is the only port/endpoint */
	endpoint = of_graph_get_endpoint_by_regs(drm->dev->of_node, 0, 0);
	if (!endpoint) {
		DRM_DEV_ERROR(lcdc->dev, "failed to find endpoint node");
		return -ENODEV;
	}

	lcdc_get_of_wiring(lcdc, endpoint);
	of_node_put(endpoint);

	ret = lcdc_attach_panel(lcdc, drm);

	return ret;
}


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

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Ahmad Fatoum]

Hello Sam,

On 7/1/19 19:04, Sam Ravnborg wrote:
> Hi Ahmad.
> 
>> On 2/1/19 22:37, Sam Ravnborg wrote:
>>> The problem with the RED/BLUE lines swapped is something I
>>> have encountered while working with DRM support for Atmel at91sam9263 too.
>>>
>>> The solution selected is to extend the endpoint with
>>> a new optional property:
>>>
>>> - wiring: Wiring of data lines to display.
>>>   "straight" - normal wiring.
>>>   "red-blue-reversed" - red and blue lines reversed.
>>>
>>> (media/video-interfaces.txt)
>>>
>>>
>>> The DT node looks like this:
>>>
>>>                port@0 {
>>>                         reg = <0>;
>>>                         #address-cells = <1>;
>>>                         #size-cells = <0>;
>>>                         lcdc_panel_output: endpoint@0 {
>>>                                 reg = <0>;
>>>                                 wiring = "red-blue-reversed";
>>>                                 remote-endpoint = <&panel_input>;
>>>                         };
>>>                 };
>>>
>>> This allows us to specify the swapping in the endpoint and
>>> not in the panel.
>>> So we can use the same panel, with the same bus_format, in several
>>> designs some with red-blue swapped (reversed), and some not.
>>
>> A colleague suggested a property in the endpoint as well, but I shied
>> away because of the extra hassle. Seems there's won't be a way around it ^^'..
>>
>> How do you intend to propagate this different wiring setting?
> 
> The way I have it implmented is more or less like this:
> 
> First find the wiring property:
> 1) Look up endpoint using of_graph_get_endpoint_by_regs()
> 2) Get wiring property
> 3) of_node_put(endpoint);
> 
> And then find and attach the panel:
> 4) drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
> 5) devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_DPI);
> 6) Then based on the wiring property I adjust bus_format
> 7) drm_simple_display_pipe_init()
> 8) drm_simple_display_pipe_attach_bridge()
> 
> But this is all virgin code that for now can build,
> but has not yet seen any testing.
> It is a lot of boilerplate for something relatively simple
> and I hope there are ways to simplify this.
> Relevant parts of the file pasted below.
> 
> But the translation of bus_format in a central place may prove a bit
> difficult and I assume this as something that can differ
> a lot between different HW solutions.
> 
>> How about having drm_of_find_panel_or_bridge adjust the
>> (*panel)->connector->display_info.bus_formats array to account for the
>> different wiring? That way there shouldn't be any need to adjust drivers.
> But if you prove me wrong and this fly I am all for it.
> 
> Keep in mind that I am novice in the DRM land. So there may be better ways to do it.

Same here. I looked a bit into how this could be done generically, and doing it
inside mxsfb, like you suggested, seems quite easier, albeit still too much effort
as the next hardware revision should unswap the lines again.

I'll ask for the latter two patches to be dropped.

Thanks again.

> 
> 	Sam
> 
> 
> static int lcdc_get_of_wiring(struct lcdc *lcdc,
> 			      const struct device_node *ep)
> {
> 	const char *str;
> 	int ret;
> 
> 	ret = of_property_read_string(ep, "wiring", &str);
> 	if (ret)
> 		return ret;
> 
> 	if (strcmp(str, "red-green-reversed") == 0) {
> 		lcdc->wiring_reversed = true;
> 	} else if (strcmp(str, "straight") == 0) {
> 		/* Use default format */
> 	} else {
> 		DRM_DEV_ERROR(lcdc->dev, "unknown \"wiring\" property: %s",
> 			      str);
> 		return -EINVAL;
> 	}
> 
> 	return 0;
> }
> 
> static int lcdc_display_init(struct lcdc *lcdc, struct drm_device *drm)
> {
> 	struct drm_display_info *display_info;
> 	const u32 *formats;
> 	size_t nformats;
> 	int ret;
> 
> 	display_info = &lcdc->panel->connector->display_info;
> 
> 	if (!display_info->num_bus_formats || !display_info->bus_formats) {
> 		DRM_DEV_ERROR(lcdc->dev, "missing bus_format from panel");
> 		return -EINVAL;
> 	}
> 
> 	switch (display_info->bus_formats[0]) {
> 		case MEDIA_BUS_FMT_RGB888_1X24:
> 		case MEDIA_BUS_FMT_RGB565_1X16:
> 			lcdc->bus_format = display_info->bus_formats[0];
> 			break;
> 		default:
> 			DRM_DEV_ERROR(lcdc->dev, "unsupported bus_format: %d",
> 				      display_info->bus_formats[0]);
> 			return -EINVAL;
> 	}
> 
> 	/* Select formats depending on wiring (from bus_formats + from DT) */
> 	if (lcdc->bus_format == MEDIA_BUS_FMT_RGB888_1X24) {
> 		if (!lcdc->wiring_reversed) {
> 			formats = bgr_formats_24;
> 			nformats = ARRAY_SIZE(bgr_formats_24);
> 		} else {
> 			formats = rgb_formats_24;
> 			nformats = ARRAY_SIZE(rgb_formats_24);
> 		}
> 	} else {
> 		if (!lcdc->wiring_reversed) {
> 			formats = bgr_formats_16;
> 			nformats = ARRAY_SIZE(bgr_formats_16);
> 		} else {
> 			formats = rgb_formats_16;
> 			nformats = ARRAY_SIZE(rgb_formats_16);
> 		}
> 	}
> 
> 	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
> 					   &lcdc_display_funcs,
> 					   formats, nformats,
> 					   NULL, &lcdc->connector);
> 	if (ret < 0)
> 		DRM_DEV_ERROR(lcdc->dev, "failed to init display pipe: %d",
> 			      ret);
> 
> 	return ret;
> }
> 
> static int lcdc_attach_panel(struct lcdc *lcdc, struct drm_device *drm)
> {
> 	struct drm_bridge *bridge;
> 	struct drm_panel *panel;
> 	struct device *dev;
> 	int ret;
> 
> 	dev = lcdc->dev;
> 
> 	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
> 	if (ret < 0)
> 		return ret;
> 
> 	if (panel) {
> 		lcdc->panel = panel;
> 		bridge = devm_drm_panel_bridge_add(dev, panel,
> 						   DRM_MODE_CONNECTOR_DPI);
> 		ret = PTR_ERR_OR_ZERO(bridge);
> 		if (ret < 0) {
> 			DRM_DEV_ERROR(dev, "failed to add bridge: %d", ret);
> 			return ret;
> 		}
> 
> 	} else {
> 		DRM_DEV_ERROR(dev, "the bridge is not a panel");
> 		return -ENODEV;
> 	}
> 
> 	ret = lcdc_display_init(lcdc, drm);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = drm_simple_display_pipe_attach_bridge(&lcdc->pipe, bridge);
> 	if (ret < 0)
> 		DRM_DEV_ERROR(dev, "failed to attach bridge: %d", ret);
> 
> 	return ret;
> }
> 
> static int lcdc_create_output(struct lcdc *lcdc, struct drm_device *drm)
> {
> 	struct device_node *endpoint;
> 	int ret;
> 
> 	/* port@0/endpoint@0 is the only port/endpoint */
> 	endpoint = of_graph_get_endpoint_by_regs(drm->dev->of_node, 0, 0);
> 	if (!endpoint) {
> 		DRM_DEV_ERROR(lcdc->dev, "failed to find endpoint node");
> 		return -ENODEV;
> 	}
> 
> 	lcdc_get_of_wiring(lcdc, endpoint);
> 	of_node_put(endpoint);
> 
> 	ret = lcdc_attach_panel(lcdc, drm);
> 
> 	return ret;
> }
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes

[Ahmad Fatoum]

Hello,

On 2/1/19 22:05, Stefan Agner wrote:
> On 02.01.2019 18:02, Ahmad Fatoum wrote:
>> Hello,
>>
>> I got a board with the RED[0:7]/BLUE[0:7] lanes originating from the
>> LCDIF swapped and would like to describe this in the device tree:
>>
>> This first patch extends the mxsfb driver to support
>> following bus formats:
>> 	MEDIA_BUS_FMT_BGR888_1X24
>> 	MEDIA_BUS_FMT_RBG888_1X24
>> 	MEDIA_BUS_FMT_GBR888_1X24
>>
>> The latter two patches add a new interface-pix-fmt property
>> (named so because fsl,imx-parallel-display has one),
>> which allows a device tree to override the bus format to account
>> for swapped signal lanes.
>>
>> Thoughts?
> 
> On a quick glance patch 1 looks good.

Did you have time to look at this again?
Only the first patch is relevant, please ignore the other two.

> 
> However, patch 2/3 are probably unnecessary when using of graph/panel
> support. E.g. panel-simple.c supports bus formats.

I decided against trying to make this configurable over device tree.
I'll make use of this by specifying the adjusted bus format in
a new panel-simple.c entry.

> 
> Is the display you are using regular RGB and only the board/connectors
> happen to swap colors?
> 
> --
> Stefan
> 
>>
>> Cheers
>> Ahmad
>>
>> --
>>
>> Ahmad Fatoum (3):
>>   drm/mxsfb: use bus_format to determine pixel RGB component order
>>   drm/mxsfb: implement interface-pix-fmt of_property to override bus
>>     format
>>   dt-bindings: mxsfb: document new interface-pix-fmt property
>>
>>  .../devicetree/bindings/display/mxsfb.txt     |  5 +++
>>  drivers/gpu/drm/mxsfb/mxsfb_crtc.c            | 45 +++++++++++++++----
>>  drivers/gpu/drm/mxsfb/mxsfb_drv.c             | 13 ++++++
>>  drivers/gpu/drm/mxsfb/mxsfb_drv.h             |  7 +++
>>  drivers/gpu/drm/mxsfb/mxsfb_regs.h            | 17 +++++++
>>  5 files changed, 79 insertions(+), 8 deletions(-)
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel