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

Ahmad Fatoum a.fatoum at pengutronix.de
Wed Mar 27 14:26:07 UTC 2019


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 at 0 {
>>>                         reg = <0>;
>>>                         #address-cells = <1>;
>>>                         #size-cells = <0>;
>>>                         lcdc_panel_output: endpoint at 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 at 0/endpoint at 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 |


More information about the dri-devel mailing list