[PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

Move amdgpu_bo_unreserve() outside of the switch. While we are
at it, add a missing break in the default case.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 1dc59aafec71..ae4658a10e2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 		info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
 		info.domains = robj->prefered_domains;
 		info.domain_flags = robj->flags;
-		amdgpu_bo_unreserve(robj);
 		if (copy_to_user(out, &info, sizeof(info)))
 			r = -EFAULT;
 		break;
@@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 	case AMDGPU_GEM_OP_SET_PLACEMENT:
 		if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
 			r = -EPERM;
-			amdgpu_bo_unreserve(robj);
 			break;
 		}
 		robj->prefered_domains = args->value & (AMDGPU_GEM_DOMAIN_VRAM |
@@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 		robj->allowed_domains = robj->prefered_domains;
 		if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
 			robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
-
-		amdgpu_bo_unreserve(robj);
 		break;
 	default:
-		amdgpu_bo_unreserve(robj);
 		r = -EINVAL;
+		break;
 	}
 
+	amdgpu_bo_unreserve(robj);
 out:
 	drm_gem_object_unreference_unlocked(gobj);
 	return r;
-- 
2.11.1

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

[PATCH 2/2] drm/amdgpu: use amdgpu_gem_va_check() in amdgpu_gem_va_update_vm()

[Samuel Pitoiset]

This removes code duplication.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ae4658a10e2c..76569e31bfd8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -506,13 +506,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	list_for_each_entry(entry, list, head) {
 		struct amdgpu_bo *bo =
 			container_of(entry->bo, struct amdgpu_bo, tbo);
-
-		/* if anything is swapped out don't swap it in here,
-		   just abort and wait for the next CS */
-		if (!amdgpu_bo_gpu_accessible(bo))
-			goto error;
-
-		if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
+		if (amdgpu_gem_va_check(NULL, bo))
 			goto error;
 	}
 
-- 
2.11.1

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

Re: [PATCH 2/2] drm/amdgpu: use amdgpu_gem_va_check() in amdgpu_gem_va_update_vm()

[zhoucm1]

On 2017年02月10日 06:28, Samuel Pitoiset wrote:
> This removes code duplication.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>

Thanks,
David Zhou
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ae4658a10e2c..76569e31bfd8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -506,13 +506,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>   	list_for_each_entry(entry, list, head) {
>   		struct amdgpu_bo *bo =
>   			container_of(entry->bo, struct amdgpu_bo, tbo);
> -
> -		/* if anything is swapped out don't swap it in here,
> -		   just abort and wait for the next CS */
> -		if (!amdgpu_bo_gpu_accessible(bo))
> -			goto error;
> -
> -		if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
> +		if (amdgpu_gem_va_check(NULL, bo))
>   			goto error;
>   	}
>   

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

Re: [PATCH 2/2] drm/amdgpu: use amdgpu_gem_va_check() in amdgpu_gem_va_update_vm()

[Christian König]

Am 09.02.2017 um 23:28 schrieb Samuel Pitoiset:
> This removes code duplication.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

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

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ae4658a10e2c..76569e31bfd8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -506,13 +506,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>   	list_for_each_entry(entry, list, head) {
>   		struct amdgpu_bo *bo =
>   			container_of(entry->bo, struct amdgpu_bo, tbo);
> -
> -		/* if anything is swapped out don't swap it in here,
> -		   just abort and wait for the next CS */
> -		if (!amdgpu_bo_gpu_accessible(bo))
> -			goto error;
> -
> -		if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
> +		if (amdgpu_gem_va_check(NULL, bo))
>   			goto error;
>   	}
>   


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

Re: [PATCH 2/2] drm/amdgpu: use amdgpu_gem_va_check() in amdgpu_gem_va_update_vm()

[Alex Deucher]

On Thu, Feb 9, 2017 at 5:28 PM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
> This removes code duplication.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

