Quoting Matthew Auld (2019-06-17 14:43:34)
> On 17/06/2019 08:19, Chris Wilson wrote:
> > Remove the accumulated optimisations that we have for i915_vma_retire
> > and reduce it to the bare essential of tracking the active object
> > reference. This allows us to only use atomic operations, and so will be
> > able to avoid the struct_mutex requirement.
> >
> > The principal loss here is the shrinker MRU bumping, so now if we have
> > to shrink, we will do so in much more random order and more likely to
> > try and shrink recently used objects. That is a nuisance, but shrinking
> > active objects is a second step we try to avoid and will always be a
> > system-wide performance issue.
> >
> > The other loss is here is in the automatic pruning of the
> > reservation_object when idling. This is not as large an issue as upon
> > reservation_object introduction as now adding new fences into the object
> > replaces already signaled fences, keeping the array compact. But we do
> > lose the auto-expiration of stale fences and unused arrays. That may be
> > a noticeable problem for which we need to re-implement autopruning.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 1 -
> > drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ---
> > .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 -
> > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 5 +-
> > .../drm/i915/gem/selftests/i915_gem_mman.c | 9 ----
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +-
> > drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 1 -
> > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 32 +++++------
> > drivers/gpu/drm/i915/i915_debugfs.c | 8 +--
> > drivers/gpu/drm/i915/i915_gem_batch_pool.c | 42 ++++++---------
> > drivers/gpu/drm/i915/i915_vma.c | 54 ++++---------------
> > 11 files changed, 47 insertions(+), 116 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index bb5b6e63a2cc..252e752da211 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -162,7 +162,6 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> >
> > mutex_lock(&i915->drm.struct_mutex);
> >
> > - GEM_BUG_ON(i915_gem_object_is_active(obj));
> > list_for_each_entry_safe(vma, vn, &obj->vma.list, obj_link) {
> > GEM_BUG_ON(i915_vma_is_active(vma));
> > vma->flags &= ~I915_VMA_PIN_MASK;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > index 7cb1871d7128..454bfb498001 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > @@ -158,12 +158,6 @@ i915_gem_object_needs_async_cancel(const struct drm_i915_gem_object *obj)
> > return obj->ops->flags & I915_GEM_OBJECT_ASYNC_CANCEL;
> > }
> >
> > -static inline bool
> > -i915_gem_object_is_active(const struct drm_i915_gem_object *obj)
> > -{
> > - return READ_ONCE(obj->active_count);
> > -}
> > -
> > static inline bool
> > i915_gem_object_is_framebuffer(const struct drm_i915_gem_object *obj)
> > {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > index 5b05698619ce..c299fed2c6b1 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > @@ -156,7 +156,6 @@ struct drm_i915_gem_object {
> >
> > /** Count of VMA actually bound by this object */
> > atomic_t bind_count;
> > - unsigned int active_count;
> > /** Count of how many global VMA are currently pinned for use by HW */
> > unsigned int pin_global;
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > index 3a926a8755c6..f4677f70cce7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> > @@ -230,8 +230,9 @@ i915_gem_shrink(struct drm_i915_private *i915,
> > continue;
> >
> > if (!(shrink & I915_SHRINK_ACTIVE) &&
> > - (i915_gem_object_is_active(obj) ||
> > - i915_gem_object_is_framebuffer(obj)))
> > + (i915_gem_object_is_framebuffer(obj) ||
> > + reservation_object_test_signaled_rcu(obj->resv,
> > + true)))
>
> Wait, isn't it the other way around, so
> !reservation_object_test_signaled_rcu() ?
If you insist ;)
>
> > continue;
> >
> > if (!(shrink & I915_SHRINK_BOUND) &&
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > index 5c81f4b4813a..2053194a8b70 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > @@ -474,15 +474,6 @@ static int igt_mmap_offset_exhaustion(void *arg)
> > pr_err("[loop %d] Failed to busy the object\n", loop);
> > goto err_obj;
> > }
> > -
> > - /* NB we rely on the _active_ reference to access obj now */
> > - GEM_BUG_ON(!i915_gem_object_is_active(obj));
> > - err = create_mmap_offset(obj);
> > - if (err) {
> > - pr_err("[loop %d] create_mmap_offset failed with err=%d\n",
> > - loop, err);
> > - goto out;
> > - }
>
> Do we really want to drop the create_mmap_offset?
The first phase we test with explicitly holding an object reference.
This second phase we are testing with the implicit active reference,
which will no longer be serialised by struct_mutex and so we cannot
simply prevent being dropped in the background. So this particular check
becomes a time bomb.
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index bbbdc63906c6..cd4cf4d0b30c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1509,9 +1509,7 @@ static void execlists_submit_request(struct i915_request *request)
> > static void __execlists_context_fini(struct intel_context *ce)
> > {
> > intel_ring_put(ce->ring);
> > -
> > - GEM_BUG_ON(i915_gem_object_is_active(ce->state->obj));
> > - i915_gem_object_put(ce->state->obj);
> > + i915_vma_put(ce->state);
>
> I guess vma_put atm is still just an alias...though this hunk seems a
> little misplaced for this patch? Not sure.
The only reason we didn't use i915_vma_put here was because we had the
active check on the object. Without that, we can reuse the same pattern
in place elsewhere. Maybe it's foreshadowing a later change ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
