[PATCH] drm/amd/display: Remove drm_modeset_lock in MST code

[Daniel Vetter]

On Fri, Sep 1, 2017 at 8:50 PM, Harry Wentland <harry.wentland at amd.com> wrote:
> This is no longer needed in 4.13
>
> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 3ce087f4e0ef..e41bb0bb0d66 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -241,7 +241,6 @@ static struct drm_connector *dm_dp_add_mst_connector(struct drm_dp_mst_topology_
>         struct amdgpu_connector *aconnector;
>         struct drm_connector *connector;
>
> -       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

You need the fancy new connector_list iterators here, otherwise this
isn't safe. You probably want to add this as another audit-item to
your TODO.
-Daniel

>                 aconnector = to_amdgpu_connector(connector);
>                 if (aconnector->mst_port == master
> @@ -252,11 +251,9 @@ static struct drm_connector *dm_dp_add_mst_connector(struct drm_dp_mst_topology_
>                         aconnector->port = port;
>                         drm_mode_connector_set_path_property(connector, pathprop);
>
> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>                         return &aconnector->base;
>                 }
>         }
> -       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>
>         aconnector = kzalloc(sizeof(*aconnector), GFP_KERNEL);
>         if (!aconnector)
> @@ -349,7 +346,6 @@ static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>         struct edid *edid;
>         struct dc_sink *dc_sink;
>
> -       drm_modeset_lock_all(dev);
>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>                 aconnector = to_amdgpu_connector(connector);
>                 if (aconnector->port &&
> @@ -400,7 +396,6 @@ static void dm_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr)
>                                 aconnector->edid);
>                 }
>         }
> -       drm_modeset_unlock_all(dev);
>
>         schedule_work(&adev->dm.mst_hotplug_work);
>  }
> @@ -411,15 +406,12 @@ static void dm_dp_mst_register_connector(struct drm_connector *connector)
>         struct amdgpu_device *adev = dev->dev_private;
>         int i;
>
> -       drm_modeset_lock_all(dev);
>         if (adev->mode_info.rfbdev) {
>                 /*Do not add if already registered in past*/
>                 for (i = 0; i < adev->mode_info.rfbdev->helper.connector_count; i++) {

Don't digg around in helper internals. Especially not since you're not
holding the right locks.

Just from what I've spotted in your context here this all smells
mildly fishy, but I'm too lazy to digg out the latest dc code from
Alex' tree.

If you do it like radeon then it /should/ be ok, no idea why this
stuff here crept in.
-Daniel

>                         if (adev->mode_info.rfbdev->helper.connector_info[i]->connector
> -                                       == connector) {
> -                               drm_modeset_unlock_all(dev);
> +                                       == connector)
>                                 return;
> -                       }
>                 }
>
>                 drm_fb_helper_add_one_connector(&adev->mode_info.rfbdev->helper, connector);
> @@ -427,8 +419,6 @@ static void dm_dp_mst_register_connector(struct drm_connector *connector)
>         else
>                 DRM_ERROR("adev->mode_info.rfbdev is NULL\n");
>
> -       drm_modeset_unlock_all(dev);
> -
>         drm_connector_register(connector);
>
>  }
> --
> 2.11.0
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch