Re: [PATCH] [rfc] drm/mst: split connector registration into two parts

[Daniel Vetter <daniel@ffwll.ch>]

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