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

[Sam Ravnborg <sam@ravnborg.org>]

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