[v2,45/49] drm/omap: Add support for drm_bridge

Submitted by Laurent Pinchart on Jan. 11, 2019, 3:51 a.m.

Details

Message ID 20190111035120.20668-46-laurent.pinchart@ideasonboard.com
State New
Headers show
Series "omapdrm: drm_bridge and drm_panel support" ( rev: 3 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Laurent Pinchart Jan. 11, 2019, 3:51 a.m.
Hook up drm_bridge support in the omapdrm driver. Despite the recent
extensive preparation work, this is a rather intrusive change, as the
management of outputs needs to be adapted through the driver to handle
both omap_dss_device and drm_bridge.

Connector creation is skipped when using a drm_bridge, as the bridge
creates the connector internally. This creates issues with systems that
split connector operations (such as modes retrieval and hot-plug
detection) across different bridges. These systems can't be supported
using drm_bridge for now (their support through the omap_dss_device
infrastructure is not affected), this will be fixed in subsequent
changes.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Changes since v1:

- Fix typo in function name (updata -> update)
---
 drivers/gpu/drm/omapdrm/dss/base.c       | 27 ++++++++--
 drivers/gpu/drm/omapdrm/dss/omapdss.h    |  1 +
 drivers/gpu/drm/omapdrm/dss/output.c     | 21 +++++---
 drivers/gpu/drm/omapdrm/omap_connector.c | 16 ++++--
 drivers/gpu/drm/omapdrm/omap_connector.h |  1 -
 drivers/gpu/drm/omapdrm/omap_crtc.c      |  2 +-
 drivers/gpu/drm/omapdrm/omap_drv.c       | 69 +++++++++++++++++-------
 drivers/gpu/drm/omapdrm/omap_encoder.c   | 69 ++++++++++++++----------
 8 files changed, 145 insertions(+), 61 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
index 81ea0f55cd75..09c9f2971aa2 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -19,6 +19,7 @@ 
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
+#include <linux/platform_device.h>
 
 #include "dss.h"
 #include "omapdss.h"
@@ -156,7 +157,7 @@  struct omap_dss_device *omapdss_device_next_output(struct omap_dss_device *from)
 			goto done;
 		}
 
-		if (dssdev->id && dssdev->next)
+		if (dssdev->id && (dssdev->next || dssdev->bridge))
 			goto done;
 	}
 
