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

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 17462 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel