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

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

[Harry Wentland]

This is no longer needed in 4.13

Signed-off-by: Harry Wentland <harry.wentland@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) {
 		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++) {
 			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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@amd.com> wrote:
> This is no longer needed in 4.13
>
> Signed-off-by: Harry Wentland <harry.wentland@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@amd.com> wrote:
>> This is no longer needed in 4.13
>>
>> Signed-off-by: Harry Wentland <harry.wentland@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
>>
> 
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx