| Message ID | 20200623142843.423594-8-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] Revert "drm/i915/gem: Async GPU relocations only" | expand |
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>
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)) >
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)) >>
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))
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(-)