| Message ID | 20190102170247.8208-1-a.fatoum@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mxsfb: support swapped RGB lanes | expand |
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
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
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; }
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; > } > > >
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(-) >
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(-)