* [PATCH 0/3] drm/mxsfb: support swapped RGB lanes
@ 2019-01-02 17:02 Ahmad Fatoum
2019-01-02 17:02 ` [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order Ahmad Fatoum
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Ahmad Fatoum @ 2019-01-02 17:02 UTC (permalink / raw)
To: Stefan Agner, Marek Vasut, David Airlie, Daniel Vetter; +Cc: kernel, dri-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order 2019-01-02 17:02 [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Ahmad Fatoum @ 2019-01-02 17:02 ` Ahmad Fatoum 2019-01-02 17:14 ` Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 2/3] drm/mxsfb: implement interface-pix-fmt of_property to override bus format Ahmad Fatoum ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-02 17:02 UTC (permalink / raw) To: Stefan Agner, Marek Vasut, David Airlie, Daniel Vetter; +Cc: kernel, dri-devel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order 2019-01-02 17:02 ` [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order Ahmad Fatoum @ 2019-01-02 17:14 ` Ahmad Fatoum 0 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-02 17:14 UTC (permalink / raw) To: Stefan Agner, Marek Vasut, David Airlie, Daniel Vetter; +Cc: dri-devel, kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm/mxsfb: implement interface-pix-fmt of_property to override bus format 2019-01-02 17:02 [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order Ahmad Fatoum @ 2019-01-02 17:02 ` Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 3/3] dt-bindings: mxsfb: document new interface-pix-fmt property Ahmad Fatoum 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner 3 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-02 17:02 UTC (permalink / raw) To: Stefan Agner, Marek Vasut, David Airlie, Daniel Vetter; +Cc: kernel, dri-devel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] dt-bindings: mxsfb: document new interface-pix-fmt property 2019-01-02 17:02 [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 2/3] drm/mxsfb: implement interface-pix-fmt of_property to override bus format Ahmad Fatoum @ 2019-01-02 17:02 ` Ahmad Fatoum 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner 3 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-02 17:02 UTC (permalink / raw) To: Stefan Agner, Marek Vasut, David Airlie, Daniel Vetter; +Cc: kernel, dri-devel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-02 17:02 [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Ahmad Fatoum ` (2 preceding siblings ...) 2019-01-02 17:02 ` [PATCH 3/3] dt-bindings: mxsfb: document new interface-pix-fmt property Ahmad Fatoum @ 2019-01-02 21:05 ` Stefan Agner 2019-01-02 21:37 ` Sam Ravnborg ` (2 more replies) 3 siblings, 3 replies; 12+ messages in thread From: Stefan Agner @ 2019-01-02 21:05 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: Marek Vasut, David Airlie, kernel, dri-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner @ 2019-01-02 21:37 ` Sam Ravnborg 2019-01-07 17:35 ` Ahmad Fatoum 2019-01-03 9:59 ` Ahmad Fatoum 2019-03-27 14:30 ` Ahmad Fatoum 2 siblings, 1 reply; 12+ messages in thread From: Sam Ravnborg @ 2019-01-02 21:37 UTC (permalink / raw) To: Stefan Agner, Peter Rosin Cc: Marek Vasut, David Airlie, Ahmad Fatoum, dri-devel, kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-02 21:37 ` Sam Ravnborg @ 2019-01-07 17:35 ` Ahmad Fatoum 2019-01-07 18:04 ` Sam Ravnborg 0 siblings, 1 reply; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-07 17:35 UTC (permalink / raw) To: Sam Ravnborg, Stefan Agner, Peter Rosin Cc: Marek Vasut, David Airlie, dri-devel, kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-07 17:35 ` Ahmad Fatoum @ 2019-01-07 18:04 ` Sam Ravnborg 2019-03-27 14:26 ` Ahmad Fatoum 0 siblings, 1 reply; 12+ messages in thread From: Sam Ravnborg @ 2019-01-07 18:04 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: Marek Vasut, David Airlie, dri-devel, kernel, Peter Rosin 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-07 18:04 ` Sam Ravnborg @ 2019-03-27 14:26 ` Ahmad Fatoum 0 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-03-27 14:26 UTC (permalink / raw) To: Sam Ravnborg; +Cc: Marek Vasut, David Airlie, dri-devel, kernel, Peter Rosin 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner 2019-01-02 21:37 ` Sam Ravnborg @ 2019-01-03 9:59 ` Ahmad Fatoum 2019-03-27 14:30 ` Ahmad Fatoum 2 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-01-03 9:59 UTC (permalink / raw) To: Stefan Agner; +Cc: Marek Vasut, David Airlie, kernel, dri-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] drm/mxsfb: support swapped RGB lanes 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner 2019-01-02 21:37 ` Sam Ravnborg 2019-01-03 9:59 ` Ahmad Fatoum @ 2019-03-27 14:30 ` Ahmad Fatoum 2 siblings, 0 replies; 12+ messages in thread From: Ahmad Fatoum @ 2019-03-27 14:30 UTC (permalink / raw) To: Stefan Agner; +Cc: Marek Vasut, David Airlie, kernel, dri-devel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-27 14:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-02 17:02 [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 1/3] drm/mxsfb: use bus_format to determine pixel RGB component order Ahmad Fatoum 2019-01-02 17:14 ` Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 2/3] drm/mxsfb: implement interface-pix-fmt of_property to override bus format Ahmad Fatoum 2019-01-02 17:02 ` [PATCH 3/3] dt-bindings: mxsfb: document new interface-pix-fmt property Ahmad Fatoum 2019-01-02 21:05 ` [PATCH 0/3] drm/mxsfb: support swapped RGB lanes Stefan Agner 2019-01-02 21:37 ` Sam Ravnborg 2019-01-07 17:35 ` Ahmad Fatoum 2019-01-07 18:04 ` Sam Ravnborg 2019-03-27 14:26 ` Ahmad Fatoum 2019-01-03 9:59 ` Ahmad Fatoum 2019-03-27 14:30 ` Ahmad Fatoum
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.