Applied.  thanks!

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ae4658a10e2c..76569e31bfd8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -506,13 +506,7 @@ static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>         list_for_each_entry(entry, list, head) {
>                 struct amdgpu_bo *bo =
>                         container_of(entry->bo, struct amdgpu_bo, tbo);
> -
> -               /* if anything is swapped out don't swap it in here,
> -                  just abort and wait for the next CS */
> -               if (!amdgpu_bo_gpu_accessible(bo))
> -                       goto error;
> -
> -               if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
> +               if (amdgpu_gem_va_check(NULL, bo))
>                         goto error;
>         }
>
> --
> 2.11.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[zhoucm1]

On 2017年02月10日 06:28, Samuel Pitoiset wrote:
> Move amdgpu_bo_unreserve() outside of the switch. While we are
> at it, add a missing break in the default case.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 1dc59aafec71..ae4658a10e2c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   		info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>   		info.domains = robj->prefered_domains;
>   		info.domain_flags = robj->flags;
> -		amdgpu_bo_unreserve(robj);
>   		if (copy_to_user(out, &info, sizeof(info)))
>   			r = -EFAULT;
NAK, your this change will break our previous deadlock fix for ww_mutex 
and mm->mmap_sem if I remember correctly.

Regards,
David Zhou
>   		break;
> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   	case AMDGPU_GEM_OP_SET_PLACEMENT:
>   		if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>   			r = -EPERM;
> -			amdgpu_bo_unreserve(robj);
>   			break;
>   		}
>   		robj->prefered_domains = args->value & (AMDGPU_GEM_DOMAIN_VRAM |
> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   		robj->allowed_domains = robj->prefered_domains;
>   		if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>   			robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
> -
> -		amdgpu_bo_unreserve(robj);
>   		break;
>   	default:
> -		amdgpu_bo_unreserve(robj);
>   		r = -EINVAL;
> +		break;
>   	}
>   
> +	amdgpu_bo_unreserve(robj);
>   out:
>   	drm_gem_object_unreference_unlocked(gobj);
>   	return r;

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

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

On 02/10/2017 03:55 AM, zhoucm1 wrote:
>
>
> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>> at it, add a missing break in the default case.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 1dc59aafec71..ae4658a10e2c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>           info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>>           info.domains = robj->prefered_domains;
>>           info.domain_flags = robj->flags;
>> -        amdgpu_bo_unreserve(robj);
>>           if (copy_to_user(out, &info, sizeof(info)))
>>               r = -EFAULT;
> NAK, your this change will break our previous deadlock fix for ww_mutex
> and mm->mmap_sem if I remember correctly.

Mmh, really? Can you pinpoint the commit? I don't see anything obvious 
in the history about that.

Thanks.

>
> Regards,
> David Zhou
>>           break;
>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>               r = -EPERM;
>> -            amdgpu_bo_unreserve(robj);
>>               break;
>>           }
>>           robj->prefered_domains = args->value &
>> (AMDGPU_GEM_DOMAIN_VRAM |
>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>           robj->allowed_domains = robj->prefered_domains;
>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>> -
>> -        amdgpu_bo_unreserve(robj);
>>           break;
>>       default:
>> -        amdgpu_bo_unreserve(robj);
>>           r = -EINVAL;
>> +        break;
>>       }
>>   +    amdgpu_bo_unreserve(robj);
>>   out:
>>       drm_gem_object_unreference_unlocked(gobj);
>>       return r;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Christian König]

Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>
>
> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>
>>
>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>> at it, add a missing break in the default case.
>>>
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 1dc59aafec71..ae4658a10e2c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>> void *data,
>>>           info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>>>           info.domains = robj->prefered_domains;
>>>           info.domain_flags = robj->flags;
>>> -        amdgpu_bo_unreserve(robj);
>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>               r = -EFAULT;
>> NAK, your this change will break our previous deadlock fix for ww_mutex
>> and mm->mmap_sem if I remember correctly.
>
> Mmh, really? Can you pinpoint the commit? I don't see anything obvious 
> in the history about that.

David is right here. Not sure when we fixed that, but in general calling 
copy_to/from_user while a BO is reserved is illegal.

Otherwise somebody could send the kernel a memory mapped BO as address 
and the copy_to/from_user would just deadlock because it tries to 
reserve a BO while another (or the same) BO is already reserved in the 
call path.

