dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] [rfc] drm/mst: split connector registration into two parts
@ 2015-09-16  7:55 Dave Airlie
  2015-09-30  8:02 ` Daniel Vetter
  0 siblings, 1 reply; 2+ messages in thread
From: Dave Airlie @ 2015-09-16  7:55 UTC (permalink / raw)
  To: dri-devel

From: Dave Airlie <airlied@redhat.com>

In order to cache the EDID properly for tiled displays, we
need to retrieve it before we register the connector with
userspace, otherwise userspace can call get resources
and try and get the edid before we've even cached it.

This fixes some problems when hotplugging mst monitors,
with X/mutter running.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c  |  1 +
 drivers/gpu/drm/i915/intel_dp_mst.c    |  9 ++++++++-
 drivers/gpu/drm/radeon/radeon_dp_mst.c | 10 +++++++++-
 include/drm/drm_dp_mst_helper.h        |  1 +
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index f6acb57..1b0ca1f 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1129,6 +1129,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 		if (port->port_num >= 8) {
 			port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc);
 		}
+		(*mstb->mgr->cbs->register_connector)(port->connector);
 	}
 
 out:
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3e4be5a..6ade068 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -462,11 +462,17 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
 
 	drm_mode_connector_set_path_property(connector, pathprop);
+	return connector;
+}
+
+static void intel_dp_register_mst_connector(struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct drm_device *dev = connector->dev;
 	drm_modeset_lock_all(dev);
 	intel_connector_add_to_fbdev(intel_connector);
 	drm_modeset_unlock_all(dev);
 	drm_connector_register(&intel_connector->base);
-	return connector;
 }
 
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
@@ -512,6 +518,7 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 
 static struct drm_dp_mst_topology_cbs mst_cbs = {
 	.add_connector = intel_dp_add_mst_connector,
+	.register_connector = intel_dp_register_mst_connector,
 	.destroy_connector = intel_dp_destroy_mst_connector,
 	.hotplug = intel_dp_mst_hotplug,
 };
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 5e09c06..9a9193b 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -286,12 +286,19 @@ static struct drm_connector *radeon_dp_add_mst_connector(struct drm_dp_mst_topol
 	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
 	drm_mode_connector_set_path_property(connector, pathprop);
 
+	return connector;
+}
+
+static void radeon_dp_register_mst_connector(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct radeon_device *rdev = dev->dev_private;
+
 	drm_modeset_lock_all(dev);
 	radeon_fb_add_connector(rdev, connector);
 	drm_modeset_unlock_all(dev);
 
 	drm_connector_register(connector);
-	return connector;
 }
 
 static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
@@ -324,6 +331,7 @@ static void radeon_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
 
 struct drm_dp_mst_topology_cbs mst_cbs = {
 	.add_connector = radeon_dp_add_mst_connector,
+	.register_connector = radeon_dp_register_mst_connector,
 	.destroy_connector = radeon_dp_destroy_mst_connector,
 	.hotplug = radeon_dp_mst_hotplug,
 };
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 86d0b25..0f408b0 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -374,6 +374,7 @@ struct drm_dp_mst_topology_mgr;
 struct drm_dp_mst_topology_cbs {
 	/* create a connector for a port */
 	struct drm_connector *(*add_connector)(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, const char *path);
+	void (*register_connector)(struct drm_connector *connector);
 	void (*destroy_connector)(struct drm_dp_mst_topology_mgr *mgr,
 				  struct drm_connector *connector);
 	void (*hotplug)(struct drm_dp_mst_topology_mgr *mgr);
-- 
2.5.0

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

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] [rfc] drm/mst: split connector registration into two parts
  2015-09-16  7:55 [PATCH] [rfc] drm/mst: split connector registration into two parts Dave Airlie
@ 2015-09-30  8:02 ` Daniel Vetter
  0 siblings, 0 replies; 2+ messages in thread
From: Daniel Vetter @ 2015-09-30  8:02 UTC (permalink / raw)
  To: Dave Airlie; +Cc: dri-devel

