| Message ID | 20210617063018.92802-2-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915 TTM sync accelerated migration and clear | expand |
On Thu, Jun 17, 2021 at 08:30:07AM +0200, Thomas Hellström wrote: > Since the ww transaction endpoint easily end up far out-of-scope of > the objects on the ww object list, particularly for contending lock > objects, make sure we reference objects on the list so they don't > disappear under us. > > This comes with a performance penalty so it's been debated whether this > is really needed. But I think this is motivated by the fact that locking > is typically difficult to get right, and whatever we can do to make it > simpler for developers moving forward should be done, unless the > performance impact is far too high. > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reviewed-by: Matthew Auld <matthew.auld@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> I've looked the past 2-3 weeks in-depth at our execbuf code. That has definitely gone way too far into "very clevery" territory, and safe is so much better than clever. If there's a fundamental performance issue, we need to fix this in a fundamental way. E.g. for this one here a possible solution could be VM_BIND, at least in the fastpath, where we don't need to look-up any objects, nor refcount them, nor anything else (at least that's the goal). Only some per vm/request book-keeping and done. Also I think we can easily claw this back once we get to the cleanup part of this work: i915_vma_pin has a bunch of atomics (and lots of locks in slow-paths) of its own, which are largely redundant now that object state is protected by dma_resv_lock. Once that's cleaned up we can pay our atomic inc/dec here with the removed atomic ops from the vma side I think. Anyway just figured I drop some thoughts and my ack on the direction you're pushing here. -Daniel > --- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 ++++++-- > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index d66aa00d023a..241666931945 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -169,13 +169,17 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj, > else > ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL); > > - if (!ret && ww) > + if (!ret && ww) { > + i915_gem_object_get(obj); > list_add_tail(&obj->obj_link, &ww->obj_list); > + } > if (ret == -EALREADY) > ret = 0; > > - if (ret == -EDEADLK) > + if (ret == -EDEADLK) { > + i915_gem_object_get(obj); > ww->contended = obj; > + } > > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6a0a3f0e36e1..c62dcd0e341a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1222,6 +1222,7 @@ static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww) > while ((obj = list_first_entry_or_null(&ww->obj_list, struct drm_i915_gem_object, obj_link))) { > list_del(&obj->obj_link); > i915_gem_object_unlock(obj); > + i915_gem_object_put(obj); > } > } > > @@ -1229,6 +1230,7 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj) > { > list_del(&obj->obj_link); > i915_gem_object_unlock(obj); > + i915_gem_object_put(obj); > } > > void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww) > @@ -1253,6 +1255,8 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww) > > if (!ret) > list_add_tail(&ww->contended->obj_link, &ww->obj_list); > + else > + i915_gem_object_put(ww->contended); > > ww->contended = NULL; > > -- > 2.31.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index d66aa00d023a..241666931945 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -169,13 +169,17 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj, else ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL); - if (!ret && ww) + if (!ret && ww) { + i915_gem_object_get(obj); list_add_tail(&obj->obj_link, &ww->obj_list); + } if (ret == -EALREADY) ret = 0; - if (ret == -EDEADLK) + if (ret == -EDEADLK) { + i915_gem_object_get(obj); ww->contended = obj; + } return ret; } diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6a0a3f0e36e1..c62dcd0e341a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1222,6 +1222,7 @@ static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww) while ((obj = list_first_entry_or_null(&ww->obj_list, struct drm_i915_gem_object, obj_link))) { list_del(&obj->obj_link); i915_gem_object_unlock(obj); + i915_gem_object_put(obj); } } @@ -1229,6 +1230,7 @@ void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj) { list_del(&obj->obj_link); i915_gem_object_unlock(obj); + i915_gem_object_put(obj); } void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww) @@ -1253,6 +1255,8 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww) if (!ret) list_add_tail(&ww->contended->obj_link, &ww->obj_list); + else + i915_gem_object_put(ww->contended); ww->contended = NULL;