Regards,
Christian.

>
> Thanks.
>
>>
>> Regards,
>> David Zhou
>>>           break;
>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>> void *data,
>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>               r = -EPERM;
>>> -            amdgpu_bo_unreserve(robj);
>>>               break;
>>>           }
>>>           robj->prefered_domains = args->value &
>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>> void *data,
>>>           robj->allowed_domains = robj->prefered_domains;
>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>> -
>>> -        amdgpu_bo_unreserve(robj);
>>>           break;
>>>       default:
>>> -        amdgpu_bo_unreserve(robj);
>>>           r = -EINVAL;
>>> +        break;
>>>       }
>>>   +    amdgpu_bo_unreserve(robj);
>>>   out:
>>>       drm_gem_object_unreference_unlocked(gobj);
>>>       return r;
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

On 02/10/2017 11:19 AM, Christian König wrote:
> Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>>
>>
>> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>>
>>>
>>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>>> at it, add a missing break in the default case.
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 1dc59aafec71..ae4658a10e2c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>           info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>>>>           info.domains = robj->prefered_domains;
>>>>           info.domain_flags = robj->flags;
>>>> -        amdgpu_bo_unreserve(robj);
>>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>>               r = -EFAULT;
>>> NAK, your this change will break our previous deadlock fix for ww_mutex
>>> and mm->mmap_sem if I remember correctly.
>>
>> Mmh, really? Can you pinpoint the commit? I don't see anything obvious
>> in the history about that.
>
> David is right here. Not sure when we fixed that, but in general calling
> copy_to/from_user while a BO is reserved is illegal.
>
> Otherwise somebody could send the kernel a memory mapped BO as address
> and the copy_to/from_user would just deadlock because it tries to
> reserve a BO while another (or the same) BO is already reserved in the
> call path.

Okay, makes more sense.

Thanks for the review. My goal is not to introduce new regressions but I 
didn't know that.

Maybe this should be explained in the code? Just a thought.

>
> Regards,
> Christian.
>
>>
>> Thanks.
>>
>>>
>>> Regards,
>>> David Zhou
>>>>           break;
>>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>>               r = -EPERM;
>>>> -            amdgpu_bo_unreserve(robj);
>>>>               break;
>>>>           }
>>>>           robj->prefered_domains = args->value &
>>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>           robj->allowed_domains = robj->prefered_domains;
>>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>> -
>>>> -        amdgpu_bo_unreserve(robj);
>>>>           break;
>>>>       default:
>>>> -        amdgpu_bo_unreserve(robj);
>>>>           r = -EINVAL;
>>>> +        break;
>>>>       }
>>>>   +    amdgpu_bo_unreserve(robj);
>>>>   out:
>>>>       drm_gem_object_unreference_unlocked(gobj);
>>>>       return r;
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Christian König]

Am 10.02.2017 um 11:22 schrieb Samuel Pitoiset:
>
>
> On 02/10/2017 11:19 AM, Christian König wrote:
>> Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>>>
>>>
>>> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>>>
>>>>
>>>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>>>> at it, add a missing break in the default case.
>>>>>
>>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 1dc59aafec71..ae4658a10e2c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>           info.alignment = robj->tbo.mem.page_alignment << 
>>>>> PAGE_SHIFT;
>>>>>           info.domains = robj->prefered_domains;
>>>>>           info.domain_flags = robj->flags;
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>>>               r = -EFAULT;
>>>> NAK, your this change will break our previous deadlock fix for 
>>>> ww_mutex
>>>> and mm->mmap_sem if I remember correctly.
>>>
>>> Mmh, really? Can you pinpoint the commit? I don't see anything obvious
>>> in the history about that.
>>
>> David is right here. Not sure when we fixed that, but in general calling
>> copy_to/from_user while a BO is reserved is illegal.
>>
>> Otherwise somebody could send the kernel a memory mapped BO as address
>> and the copy_to/from_user would just deadlock because it tries to
>> reserve a BO while another (or the same) BO is already reserved in the
>> call path.
>
> Okay, makes more sense.
>
> Thanks for the review. My goal is not to introduce new regressions but 
> I didn't know that.
>
> Maybe this should be explained in the code? Just a thought.