On Wed, Sep 16, 2015 at 05:55:23PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> In order to cache the EDID properly for tiled displays, we
> need to retrieve it before we register the connector with
> userspace, otherwise userspace can call get resources
> and try and get the edid before we've even cached it.
> 
> This fixes some problems when hotplugging mst monitors,
> with X/mutter running.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c  |  1 +
>  drivers/gpu/drm/i915/intel_dp_mst.c    |  9 ++++++++-
>  drivers/gpu/drm/radeon/radeon_dp_mst.c | 10 +++++++++-
>  include/drm/drm_dp_mst_helper.h        |  1 +
>  4 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f6acb57..1b0ca1f 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1129,6 +1129,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>  		if (port->port_num >= 8) {
>  			port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc);

We're not updating the tile prop here like in drm_dp_mst_get_edid, maybe
do that here and then simplify drm_dp_mst_get_edid to just do a
drm_edid_duplicate and nothing else?

Also I'm a bit unclear on what this fixes for mutter - if it looks at the
tile prop that still won't be there really with this patch. The edid otoh
will be there. Slightly confused.
>  		}
> +		(*mstb->mgr->cbs->register_connector)(port->connector);
>  	}
>  
>  out:
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3e4be5a..6ade068 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -462,11 +462,17 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	drm_object_attach_property(&connector->base, dev->mode_config.tile_property, 0);
>  
>  	drm_mode_connector_set_path_property(connector, pathprop);
> +	return connector;
> +}
> +
> +static void intel_dp_register_mst_connector(struct drm_connector *connector)
> +{
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct drm_device *dev = connector->dev;
>  	drm_modeset_lock_all(dev);
>  	intel_connector_add_to_fbdev(intel_connector);
>  	drm_modeset_unlock_all(dev);
>  	drm_connector_register(&intel_connector->base);
> -	return connector;
>  }
>  
>  static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
> @@ -512,6 +518,7 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  
>  static struct drm_dp_mst_topology_cbs mst_cbs = {
>  	.add_connector = intel_dp_add_mst_connector,
> +	.register_connector = intel_dp_register_mst_connector,
>  	.destroy_connector = intel_dp_destroy_mst_connector,
>  	.hotplug = intel_dp_mst_hotplug,
>  };
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index 5e09c06..9a9193b 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -286,12 +286,19 @@ static struct drm_connector *radeon_dp_add_mst_connector(struct drm_dp_mst_topol
>  	drm_object_attach_property(&connector->base, dev->mode_config.path_property, 0);
>  	drm_mode_connector_set_path_property(connector, pathprop);
>  
> +	return connector;
> +}
> +
> +static void radeon_dp_register_mst_connector(struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct radeon_device *rdev = dev->dev_private;
> +
>  	drm_modeset_lock_all(dev);
>  	radeon_fb_add_connector(rdev, connector);
>  	drm_modeset_unlock_all(dev);

Random observation aside: If we rework the fb helpers to rescan the
connector list on every hotplug (irrespective or mst or not) we could get
rid of this heavywheight modeset_lock_all from the connector add case.
There's another one in drm_connector_init but that's fully internal (and
can be fixed easily). That would at least take care of making connector
adding sane from a locking pov, removal is still a problem on it's on.

Anyway the split itself looks correct.
-Daniel

>  
>  	drm_connector_register(connector);
> -	return connector;
>  }
>  
>  static void radeon_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
> @@ -324,6 +331,7 @@ static void radeon_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>  
>  struct drm_dp_mst_topology_cbs mst_cbs = {
>  	.add_connector = radeon_dp_add_mst_connector,
> +	.register_connector = radeon_dp_register_mst_connector,
>  	.destroy_connector = radeon_dp_destroy_mst_connector,
>  	.hotplug = radeon_dp_mst_hotplug,
>  };
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 86d0b25..0f408b0 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -374,6 +374,7 @@ struct drm_dp_mst_topology_mgr;
>  struct drm_dp_mst_topology_cbs {
>  	/* create a connector for a port */
>  	struct drm_connector *(*add_connector)(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port, const char *path);
> +	void (*register_connector)(struct drm_connector *connector);
>  	void (*destroy_connector)(struct drm_dp_mst_topology_mgr *mgr,
>  				  struct drm_connector *connector);
>  	void (*hotplug)(struct drm_dp_mst_topology_mgr *mgr);
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-09-30  7:59 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-16  7:55 [PATCH] [rfc] drm/mst: split connector registration into two parts Dave Airlie
2015-09-30  8:02 ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).