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

[Harry Wentland]

On 2017-09-01 05:40 PM, Daniel Vetter wrote:
> 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

Makes sense. Gonna post a v2.

> 
>>                 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
> 

This was added after porting the radeon code and is supposed to solve
some issue when going headless. I'm gonna take it out and see if I can
find someone to test it thoroughly.

Andrey, do you remember what caused things to go haywire here? Headless
in fbcon mode?

Harry

>>                         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
>>
> 
> 
>