We do have a comment in the TTM mapping code I think.

But copy_to/from_user and BO reservation is just so common that we would 
need to add the same comment on a whole bunch of different places and 
that doesn't make to much sense.

But in this particular case a comment might make sense because I think 
somebody proposed the same patch before. Ah! Enlightenment, that also 
explains why you can't find it in the history. We just rejected the same 
patch multiple times now :)

Regards,
Christian.

>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>>           break;
>>>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>>>               r = -EPERM;
>>>>> -            amdgpu_bo_unreserve(robj);
>>>>>               break;
>>>>>           }
>>>>>           robj->prefered_domains = args->value &
>>>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>> void *data,
>>>>>           robj->allowed_domains = robj->prefered_domains;
>>>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>> -
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           break;
>>>>>       default:
>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>           r = -EINVAL;
>>>>> +        break;
>>>>>       }
>>>>>   +    amdgpu_bo_unreserve(robj);
>>>>>   out:
>>>>>       drm_gem_object_unreference_unlocked(gobj);
>>>>>       return r;
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

On 02/10/2017 11:37 AM, Christian König wrote:
> Am 10.02.2017 um 11:22 schrieb Samuel Pitoiset:
>>
>>
>> On 02/10/2017 11:19 AM, Christian König wrote:
>>> Am 10.02.2017 um 11:11 schrieb Samuel Pitoiset:
>>>>
>>>>
>>>> On 02/10/2017 03:55 AM, zhoucm1 wrote:
>>>>>
>>>>>
>>>>> On 2017年02月10日 06:28, Samuel Pitoiset wrote:
>>>>>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>>>>>> at it, add a missing break in the default case.
>>>>>>
>>>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>>>>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> index 1dc59aafec71..ae4658a10e2c 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>           info.alignment = robj->tbo.mem.page_alignment <<
>>>>>> PAGE_SHIFT;
>>>>>>           info.domains = robj->prefered_domains;
>>>>>>           info.domain_flags = robj->flags;
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           if (copy_to_user(out, &info, sizeof(info)))
>>>>>>               r = -EFAULT;
>>>>> NAK, your this change will break our previous deadlock fix for
>>>>> ww_mutex
>>>>> and mm->mmap_sem if I remember correctly.
>>>>
>>>> Mmh, really? Can you pinpoint the commit? I don't see anything obvious
>>>> in the history about that.
>>>
>>> David is right here. Not sure when we fixed that, but in general calling
>>> copy_to/from_user while a BO is reserved is illegal.
>>>
>>> Otherwise somebody could send the kernel a memory mapped BO as address
>>> and the copy_to/from_user would just deadlock because it tries to
>>> reserve a BO while another (or the same) BO is already reserved in the
>>> call path.
>>
>> Okay, makes more sense.
>>
>> Thanks for the review. My goal is not to introduce new regressions but
>> I didn't know that.
>>
>> Maybe this should be explained in the code? Just a thought.
>
> We do have a comment in the TTM mapping code I think.
>
> But copy_to/from_user and BO reservation is just so common that we would
> need to add the same comment on a whole bunch of different places and
> that doesn't make to much sense.
>
> But in this particular case a comment might make sense because I think
> somebody proposed the same patch before. Ah! Enlightenment, that also
> explains why you can't find it in the history. We just rejected the same
> patch multiple times now :)

Yeah, I bet someone else will submit the same patch in few weeks/months 
without a comment explaining why we shouldn't change it. :)

