[08/26] drm/i915/gem: Make eb_add_lut interruptible wait on object lock.
diff mbox series

Message ID 20200623142843.423594-8-maarten.lankhorst@linux.intel.com
State New
Headers show
Series
  • [01/26] Revert "drm/i915/gem: Async GPU relocations only"
Related show

Commit Message

Maarten Lankhorst June 23, 2020, 2:28 p.m. UTC
The lock here should be interruptible, so we can backoff if needed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Thomas Hellström (Intel) June 26, 2020, 1:52 p.m. UTC | #1
On 6/23/20 4:28 PM, Maarten Lankhorst wrote:
> The lock here should be interruptible, so we can backoff if needed.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 2636a130fb57..aa441af81431 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -774,7 +774,12 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>   		if (err == 0) { /* And nor has this handle */
>   			struct drm_i915_gem_object *obj = vma->obj;
>   
> -			i915_gem_object_lock(obj, NULL);
> +			err = i915_gem_object_lock_interruptible(obj, NULL);
> +			if (err) {
> +				radix_tree_delete(&ctx->handles_vma, handle);
> +				goto unlock;
> +			}
> +
>   			if (idr_find(&eb->file->object_idr, handle) == obj) {
>   				list_add(&lut->obj_link, &obj->lut_list);
>   			} else {
> @@ -783,6 +788,7 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>   			}
>   			i915_gem_object_unlock(obj);
>   		}
> +unlock:
>   		mutex_unlock(&ctx->mutex);
>   	}
>   	if (unlikely(err))
Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>
Tvrtko Ursulin June 29, 2020, 3:14 p.m. UTC | #2
On 23/06/2020 15:28, Maarten Lankhorst wrote:
> The lock here should be interruptible, so we can backoff if needed.

I spied Chris posting "drm/i915/gem: Move obj->lut_list under its own 
lock" so maybe have a look at that.

My question here is..

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 2636a130fb57..aa441af81431 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -774,7 +774,12 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>   		if (err == 0) { /* And nor has this handle */
>   			struct drm_i915_gem_object *obj = vma->obj;
>   
> -			i915_gem_object_lock(obj, NULL);
> +			err = i915_gem_object_lock_interruptible(obj, NULL);

.. does this lock-unlock survive to the end of your series or gets 
completely subsumed by the ctx locking?

Regards,

Tvrtko

> +			if (err) {
> +				radix_tree_delete(&ctx->handles_vma, handle);
> +				goto unlock;
> +			}
> +
>   			if (idr_find(&eb->file->object_idr, handle) == obj) {
>   				list_add(&lut->obj_link, &obj->lut_list);
>   			} else {
> @@ -783,6 +788,7 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>   			}
>   			i915_gem_object_unlock(obj);
>   		}
> +unlock:
>   		mutex_unlock(&ctx->mutex);
>   	}
>   	if (unlikely(err))
>
Maarten Lankhorst June 30, 2020, 11:56 a.m. UTC | #3
Op 29-06-2020 om 17:14 schreef Tvrtko Ursulin:
>
> On 23/06/2020 15:28, Maarten Lankhorst wrote:
>> The lock here should be interruptible, so we can backoff if needed.
>
> I spied Chris posting "drm/i915/gem: Move obj->lut_list under its own lock" so maybe have a look at that.
>
> My question here is..
>
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 2636a130fb57..aa441af81431 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -774,7 +774,12 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>>           if (err == 0) { /* And nor has this handle */
>>               struct drm_i915_gem_object *obj = vma->obj;
>>   -            i915_gem_object_lock(obj, NULL);
>> +            err = i915_gem_object_lock_interruptible(obj, NULL);
>
> .. does this lock-unlock survive to the end of your series or gets completely subsumed by the ctx locking?
>
> Regards,
>
> Tvrtko
>
Yeah it survives, it's too early to use ww waiting. Separate lut lock is fine as well as re-using ww is a bit overkill.
>> +            if (err) {
>> +                radix_tree_delete(&ctx->handles_vma, handle);
>> +                goto unlock;
>> +            }
>> +
>>               if (idr_find(&eb->file->object_idr, handle) == obj) {
>>                   list_add(&lut->obj_link, &obj->lut_list);
>>               } else {
>> @@ -783,6 +788,7 @@ static int __eb_add_lut(struct i915_execbuffer *eb,
>>               }
>>               i915_gem_object_unlock(obj);
>>           }
>> +unlock:
>>           mutex_unlock(&ctx->mutex);
>>       }
>>       if (unlikely(err))
>>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 2636a130fb57..aa441af81431 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -774,7 +774,12 @@  static int __eb_add_lut(struct i915_execbuffer *eb,
 		if (err == 0) { /* And nor has this handle */
 			struct drm_i915_gem_object *obj = vma->obj;
 
-			i915_gem_object_lock(obj, NULL);
+			err = i915_gem_object_lock_interruptible(obj, NULL);
+			if (err) {
+				radix_tree_delete(&ctx->handles_vma, handle);
+				goto unlock;
+			}
+
 			if (idr_find(&eb->file->object_idr, handle) == obj) {
 				list_add(&lut->obj_link, &obj->lut_list);
 			} else {
@@ -783,6 +788,7 @@  static int __eb_add_lut(struct i915_execbuffer *eb,
 			}
 			i915_gem_object_unlock(obj);
 		}
+unlock:
 		mutex_unlock(&ctx->mutex);
 	}
 	if (unlikely(err))