On Wed, Sep 16, 2015 at 05:55:23PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
