diff mbox

[18/25] drm/radeon: Use rdev->gem.mutex to protect hyperz/cmask owners

Message ID 1444894601-5200-19-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter Oct. 15, 2015, 7:36 a.m. UTC
This removes the last depency of radeon for dev->struct_mutex!

Also the locking scheme for hyperz/cmask owners seems a bit unsound,
there's no protection in the preclose handler (and that never did hold
dev->struct_mutex while being called). So grab the same lock there,
too.

There's also all the checks in the cs checker, but since the overall
design seems to never stall for the previous owner I figured it's ok
if I leave this racy. It was racy even before I touched it after all
too.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Christian König Oct. 15, 2015, 8:27 a.m. UTC | #1
On 15.10.2015 09:36, Daniel Vetter wrote:
> This removes the last depency of radeon for dev->struct_mutex!
>
> Also the locking scheme for hyperz/cmask owners seems a bit unsound,
> there's no protection in the preclose handler (and that never did hold
> dev->struct_mutex while being called). So grab the same lock there,
> too.
>
> There's also all the checks in the cs checker, but since the overall
> design seems to never stall for the previous owner I figured it's ok
> if I leave this racy. It was racy even before I touched it after all
> too.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I'm not so deep into the pre R600 stuff but this looks completely sane.

Patch is Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index efa45e502975..893cf1079552 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -181,7 +181,9 @@ static void radeon_set_filp_rights(struct drm_device *dev,
>   				   struct drm_file *applier,
>   				   uint32_t *value)
>   {
> -	mutex_lock(&dev->struct_mutex);
> +	struct radeon_device *rdev = dev->dev_private;
> +
> +	mutex_lock(&rdev->gem.mutex);
>   	if (*value == 1) {
>   		/* wants rights */
>   		if (!*owner)
> @@ -192,7 +194,7 @@ static void radeon_set_filp_rights(struct drm_device *dev,
>   			*owner = NULL;
>   	}
>   	*value = *owner == applier ? 1 : 0;
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&rdev->gem.mutex);
>   }
>   
>   /*
> @@ -727,10 +729,14 @@ void radeon_driver_preclose_kms(struct drm_device *dev,
>   				struct drm_file *file_priv)
>   {
>   	struct radeon_device *rdev = dev->dev_private;
> +
> +	mutex_lock(&rdev->gem.mutex);
>   	if (rdev->hyperz_filp == file_priv)
>   		rdev->hyperz_filp = NULL;
>   	if (rdev->cmask_filp == file_priv)
>   		rdev->cmask_filp = NULL;
> +	mutex_unlock(&rdev->gem.mutex);
> +
>   	radeon_uvd_free_handles(rdev, file_priv);
>   	radeon_vce_free_handles(rdev, file_priv);
>   }
Alex Deucher Oct. 15, 2015, 2:24 p.m. UTC | #2
On Thu, Oct 15, 2015 at 4:27 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 15.10.2015 09:36, Daniel Vetter wrote:
>>
>> This removes the last depency of radeon for dev->struct_mutex!
>>
>> Also the locking scheme for hyperz/cmask owners seems a bit unsound,
>> there's no protection in the preclose handler (and that never did hold
>> dev->struct_mutex while being called). So grab the same lock there,
>> too.
>>
>> There's also all the checks in the cs checker, but since the overall
>> design seems to never stall for the previous owner I figured it's ok
>> if I leave this racy. It was racy even before I touched it after all
>> too.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
>
> I'm not so deep into the pre R600 stuff but this looks completely sane.
>
> Patch is Reviewed-by: Christian König <christian.koenig@amd.com>

Applied to my -next branch.

Thanks,

Alex

>
> Regards,
> Christian.
>
>
>> ---
>>   drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
>> b/drivers/gpu/drm/radeon/radeon_kms.c
>> index efa45e502975..893cf1079552 100644
>> --- a/drivers/gpu/drm/radeon/radeon_kms.c
>> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
>> @@ -181,7 +181,9 @@ static void radeon_set_filp_rights(struct drm_device
>> *dev,
>>                                    struct drm_file *applier,
>>                                    uint32_t *value)
>>   {
>> -       mutex_lock(&dev->struct_mutex);
>> +       struct radeon_device *rdev = dev->dev_private;
>> +
>> +       mutex_lock(&rdev->gem.mutex);
>>         if (*value == 1) {
>>                 /* wants rights */
>>                 if (!*owner)
>> @@ -192,7 +194,7 @@ static void radeon_set_filp_rights(struct drm_device
>> *dev,
>>                         *owner = NULL;
>>         }
>>         *value = *owner == applier ? 1 : 0;
>> -       mutex_unlock(&dev->struct_mutex);
>> +       mutex_unlock(&rdev->gem.mutex);
>>   }
>>     /*
>> @@ -727,10 +729,14 @@ void radeon_driver_preclose_kms(struct drm_device
>> *dev,
>>                                 struct drm_file *file_priv)
>>   {
>>         struct radeon_device *rdev = dev->dev_private;
>> +
>> +       mutex_lock(&rdev->gem.mutex);
>>         if (rdev->hyperz_filp == file_priv)
>>                 rdev->hyperz_filp = NULL;
>>         if (rdev->cmask_filp == file_priv)
>>                 rdev->cmask_filp = NULL;
>> +       mutex_unlock(&rdev->gem.mutex);
>> +
>>         radeon_uvd_free_handles(rdev, file_priv);
>>         radeon_vce_free_handles(rdev, file_priv);
>>   }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index efa45e502975..893cf1079552 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -181,7 +181,9 @@  static void radeon_set_filp_rights(struct drm_device *dev,
 				   struct drm_file *applier,
 				   uint32_t *value)
 {
-	mutex_lock(&dev->struct_mutex);
+	struct radeon_device *rdev = dev->dev_private;
+
+	mutex_lock(&rdev->gem.mutex);
 	if (*value == 1) {
 		/* wants rights */
 		if (!*owner)
@@ -192,7 +194,7 @@  static void radeon_set_filp_rights(struct drm_device *dev,
 			*owner = NULL;
 	}
 	*value = *owner == applier ? 1 : 0;
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&rdev->gem.mutex);
 }
 
 /*
@@ -727,10 +729,14 @@  void radeon_driver_preclose_kms(struct drm_device *dev,
 				struct drm_file *file_priv)
 {
 	struct radeon_device *rdev = dev->dev_private;
+
+	mutex_lock(&rdev->gem.mutex);
 	if (rdev->hyperz_filp == file_priv)
 		rdev->hyperz_filp = NULL;
 	if (rdev->cmask_filp == file_priv)
 		rdev->cmask_filp = NULL;
+	mutex_unlock(&rdev->gem.mutex);
+
 	radeon_uvd_free_handles(rdev, file_priv);
 	radeon_vce_free_handles(rdev, file_priv);
 }