@@ -184,7 +185,18 @@  int omapdss_device_connect(struct dss_device *dss,
 {
 	int ret;
 
-	dev_dbg(dst->dev, "connect\n");
+	dev_dbg(&dss->pdev->dev, "connect(%s, %s)\n",
+		src ? dev_name(src->dev) : "NULL",
+		dst ? dev_name(dst->dev) : "NULL");
+
+	if (!dst) {
+		/*
+		 * The destination is NULL when the source is connected to a
+		 * bridge instead of a DSS device. Stop here, we will attach the
+		 * bridge later when we will have a DRM encoder.
+		 */
+		return src && src->bridge ? 0 : -EINVAL;
+	}
 
 	if (omapdss_device_is_connected(dst))
 		return -EBUSY;
@@ -204,7 +216,16 @@  EXPORT_SYMBOL_GPL(omapdss_device_connect);
 void omapdss_device_disconnect(struct omap_dss_device *src,
 			       struct omap_dss_device *dst)
 {
-	dev_dbg(dst->dev, "disconnect\n");
+	struct dss_device *dss = src ? src->dss : dst->dss;
+
+	dev_dbg(&dss->pdev->dev, "disconnect(%s, %s)\n",
+		src ? dev_name(src->dev) : "NULL",
+		dst ? dev_name(dst->dev) : "NULL");
+
+	if (!dst) {
+		WARN_ON(!src->bridge);
+		return;
+	}
 
 	if (!dst->id && !omapdss_device_is_connected(dst)) {
 		WARN_ON(!dst->display);
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index ab5467a1e92c..f47e9b94288f 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -410,6 +410,7 @@  struct omap_dss_device {
 
 	struct dss_device *dss;
 	struct omap_dss_device *next;
+	struct drm_bridge *bridge;
 
 	struct list_head list;
 
diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
index f25ecfd26534..2a53025c2fde 100644
--- a/drivers/gpu/drm/omapdrm/dss/output.c
+++ b/drivers/gpu/drm/omapdrm/dss/output.c
@@ -20,25 +20,34 @@ 
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_graph.h>
 
 #include "dss.h"
 #include "omapdss.h"
 
 int omapdss_device_init_output(struct omap_dss_device *out)
 {
-	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
-	if (IS_ERR(out->next)) {
-		if (PTR_ERR(out->next) != -EPROBE_DEFER)
-			dev_err(out->dev, "failed to find video sink\n");
-		return PTR_ERR(out->next);
+	struct device_node *remote_node;
+
+	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
+	if (!remote_node) {
+		dev_dbg(out->dev, "failed to find video sink\n");
+		return 0;
 	}
 
+	out->next = omapdss_find_device_by_node(remote_node);
+	out->bridge = of_drm_find_bridge(remote_node);
+
+	of_node_put(remote_node);
+
 	if (out->next && out->type != out->next->type) {
 		dev_err(out->dev, "output type and display type don't match\n");
+		omapdss_device_put(out->next);
+		out->next = NULL;
 		return -EINVAL;
 	}
 
-	return 0;
+	return out->next || out->bridge ? 0 : -EPROBE_DEFER;
 }
 EXPORT_SYMBOL(omapdss_device_init_output);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
index 487603c56b08..f528baa80114 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -304,9 +304,16 @@  static const struct drm_connector_helper_funcs omap_connector_helper_funcs = {
 	.mode_valid = omap_connector_mode_valid,
 };
 
-static int omap_connector_get_type(struct omap_dss_device *display)
+static int omap_connector_get_type(struct omap_dss_device *output)
 {
-	switch (display->type) {
+	struct omap_dss_device *display;
+	enum omap_display_type type;
+
+	display = omapdss_display_get(output);
+	type = display->type;
+	omapdss_device_put(display);
+
+	switch (type) {
 	case OMAP_DISPLAY_TYPE_HDMI:
 		return DRM_MODE_CONNECTOR_HDMIA;
 	case OMAP_DISPLAY_TYPE_DVI:
@@ -329,14 +336,13 @@  static int omap_connector_get_type(struct omap_dss_device *display)
 /* initialize connector */
 struct drm_connector *omap_connector_init(struct drm_device *dev,
 					  struct omap_dss_device *output,
-					  struct omap_dss_device *display,
 					  struct drm_encoder *encoder)
 {
 	struct drm_connector *connector = NULL;
 	struct omap_connector *omap_connector;
 	struct omap_dss_device *dssdev;
 
-	DBG("%s", display->name);
+	DBG("%s", output->name);
 
 	omap_connector = kzalloc(sizeof(*omap_connector), GFP_KERNEL);
 	if (!omap_connector)
@@ -349,7 +355,7 @@  struct drm_connector *omap_connector_init(struct drm_device *dev,
 	connector->doublescan_allowed = 0;
 
 	drm_connector_init(dev, connector, &omap_connector_funcs,
-			   omap_connector_get_type(display));
+			   omap_connector_get_type(output));
 	drm_connector_helper_add(connector, &omap_connector_helper_funcs);
 
 	/*
diff --git a/drivers/gpu/drm/omapdrm/omap_connector.h b/drivers/gpu/drm/omapdrm/omap_connector.h
index 6b7d4d95e32b..608085219336 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.h
+++ b/drivers/gpu/drm/omapdrm/omap_connector.h
@@ -31,7 +31,6 @@  struct omap_dss_device;
 
 struct drm_connector *omap_connector_init(struct drm_device *dev,
 					  struct omap_dss_device *output,
-					  struct omap_dss_device *display,
 					  struct drm_encoder *encoder);
 bool omap_connector_get_hdmi_mode(struct drm_connector *connector);
 void omap_connector_enable_hpd(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0cea3824d3a6..4486152fd6cc 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -671,7 +671,7 @@  struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 					&omap_crtc_funcs, NULL);
 	if (ret < 0) {
 		dev_err(dev->dev, "%s(): could not init crtc for: %s\n",
-			__func__, pipe->display->name);
+			__func__, pipe->output->name);
 		kfree(omap_crtc);
 		return ERR_PTR(ret);
 	}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 6b91f44e5a60..35c4669dc69d 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -140,9 +140,7 @@  static void omap_disconnect_pipelines(struct drm_device *ddev)
 		omapdss_device_disconnect(NULL, pipe->output);
 
 		omapdss_device_put(pipe->output);
-		omapdss_device_put(pipe->display);
 		pipe->output = NULL;
-		pipe->display = NULL;
 	}
 
 	memset(&priv->channels, 0, sizeof(priv->channels));
@@ -169,7 +167,6 @@  static int omap_connect_pipelines(struct drm_device *ddev)
 
 			pipe = &priv->pipes[priv->num_pipes++];
 			pipe->output = omapdss_device_get(output);
-			pipe->display = omapdss_display_get(output);
 
 			if (priv->num_pipes == ARRAY_SIZE(priv->pipes)) {
 				/* To balance the 'for_each_dss_output' loop */
@@ -207,6 +204,28 @@  static int omap_modeset_init_properties(struct drm_device *dev)
 	return 0;
 }
 
+static int omap_display_id(struct omap_dss_device *output)
+{
+	struct device_node *node = NULL;
+
+	if (output->next) {
+		struct omap_dss_device *display;
+
+		display = omapdss_display_get(output);
+		node = display->dev->of_node;
+		omapdss_device_put(display);
+	} else {
+		struct drm_bridge *bridge = output->bridge;
+
+		while (bridge->next)
+			bridge = bridge->next;
+
+		node = bridge->of_node;
+	}
+
+	return node ? of_alias_get_id(node, "display") : -ENODEV;
+}
+
 static int omap_modeset_init(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
@@ -262,7 +281,10 @@  static int omap_modeset_init(struct drm_device *dev)
 		priv->planes[priv->num_planes++] = plane;
 	}
 
-	/* Create the encoders and get the pipelines aliases. */
+	/*
+	 * Create the encoders, attach the bridges and get the pipeline alias
+	 * IDs.
+	 */
 	for (i = 0; i < priv->num_pipes; i++) {
 		struct omap_drm_pipeline *pipe = &priv->pipes[i];
 		int id;
@@ -271,7 +293,14 @@  static int omap_modeset_init(struct drm_device *dev)
 		if (!pipe->encoder)
 			return -ENOMEM;
 
-		id = of_alias_get_id(pipe->display->dev->of_node, "display");
+		if (pipe->output->bridge) {
+			ret = drm_bridge_attach(pipe->encoder,
+						pipe->output->bridge, NULL);
+			if (ret < 0)
+				return ret;
+		}
+
+		id = omap_display_id(pipe->output);
 		pipe->alias_id = id >= 0 ? id : i;
 	}
 
@@ -297,16 +326,16 @@  static int omap_modeset_init(struct drm_device *dev)
 	for (i = 0; i < priv->num_pipes; i++) {
 		struct omap_drm_pipeline *pipe = &priv->pipes[i];
 		struct drm_encoder *encoder = pipe->encoder;
-		struct drm_connector *connector;
 		struct drm_crtc *crtc;
 
-		connector = omap_connector_init(dev, pipe->output,
-						pipe->display, encoder);
-		if (!connector)
-			return -ENOMEM;
+		if (!pipe->output->bridge) {
+			pipe->connector = omap_connector_init(dev, pipe->output,
+							      encoder);
+			if (!pipe->connector)
+				return -ENOMEM;
 
-		drm_connector_attach_encoder(connector, encoder);
-		pipe->connector = connector;
+			drm_connector_attach_encoder(pipe->connector, encoder);
+		}
 
 		crtc = omap_crtc_init(dev, pipe, priv->planes[i]);
 		if (IS_ERR(crtc))
@@ -350,10 +379,12 @@  static int omap_modeset_init(struct drm_device *dev)
 static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
-	int i;
+	unsigned int i;
 
-	for (i = 0; i < priv->num_pipes; i++)
-		omap_connector_enable_hpd(priv->pipes[i].connector);
+	for (i = 0; i < priv->num_pipes; i++) {
+		if (priv->pipes[i].connector)
+			omap_connector_enable_hpd(priv->pipes[i].connector);
+	}
 }
 
 /*
@@ -362,10 +393,12 @@  static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
 static void omap_modeset_disable_external_hpd(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
-	int i;
+	unsigned int i;
 
-	for (i = 0; i < priv->num_pipes; i++)
-		omap_connector_disable_hpd(priv->pipes[i].connector);
+	for (i = 0; i < priv->num_pipes; i++) {
+		if (priv->pipes[i].connector)
+			omap_connector_disable_hpd(priv->pipes[i].connector);
+	}
 }
 
 /*
diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index e71d359a8f07..76f94cc0c0cf 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -51,6 +51,34 @@  static const struct drm_encoder_funcs omap_encoder_funcs = {
 	.destroy = omap_encoder_destroy,
 };
 
+static void omap_encoder_update_videomode_flags(struct videomode *vm,
+						u32 bus_flags)
+{
+	if (!(vm->flags & (DISPLAY_FLAGS_DE_LOW |
+			   DISPLAY_FLAGS_DE_HIGH))) {
+		if (bus_flags & DRM_BUS_FLAG_DE_LOW)
+			vm->flags |= DISPLAY_FLAGS_DE_LOW;
+		else if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
+			vm->flags |= DISPLAY_FLAGS_DE_HIGH;
+	}
+
+	if (!(vm->flags & (DISPLAY_FLAGS_PIXDATA_POSEDGE |
+			   DISPLAY_FLAGS_PIXDATA_NEGEDGE))) {
+		if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+			vm->flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE;
+		else if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+			vm->flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE;
+	}
+
+	if (!(vm->flags & (DISPLAY_FLAGS_SYNC_POSEDGE |
+			   DISPLAY_FLAGS_SYNC_NEGEDGE))) {
+		if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE)
+			vm->flags |= DISPLAY_FLAGS_SYNC_POSEDGE;
+		else if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE)
+			vm->flags |= DISPLAY_FLAGS_SYNC_NEGEDGE;
+	}
+}
+
 static void omap_encoder_hdmi_mode_set(struct drm_encoder *encoder,
 				       struct drm_display_mode *adjusted_mode)
 {
@@ -87,7 +115,9 @@  static void omap_encoder_mode_set(struct drm_encoder *encoder,
 				  struct drm_display_mode *adjusted_mode)
 {
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
+	struct omap_dss_device *output = omap_encoder->output;
 	struct omap_dss_device *dssdev;
+	struct drm_bridge *bridge;
 	struct videomode vm = { 0 };
 
 	drm_display_mode_to_videomode(adjusted_mode, &vm);
@@ -101,44 +131,29 @@  static void omap_encoder_mode_set(struct drm_encoder *encoder,
 	 *
 	 * A better solution is to use DRM's bus-flags through the whole driver.
 	 */
-	for (dssdev = omap_encoder->output; dssdev; dssdev = dssdev->next) {
-		unsigned long bus_flags = dssdev->bus_flags;
-
-		if (!(vm.flags & (DISPLAY_FLAGS_DE_LOW |
-				  DISPLAY_FLAGS_DE_HIGH))) {
-			if (bus_flags & DRM_BUS_FLAG_DE_LOW)
-				vm.flags |= DISPLAY_FLAGS_DE_LOW;
-			else if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
-				vm.flags |= DISPLAY_FLAGS_DE_HIGH;
-		}
+	for (dssdev = output; dssdev; dssdev = dssdev->next)
+		omap_encoder_update_videomode_flags(&vm, dssdev->bus_flags);
 
-		if (!(vm.flags & (DISPLAY_FLAGS_PIXDATA_POSEDGE |
-				  DISPLAY_FLAGS_PIXDATA_NEGEDGE))) {
-			if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
-				vm.flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE;
-			else if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
-				vm.flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE;
-		}
+	for (bridge = output->bridge; bridge; bridge = bridge->next) {
+		u32 bus_flags;
 
-		if (!(vm.flags & (DISPLAY_FLAGS_SYNC_POSEDGE |
-				  DISPLAY_FLAGS_SYNC_NEGEDGE))) {
-			if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE)
-				vm.flags |= DISPLAY_FLAGS_SYNC_POSEDGE;
-			else if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE)
-				vm.flags |= DISPLAY_FLAGS_SYNC_NEGEDGE;
-		}
+		if (!bridge->timings)
+			continue;
+
+		bus_flags = bridge->timings->input_bus_flags;
+		omap_encoder_update_videomode_flags(&vm, bus_flags);
 	}
 
 	/* Set timings for all devices in the display pipeline. */
-	dss_mgr_set_timings(omap_encoder->output, &vm);
+	dss_mgr_set_timings(output, &vm);
 
-	for (dssdev = omap_encoder->output; dssdev; dssdev = dssdev->next) {
+	for (dssdev = output; dssdev; dssdev = dssdev->next) {
 		if (dssdev->ops->set_timings)
 			dssdev->ops->set_timings(dssdev, adjusted_mode);
 	}
 
 	/* Set the HDMI mode and HDMI infoframe if applicable. */
-	if (omap_encoder->output->type == OMAP_DISPLAY_TYPE_HDMI)
+	if (output->type == OMAP_DISPLAY_TYPE_HDMI)
 		omap_encoder_hdmi_mode_set(encoder, adjusted_mode);
 }
 

Comments

Hi Laurent,

On 11/01/19 05:51, Laurent Pinchart wrote:
> Hook up drm_bridge support in the omapdrm driver. Despite the recent
> extensive preparation work, this is a rather intrusive change, as the
> management of outputs needs to be adapted through the driver to handle
> both omap_dss_device and drm_bridge.
> 
> Connector creation is skipped when using a drm_bridge, as the bridge
> creates the connector internally. This creates issues with systems that
> split connector operations (such as modes retrieval and hot-plug
> detection) across different bridges. These systems can't be supported
> using drm_bridge for now (their support through the omap_dss_device
> infrastructure is not affected), this will be fixed in subsequent
> changes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

<snip>

> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
> index f25ecfd26534..2a53025c2fde 100644
> --- a/drivers/gpu/drm/omapdrm/dss/output.c
> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> @@ -20,25 +20,34 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  
>  #include "dss.h"
>  #include "omapdss.h"
>  
>  int omapdss_device_init_output(struct omap_dss_device *out)
>  {
> -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
> -	if (IS_ERR(out->next)) {
> -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
> -			dev_err(out->dev, "failed to find video sink\n");
> -		return PTR_ERR(out->next);
> +	struct device_node *remote_node;
> +
> +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
> +	if (!remote_node) {
> +		dev_dbg(out->dev, "failed to find video sink\n");
> +		return 0;
>  	}

This breaks boards that have displays/bridges connected to ports above
0. For example, DPI output has 3 ports on some SoCs.

I made a quick fix like so:

+       port_num = __ffs(out->of_ports);
+
+       remote_node = of_graph_get_remote_node(out->dev->of_node,
port_num, 0);

Which I think works for all our outputs as we only set a single bit in
of_ports for outputs. But I don't think that's quite correct. Should we
have another field which tells which port is to be used? Then again,
maybe __ffs() is good enough here, as we can guarantee that there's ever
a single port in of_ports.

 Tomi
Hi Tomi,

On Fri, Jan 18, 2019 at 12:33:03PM +0200, Tomi Valkeinen wrote:
> On 11/01/19 05:51, Laurent Pinchart wrote:
> > Hook up drm_bridge support in the omapdrm driver. Despite the recent
> > extensive preparation work, this is a rather intrusive change, as the
> > management of outputs needs to be adapted through the driver to handle
> > both omap_dss_device and drm_bridge.
> > 
> > Connector creation is skipped when using a drm_bridge, as the bridge
> > creates the connector internally. This creates issues with systems that
> > split connector operations (such as modes retrieval and hot-plug
> > detection) across different bridges. These systems can't be supported
> > using drm_bridge for now (their support through the omap_dss_device
> > infrastructure is not affected), this will be fixed in subsequent
> > changes.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> <snip>
> 
> > diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
> > index f25ecfd26534..2a53025c2fde 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/output.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> > @@ -20,25 +20,34 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/of.h>
> > +#include <linux/of_graph.h>
> >  
> >  #include "dss.h"
> >  #include "omapdss.h"
> >  
> >  int omapdss_device_init_output(struct omap_dss_device *out)
> >  {
> > -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
> > -	if (IS_ERR(out->next)) {
> > -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
> > -			dev_err(out->dev, "failed to find video sink\n");
> > -		return PTR_ERR(out->next);
> > +	struct device_node *remote_node;
> > +
> > +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
> > +	if (!remote_node) {
> > +		dev_dbg(out->dev, "failed to find video sink\n");
> > +		return 0;
> >  	}
> 
> This breaks boards that have displays/bridges connected to ports above
> 0. For example, DPI output has 3 ports on some SoCs.

Does it break them, or are they already broken ?
omapdss_of_find_connected_device() is defined as follows:

struct omap_dss_device *
omapdss_of_find_connected_device(struct device_node *node, unsigned int port)
{
        struct device_node *remote_node;
        struct omap_dss_device *dssdev;

        remote_node = of_graph_get_remote_node(node, port, 0);
        if (!remote_node)
                return NULL;

        dssdev = omapdss_find_device_by_node(remote_node);
        of_node_put(remote_node);

        return dssdev ? dssdev : ERR_PTR(-EPROBE_DEFER);
}

Before this patch is was called with port set to 0, so there's no change
here.

> I made a quick fix like so:
> 
> +       port_num = __ffs(out->of_ports);
> +
> +       remote_node = of_graph_get_remote_node(out->dev->of_node,
> port_num, 0);
> 
> Which I think works for all our outputs as we only set a single bit in
> of_ports for outputs. But I don't think that's quite correct. Should we
> have another field which tells which port is to be used? Then again,
> maybe __ffs() is good enough here, as we can guarantee that there's ever
> a single port in of_ports.

Down the road I think it would make sense to replace of_ports with
of_port. I thought we can't do so right now as we have three encoder
drivers that set two bits in of_ports, but now that I've double-checked,
of_ports isn't used. We could thus replace it, and only set it for the
internal DSS devices.

If my analysis is correct and the problem isn't introduced by this
patch, could it be fixed on top of the series ?
Hi,

On Fri, Jan 11, 2019 at 05:51:16AM +0200, Laurent Pinchart wrote:
> Hook up drm_bridge support in the omapdrm driver. Despite the recent
> extensive preparation work, this is a rather intrusive change, as the
> management of outputs needs to be adapted through the driver to handle
> both omap_dss_device and drm_bridge.
> 
> Connector creation is skipped when using a drm_bridge, as the bridge
> creates the connector internally. This creates issues with systems that
> split connector operations (such as modes retrieval and hot-plug
> detection) across different bridges. These systems can't be supported
> using drm_bridge for now (their support through the omap_dss_device
> infrastructure is not affected), this will be fixed in subsequent
> changes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> Changes since v1:
> 
> - Fix typo in function name (updata -> update)
> ---

This patch drops all usage of pipe->display. It should probably also
remove the struct entry to simplify things and avoid stupid mistakes
when somebody rebases patches (*cough*).

-- Sebastian
Hi,

On Fri, Jan 11, 2019 at 05:51:16AM +0200, Laurent Pinchart wrote:
> Hook up drm_bridge support in the omapdrm driver. Despite the recent
> extensive preparation work, this is a rather intrusive change, as the
> management of outputs needs to be adapted through the driver to handle
> both omap_dss_device and drm_bridge.
> 
> Connector creation is skipped when using a drm_bridge, as the bridge
> creates the connector internally. This creates issues with systems that
> split connector operations (such as modes retrieval and hot-plug
> detection) across different bridges. These systems can't be supported
> using drm_bridge for now (their support through the omap_dss_device
> infrastructure is not affected), this will be fixed in subsequent
> changes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---

Tested-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> Changes since v1:
> 
> - Fix typo in function name (updata -> update)
> ---
>  drivers/gpu/drm/omapdrm/dss/base.c       | 27 ++++++++--
>  drivers/gpu/drm/omapdrm/dss/omapdss.h    |  1 +
>  drivers/gpu/drm/omapdrm/dss/output.c     | 21 +++++---
>  drivers/gpu/drm/omapdrm/omap_connector.c | 16 ++++--
>  drivers/gpu/drm/omapdrm/omap_connector.h |  1 -
>  drivers/gpu/drm/omapdrm/omap_crtc.c      |  2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c       | 69 +++++++++++++++++-------
>  drivers/gpu/drm/omapdrm/omap_encoder.c   | 69 ++++++++++++++----------
>  8 files changed, 145 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
> index 81ea0f55cd75..09c9f2971aa2 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -19,6 +19,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> +#include <linux/platform_device.h>
>  
>  #include "dss.h"
>  #include "omapdss.h"
> @@ -156,7 +157,7 @@ struct omap_dss_device *omapdss_device_next_output(struct omap_dss_device *from)
>  			goto done;
>  		}
>  
> -		if (dssdev->id && dssdev->next)
> +		if (dssdev->id && (dssdev->next || dssdev->bridge))
>  			goto done;
>  	}
>  
> @@ -184,7 +185,18 @@ int omapdss_device_connect(struct dss_device *dss,
>  {
>  	int ret;
>  
> -	dev_dbg(dst->dev, "connect\n");
> +	dev_dbg(&dss->pdev->dev, "connect(%s, %s)\n",
> +		src ? dev_name(src->dev) : "NULL",
> +		dst ? dev_name(dst->dev) : "NULL");
> +
> +	if (!dst) {
> +		/*
> +		 * The destination is NULL when the source is connected to a
> +		 * bridge instead of a DSS device. Stop here, we will attach the
> +		 * bridge later when we will have a DRM encoder.
> +		 */
> +		return src && src->bridge ? 0 : -EINVAL;
> +	}
>  
>  	if (omapdss_device_is_connected(dst))
>  		return -EBUSY;
> @@ -204,7 +216,16 @@ EXPORT_SYMBOL_GPL(omapdss_device_connect);
>  void omapdss_device_disconnect(struct omap_dss_device *src,
>  			       struct omap_dss_device *dst)
>  {
> -	dev_dbg(dst->dev, "disconnect\n");
> +	struct dss_device *dss = src ? src->dss : dst->dss;
> +
> +	dev_dbg(&dss->pdev->dev, "disconnect(%s, %s)\n",
> +		src ? dev_name(src->dev) : "NULL",
> +		dst ? dev_name(dst->dev) : "NULL");
> +
> +	if (!dst) {
> +		WARN_ON(!src->bridge);
> +		return;
> +	}
>  
>  	if (!dst->id && !omapdss_device_is_connected(dst)) {
>  		WARN_ON(!dst->display);
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index ab5467a1e92c..f47e9b94288f 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -410,6 +410,7 @@ struct omap_dss_device {
>  
>  	struct dss_device *dss;
>  	struct omap_dss_device *next;
> +	struct drm_bridge *bridge;
>  
>  	struct list_head list;
>  
> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
> index f25ecfd26534..2a53025c2fde 100644
> --- a/drivers/gpu/drm/omapdrm/dss/output.c
> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> @@ -20,25 +20,34 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_graph.h>
>  
>  #include "dss.h"
>  #include "omapdss.h"
>  
>  int omapdss_device_init_output(struct omap_dss_device *out)
>  {
> -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
> -	if (IS_ERR(out->next)) {
> -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
> -			dev_err(out->dev, "failed to find video sink\n");
> -		return PTR_ERR(out->next);
> +	struct device_node *remote_node;
> +
> +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
> +	if (!remote_node) {
> +		dev_dbg(out->dev, "failed to find video sink\n");
> +		return 0;
>  	}
>  
> +	out->next = omapdss_find_device_by_node(remote_node);
> +	out->bridge = of_drm_find_bridge(remote_node);
> +
> +	of_node_put(remote_node);
> +
>  	if (out->next && out->type != out->next->type) {
>  		dev_err(out->dev, "output type and display type don't match\n");
> +		omapdss_device_put(out->next);
> +		out->next = NULL;
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return out->next || out->bridge ? 0 : -EPROBE_DEFER;
>  }
>  EXPORT_SYMBOL(omapdss_device_init_output);
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 487603c56b08..f528baa80114 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -304,9 +304,16 @@ static const struct drm_connector_helper_funcs omap_connector_helper_funcs = {
>  	.mode_valid = omap_connector_mode_valid,
>  };
>  
> -static int omap_connector_get_type(struct omap_dss_device *display)
> +static int omap_connector_get_type(struct omap_dss_device *output)
>  {
> -	switch (display->type) {
> +	struct omap_dss_device *display;
> +	enum omap_display_type type;
> +
> +	display = omapdss_display_get(output);
> +	type = display->type;
> +	omapdss_device_put(display);
> +
> +	switch (type) {
>  	case OMAP_DISPLAY_TYPE_HDMI:
>  		return DRM_MODE_CONNECTOR_HDMIA;
>  	case OMAP_DISPLAY_TYPE_DVI:
> @@ -329,14 +336,13 @@ static int omap_connector_get_type(struct omap_dss_device *display)
>  /* initialize connector */
>  struct drm_connector *omap_connector_init(struct drm_device *dev,
>  					  struct omap_dss_device *output,
> -					  struct omap_dss_device *display,
>  					  struct drm_encoder *encoder)
>  {
>  	struct drm_connector *connector = NULL;
>  	struct omap_connector *omap_connector;
>  	struct omap_dss_device *dssdev;
>  
> -	DBG("%s", display->name);
> +	DBG("%s", output->name);
>  
>  	omap_connector = kzalloc(sizeof(*omap_connector), GFP_KERNEL);
>  	if (!omap_connector)
> @@ -349,7 +355,7 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
>  	connector->doublescan_allowed = 0;
>  
>  	drm_connector_init(dev, connector, &omap_connector_funcs,
> -			   omap_connector_get_type(display));
> +			   omap_connector_get_type(output));
>  	drm_connector_helper_add(connector, &omap_connector_helper_funcs);
>  
>  	/*
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.h b/drivers/gpu/drm/omapdrm/omap_connector.h
> index 6b7d4d95e32b..608085219336 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.h
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.h
> @@ -31,7 +31,6 @@ struct omap_dss_device;
>  
>  struct drm_connector *omap_connector_init(struct drm_device *dev,
>  					  struct omap_dss_device *output,
> -					  struct omap_dss_device *display,
>  					  struct drm_encoder *encoder);
>  bool omap_connector_get_hdmi_mode(struct drm_connector *connector);
>  void omap_connector_enable_hpd(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 0cea3824d3a6..4486152fd6cc 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -671,7 +671,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  					&omap_crtc_funcs, NULL);
>  	if (ret < 0) {
>  		dev_err(dev->dev, "%s(): could not init crtc for: %s\n",
> -			__func__, pipe->display->name);
> +			__func__, pipe->output->name);
>  		kfree(omap_crtc);
>  		return ERR_PTR(ret);
>  	}
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 6b91f44e5a60..35c4669dc69d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -140,9 +140,7 @@ static void omap_disconnect_pipelines(struct drm_device *ddev)
>  		omapdss_device_disconnect(NULL, pipe->output);
>  
>  		omapdss_device_put(pipe->output);
> -		omapdss_device_put(pipe->display);
>  		pipe->output = NULL;
> -		pipe->display = NULL;
>  	}
>  
>  	memset(&priv->channels, 0, sizeof(priv->channels));
> @@ -169,7 +167,6 @@ static int omap_connect_pipelines(struct drm_device *ddev)
>  
>  			pipe = &priv->pipes[priv->num_pipes++];
>  			pipe->output = omapdss_device_get(output);
> -			pipe->display = omapdss_display_get(output);
>  
>  			if (priv->num_pipes == ARRAY_SIZE(priv->pipes)) {
>  				/* To balance the 'for_each_dss_output' loop */
> @@ -207,6 +204,28 @@ static int omap_modeset_init_properties(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static int omap_display_id(struct omap_dss_device *output)
> +{
> +	struct device_node *node = NULL;
> +
> +	if (output->next) {
> +		struct omap_dss_device *display;
> +
> +		display = omapdss_display_get(output);
> +		node = display->dev->of_node;
> +		omapdss_device_put(display);
> +	} else {
> +		struct drm_bridge *bridge = output->bridge;
> +
> +		while (bridge->next)
> +			bridge = bridge->next;
> +
> +		node = bridge->of_node;
> +	}
> +
> +	return node ? of_alias_get_id(node, "display") : -ENODEV;
> +}
> +
>  static int omap_modeset_init(struct drm_device *dev)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
> @@ -262,7 +281,10 @@ static int omap_modeset_init(struct drm_device *dev)
>  		priv->planes[priv->num_planes++] = plane;
>  	}
>  
> -	/* Create the encoders and get the pipelines aliases. */
> +	/*
> +	 * Create the encoders, attach the bridges and get the pipeline alias
> +	 * IDs.
> +	 */
>  	for (i = 0; i < priv->num_pipes; i++) {
>  		struct omap_drm_pipeline *pipe = &priv->pipes[i];
>  		int id;
> @@ -271,7 +293,14 @@ static int omap_modeset_init(struct drm_device *dev)
>  		if (!pipe->encoder)
>  			return -ENOMEM;
>  
> -		id = of_alias_get_id(pipe->display->dev->of_node, "display");
> +		if (pipe->output->bridge) {
> +			ret = drm_bridge_attach(pipe->encoder,
> +						pipe->output->bridge, NULL);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		id = omap_display_id(pipe->output);
>  		pipe->alias_id = id >= 0 ? id : i;
>  	}
>  
> @@ -297,16 +326,16 @@ static int omap_modeset_init(struct drm_device *dev)
>  	for (i = 0; i < priv->num_pipes; i++) {
>  		struct omap_drm_pipeline *pipe = &priv->pipes[i];
>  		struct drm_encoder *encoder = pipe->encoder;
> -		struct drm_connector *connector;
>  		struct drm_crtc *crtc;
>  
> -		connector = omap_connector_init(dev, pipe->output,
> -						pipe->display, encoder);
> -		if (!connector)
> -			return -ENOMEM;
> +		if (!pipe->output->bridge) {
> +			pipe->connector = omap_connector_init(dev, pipe->output,
> +							      encoder);
> +			if (!pipe->connector)
> +				return -ENOMEM;
>  
> -		drm_connector_attach_encoder(connector, encoder);
> -		pipe->connector = connector;
> +			drm_connector_attach_encoder(pipe->connector, encoder);
> +		}
>  
>  		crtc = omap_crtc_init(dev, pipe, priv->planes[i]);
>  		if (IS_ERR(crtc))
> @@ -350,10 +379,12 @@ static int omap_modeset_init(struct drm_device *dev)
>  static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
> -	int i;
> +	unsigned int i;
>  
> -	for (i = 0; i < priv->num_pipes; i++)
> -		omap_connector_enable_hpd(priv->pipes[i].connector);
> +	for (i = 0; i < priv->num_pipes; i++) {
> +		if (priv->pipes[i].connector)
> +			omap_connector_enable_hpd(priv->pipes[i].connector);
> +	}
>  }
>  
>  /*
> @@ -362,10 +393,12 @@ static void omap_modeset_enable_external_hpd(struct drm_device *ddev)
>  static void omap_modeset_disable_external_hpd(struct drm_device *ddev)
>  {
>  	struct omap_drm_private *priv = ddev->dev_private;
> -	int i;
> +	unsigned int i;
>  
> -	for (i = 0; i < priv->num_pipes; i++)
> -		omap_connector_disable_hpd(priv->pipes[i].connector);
> +	for (i = 0; i < priv->num_pipes; i++) {
> +		if (priv->pipes[i].connector)
> +			omap_connector_disable_hpd(priv->pipes[i].connector);
> +	}
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
> index e71d359a8f07..76f94cc0c0cf 100644
> --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> @@ -51,6 +51,34 @@ static const struct drm_encoder_funcs omap_encoder_funcs = {
>  	.destroy = omap_encoder_destroy,
>  };
>  
> +static void omap_encoder_update_videomode_flags(struct videomode *vm,
> +						u32 bus_flags)
> +{
> +	if (!(vm->flags & (DISPLAY_FLAGS_DE_LOW |
> +			   DISPLAY_FLAGS_DE_HIGH))) {
> +		if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> +			vm->flags |= DISPLAY_FLAGS_DE_LOW;
> +		else if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
> +			vm->flags |= DISPLAY_FLAGS_DE_HIGH;
> +	}
> +
> +	if (!(vm->flags & (DISPLAY_FLAGS_PIXDATA_POSEDGE |
> +			   DISPLAY_FLAGS_PIXDATA_NEGEDGE))) {
> +		if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> +			vm->flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE;
> +		else if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> +			vm->flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE;
> +	}
> +
> +	if (!(vm->flags & (DISPLAY_FLAGS_SYNC_POSEDGE |
> +			   DISPLAY_FLAGS_SYNC_NEGEDGE))) {
> +		if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE)
> +			vm->flags |= DISPLAY_FLAGS_SYNC_POSEDGE;
> +		else if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE)
> +			vm->flags |= DISPLAY_FLAGS_SYNC_NEGEDGE;
> +	}
> +}
> +
>  static void omap_encoder_hdmi_mode_set(struct drm_encoder *encoder,
>  				       struct drm_display_mode *adjusted_mode)
>  {
> @@ -87,7 +115,9 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
>  				  struct drm_display_mode *adjusted_mode)
>  {
>  	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
> +	struct omap_dss_device *output = omap_encoder->output;
>  	struct omap_dss_device *dssdev;
> +	struct drm_bridge *bridge;
>  	struct videomode vm = { 0 };
>  
>  	drm_display_mode_to_videomode(adjusted_mode, &vm);
> @@ -101,44 +131,29 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder,
>  	 *
>  	 * A better solution is to use DRM's bus-flags through the whole driver.
>  	 */
> -	for (dssdev = omap_encoder->output; dssdev; dssdev = dssdev->next) {
> -		unsigned long bus_flags = dssdev->bus_flags;
> -
> -		if (!(vm.flags & (DISPLAY_FLAGS_DE_LOW |
> -				  DISPLAY_FLAGS_DE_HIGH))) {
> -			if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> -				vm.flags |= DISPLAY_FLAGS_DE_LOW;
> -			else if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
> -				vm.flags |= DISPLAY_FLAGS_DE_HIGH;
> -		}
> +	for (dssdev = output; dssdev; dssdev = dssdev->next)
> +		omap_encoder_update_videomode_flags(&vm, dssdev->bus_flags);
>  
> -		if (!(vm.flags & (DISPLAY_FLAGS_PIXDATA_POSEDGE |
> -				  DISPLAY_FLAGS_PIXDATA_NEGEDGE))) {
> -			if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
> -				vm.flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE;
> -			else if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> -				vm.flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE;
> -		}
> +	for (bridge = output->bridge; bridge; bridge = bridge->next) {
> +		u32 bus_flags;
>  
> -		if (!(vm.flags & (DISPLAY_FLAGS_SYNC_POSEDGE |
> -				  DISPLAY_FLAGS_SYNC_NEGEDGE))) {
> -			if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE)
> -				vm.flags |= DISPLAY_FLAGS_SYNC_POSEDGE;
> -			else if (bus_flags & DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE)
> -				vm.flags |= DISPLAY_FLAGS_SYNC_NEGEDGE;
> -		}
> +		if (!bridge->timings)
> +			continue;
> +
> +		bus_flags = bridge->timings->input_bus_flags;
> +		omap_encoder_update_videomode_flags(&vm, bus_flags);
>  	}
>  
>  	/* Set timings for all devices in the display pipeline. */
> -	dss_mgr_set_timings(omap_encoder->output, &vm);
> +	dss_mgr_set_timings(output, &vm);
>  
> -	for (dssdev = omap_encoder->output; dssdev; dssdev = dssdev->next) {
> +	for (dssdev = output; dssdev; dssdev = dssdev->next) {
>  		if (dssdev->ops->set_timings)
>  			dssdev->ops->set_timings(dssdev, adjusted_mode);
>  	}
>  
>  	/* Set the HDMI mode and HDMI infoframe if applicable. */
> -	if (omap_encoder->output->type == OMAP_DISPLAY_TYPE_HDMI)
> +	if (output->type == OMAP_DISPLAY_TYPE_HDMI)
>  		omap_encoder_hdmi_mode_set(encoder, adjusted_mode);
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 08/02/2019 17:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, Jan 18, 2019 at 12:33:03PM +0200, Tomi Valkeinen wrote:
>> On 11/01/19 05:51, Laurent Pinchart wrote:
>>> Hook up drm_bridge support in the omapdrm driver. Despite the recent
>>> extensive preparation work, this is a rather intrusive change, as the
>>> management of outputs needs to be adapted through the driver to handle
>>> both omap_dss_device and drm_bridge.
>>>
>>> Connector creation is skipped when using a drm_bridge, as the bridge
>>> creates the connector internally. This creates issues with systems that
>>> split connector operations (such as modes retrieval and hot-plug
>>> detection) across different bridges. These systems can't be supported
>>> using drm_bridge for now (their support through the omap_dss_device
>>> infrastructure is not affected), this will be fixed in subsequent
>>> changes.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>>
>> <snip>
>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
>>> index f25ecfd26534..2a53025c2fde 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
>>> @@ -20,25 +20,34 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/of.h>
>>> +#include <linux/of_graph.h>
>>>  
>>>  #include "dss.h"
>>>  #include "omapdss.h"
>>>  
>>>  int omapdss_device_init_output(struct omap_dss_device *out)
>>>  {
>>> -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
>>> -	if (IS_ERR(out->next)) {
>>> -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
>>> -			dev_err(out->dev, "failed to find video sink\n");
>>> -		return PTR_ERR(out->next);
>>> +	struct device_node *remote_node;
>>> +
>>> +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
>>> +	if (!remote_node) {
>>> +		dev_dbg(out->dev, "failed to find video sink\n");
>>> +		return 0;
>>>  	}
>>
>> This breaks boards that have displays/bridges connected to ports above
>> 0. For example, DPI output has 3 ports on some SoCs.
> 
> Does it break them, or are they already broken ?

Good point. It's very well possible it was already broken. It worked in
4.14, that I know =). That didn't have
omapdss_of_find_connected_device() yet. So possibly it's
27d624527d99265c2df999af3615ff71c29d06f4 that breaks it.

I think we don't have many boards that use non-0 port for DPI. The one I
have, DRA71 EVM, doesn't have mainline support, and I have only ran it
on 4.14 before this.

And OMAP3 SDI uses port 1, but it handles this one specifically.

>> I made a quick fix like so:
>>
>> +       port_num = __ffs(out->of_ports);
>> +
>> +       remote_node = of_graph_get_remote_node(out->dev->of_node,
>> port_num, 0);
>>
>> Which I think works for all our outputs as we only set a single bit in
>> of_ports for outputs. But I don't think that's quite correct. Should we
>> have another field which tells which port is to be used? Then again,
>> maybe __ffs() is good enough here, as we can guarantee that there's ever
>> a single port in of_ports.
> 
> Down the road I think it would make sense to replace of_ports with
> of_port. I thought we can't do so right now as we have three encoder
> drivers that set two bits in of_ports, but now that I've double-checked,
> of_ports isn't used. We could thus replace it, and only set it for the
> internal DSS devices.

Yes, this would clean up the code a bit.

> If my analysis is correct and the problem isn't introduced by this
> patch, could it be fixed on top of the series ?

Yes, that sounds fine.

 Tomi