>
> Regards,
> Christian.
>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>>           break;
>>>>>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>>>>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>>>>>               r = -EPERM;
>>>>>> -            amdgpu_bo_unreserve(robj);
>>>>>>               break;
>>>>>>           }
>>>>>>           robj->prefered_domains = args->value &
>>>>>> (AMDGPU_GEM_DOMAIN_VRAM |
>>>>>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>>>>>> void *data,
>>>>>>           robj->allowed_domains = robj->prefered_domains;
>>>>>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>>>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>> -
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           break;
>>>>>>       default:
>>>>>> -        amdgpu_bo_unreserve(robj);
>>>>>>           r = -EINVAL;
>>>>>> +        break;
>>>>>>       }
>>>>>>   +    amdgpu_bo_unreserve(robj);
>>>>>>   out:
>>>>>>       drm_gem_object_unreference_unlocked(gobj);
>>>>>>       return r;
>>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Christian König]

NAK, we need to drop the reservation before calling copy_to_user().

Otherwise you run into a double locking error when copy_to_user() is 
trying to copy to the BO in question.

Regards,
Christian.

Am 09.02.2017 um 23:28 schrieb Samuel Pitoiset:
> Move amdgpu_bo_unreserve() outside of the switch. While we are
> at it, add a missing break in the default case.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 1dc59aafec71..ae4658a10e2c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   		info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>   		info.domains = robj->prefered_domains;
>   		info.domain_flags = robj->flags;
> -		amdgpu_bo_unreserve(robj);
>   		if (copy_to_user(out, &info, sizeof(info)))
>   			r = -EFAULT;
>   		break;
> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   	case AMDGPU_GEM_OP_SET_PLACEMENT:
>   		if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>   			r = -EPERM;
> -			amdgpu_bo_unreserve(robj);
>   			break;
>   		}
>   		robj->prefered_domains = args->value & (AMDGPU_GEM_DOMAIN_VRAM |
> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
>   		robj->allowed_domains = robj->prefered_domains;
>   		if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>   			robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
> -
> -		amdgpu_bo_unreserve(robj);
>   		break;
>   	default:
> -		amdgpu_bo_unreserve(robj);
>   		r = -EINVAL;
> +		break;
>   	}
>   
> +	amdgpu_bo_unreserve(robj);
>   out:
>   	drm_gem_object_unreference_unlocked(gobj);
>   	return r;


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

Re: [PATCH 1/2] drm/amdgpu: drop multiple bo_unreserve() calls in amdgpu_gem_op_ioctl()

[Samuel Pitoiset]

On 02/10/2017 11:02 AM, Christian König wrote:
> NAK, we need to drop the reservation before calling copy_to_user().
>
> Otherwise you run into a double locking error when copy_to_user() is
> trying to copy to the BO in question.

Okay, I trust you but this seems tricky. copy_to_user() doesn't even 
take the BO as parameter, how can this happen?

>
> Regards,
> Christian.
>
> Am 09.02.2017 um 23:28 schrieb Samuel Pitoiset:
>> Move amdgpu_bo_unreserve() outside of the switch. While we are
>> at it, add a missing break in the default case.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 1dc59aafec71..ae4658a10e2c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -660,7 +660,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>           info.alignment = robj->tbo.mem.page_alignment << PAGE_SHIFT;
>>           info.domains = robj->prefered_domains;
>>           info.domain_flags = robj->flags;
>> -        amdgpu_bo_unreserve(robj);
>>           if (copy_to_user(out, &info, sizeof(info)))
>>               r = -EFAULT;
>>           break;
>> @@ -668,7 +667,6 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>       case AMDGPU_GEM_OP_SET_PLACEMENT:
>>           if (amdgpu_ttm_tt_get_usermm(robj->tbo.ttm)) {
>>               r = -EPERM;
>> -            amdgpu_bo_unreserve(robj);
>>               break;
>>           }
>>           robj->prefered_domains = args->value &
>> (AMDGPU_GEM_DOMAIN_VRAM |
>> @@ -677,14 +675,13 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev,
>> void *data,
>>           robj->allowed_domains = robj->prefered_domains;
>>           if (robj->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>               robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>> -
>> -        amdgpu_bo_unreserve(robj);
>>           break;
>>       default:
>> -        amdgpu_bo_unreserve(robj);
>>           r = -EINVAL;
>> +        break;
>>       }
>>   +    amdgpu_bo_unreserve(robj);
>>   out:
>>       drm_gem_object_unreference_unlocked(gobj);
>>       return r;
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx