[Intel-gfx] [PATCH 2/3] drm/i915: Do not call retire_requests from wait_for_rendering
Volkin, Bradley D
bradley.d.volkin at intel.com
Fri Apr 18 00:20:33 CEST 2014
Hi Chris, just want to bring this one back to your attention while
I'm going through the rest of the series.
Thanks,
Brad
On Fri, Mar 28, 2014 at 03:58:25PM -0700, Volkin, Bradley D wrote:
> On Mon, Mar 17, 2014 at 05:21:55AM -0700, Chris Wilson wrote:
> > A common issue we have is that retiring requests causes recursion
> > through GTT manipulation or page table manipulation which we can only
> > handle at very specific points. However, to maintain internal
> > consistency (enforced through our sanity checks on write_domain at
> > various points in the GEM object lifecycle) we do need to retire the
> > object prior to marking it with a new write_domain, and also clear the
> > write_domain for the implicit flush following a batch.
> >
> > Note that this then allows the unbound objects to still be on the active
> > lists, and so care must be taken when removing objects from unbound lists
> > (similar to the caveats we face processing the bound lists).
> >
> > v2: Fix i915_gem_shrink_all() to handle updated object lifetime rules,
> > by refactoring it to call into __i915_gem_shrink().
> >
> > v3: Missed an object-retire prior to changing cache domains in
> > i915_gem_object_set_cache_leve()
> >
> > v4: Rebase
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Tested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 112 ++++++++++++++++-------------
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +
> > 2 files changed, 66 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 58704ce62e3e..5cf4d80de867 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -44,6 +44,9 @@ static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *o
> > static __must_check int
> > i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
> > bool readonly);
> > +static void
> > +i915_gem_object_retire(struct drm_i915_gem_object *obj);
> > +
> > static int i915_gem_phys_pwrite(struct drm_device *dev,
> > struct drm_i915_gem_object *obj,
> > struct drm_i915_gem_pwrite *args,
> > @@ -502,6 +505,8 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
> > ret = i915_gem_object_wait_rendering(obj, true);
> > if (ret)
> > return ret;
> > +
> > + i915_gem_object_retire(obj);
> > }
> >
> > ret = i915_gem_object_get_pages(obj);
> > @@ -917,6 +922,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > ret = i915_gem_object_wait_rendering(obj, false);
> > if (ret)
> > return ret;
> > +
> > + i915_gem_object_retire(obj);
> > }
> > /* Same trick applies to invalidate partially written cachelines read
> > * before writing. */
> > @@ -1304,7 +1311,8 @@ static int
> > i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj,
> > struct intel_ring_buffer *ring)
> > {
> > - i915_gem_retire_requests_ring(ring);
> > + if (!obj->active)
> > + return 0;
> >
> > /* Manually manage the write flush as we may have not yet
> > * retired the buffer.
> > @@ -1314,7 +1322,6 @@ i915_gem_object_wait_rendering__tail(struct drm_i915_gem_object *obj,
> > * we know we have passed the last write.
> > */
> > obj->last_write_seqno = 0;
> > - obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> >
> > return 0;
> > }
> > @@ -1949,58 +1956,58 @@ static unsigned long
> > __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
> > bool purgeable_only)
> > {
> > - struct list_head still_bound_list;
> > - struct drm_i915_gem_object *obj, *next;
> > + struct list_head still_in_list;
> > + struct drm_i915_gem_object *obj;
> > unsigned long count = 0;
> >
> > - list_for_each_entry_safe(obj, next,
> > - &dev_priv->mm.unbound_list,
> > - global_list) {
> > - if ((i915_gem_object_is_purgeable(obj) || !purgeable_only) &&
> > - i915_gem_object_put_pages(obj) == 0) {
> > - count += obj->base.size >> PAGE_SHIFT;
> > - if (count >= target)
> > - return count;
> > - }
> > - }
> > -
> > /*
> > - * As we may completely rewrite the bound list whilst unbinding
> > + * As we may completely rewrite the (un)bound list whilst unbinding
> > * (due to retiring requests) we have to strictly process only
> > * one element of the list at the time, and recheck the list
> > * on every iteration.
>
> Is it still true that we could retire requests on this path? I see that
> currently we will retire requests via:
> i915_vma_unbind -> i915_gem_object_finish_gpu -> i915_gem_object_wait_rendering.
>
> But we've taken the explicit request retirement out of the wait_rendering path.
> Have I missed somewhere that it could still happen?
>
> Thanks,
> Brad
>
> > + *
> > + * In particular, we must hold a reference whilst removing the
> > + * object as we may end up waiting for and/or retiring the objects.
> > + * This might release the final reference (held by the active list)
> > + * and result in the object being freed from under us. This is
> > + * similar to the precautions the eviction code must take whilst
> > + * removing objects.
> > + *
> > + * Also note that although these lists do not hold a reference to
> > + * the object we can safely grab one here: The final object
> > + * unreferencing and the bound_list are both protected by the
> > + * dev->struct_mutex and so we won't ever be able to observe an
> > + * object on the bound_list with a reference count equals 0.
> > */
> > - INIT_LIST_HEAD(&still_bound_list);
> > + INIT_LIST_HEAD(&still_in_list);
> > + while (count < target && !list_empty(&dev_priv->mm.unbound_list)) {
> > + obj = list_first_entry(&dev_priv->mm.unbound_list,
> > + typeof(*obj), global_list);
> > + list_move_tail(&obj->global_list, &still_in_list);
> > +
> > + if (!i915_gem_object_is_purgeable(obj) && purgeable_only)
> > + continue;
> > +
> > + drm_gem_object_reference(&obj->base);
> > +
> > + if (i915_gem_object_put_pages(obj) == 0)
> > + count += obj->base.size >> PAGE_SHIFT;
> > +
> > + drm_gem_object_unreference(&obj->base);
> > + }
> > + list_splice(&still_in_list, &dev_priv->mm.unbound_list);
> > +
> > + INIT_LIST_HEAD(&still_in_list);
> > while (count < target && !list_empty(&dev_priv->mm.bound_list)) {
> > struct i915_vma *vma, *v;
> >
> > obj = list_first_entry(&dev_priv->mm.bound_list,
> > typeof(*obj), global_list);
> > - list_move_tail(&obj->global_list, &still_bound_list);
> > + list_move_tail(&obj->global_list, &still_in_list);
> >
> > if (!i915_gem_object_is_purgeable(obj) && purgeable_only)
> > continue;
> >
> > - /*
> > - * Hold a reference whilst we unbind this object, as we may
> > - * end up waiting for and retiring requests. This might
> > - * release the final reference (held by the active list)
> > - * and result in the object being freed from under us.
> > - * in this object being freed.
> > - *
> > - * Note 1: Shrinking the bound list is special since only active
> > - * (and hence bound objects) can contain such limbo objects, so
> > - * we don't need special tricks for shrinking the unbound list.
> > - * The only other place where we have to be careful with active
> > - * objects suddenly disappearing due to retiring requests is the
> > - * eviction code.
> > - *
> > - * Note 2: Even though the bound list doesn't hold a reference
> > - * to the object we can safely grab one here: The final object
> > - * unreferencing and the bound_list are both protected by the
> > - * dev->struct_mutex and so we won't ever be able to observe an
> > - * object on the bound_list with a reference count equals 0.
> > - */
> > drm_gem_object_reference(&obj->base);
> >
> > list_for_each_entry_safe(vma, v, &obj->vma_list, vma_link)
> > @@ -2012,7 +2019,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
> >
> > drm_gem_object_unreference(&obj->base);
> > }
> > - list_splice(&still_bound_list, &dev_priv->mm.bound_list);
> > + list_splice(&still_in_list, &dev_priv->mm.bound_list);
> >
> > return count;
> > }
> > @@ -2026,17 +2033,8 @@ i915_gem_purge(struct drm_i915_private *dev_priv, long target)
> > static unsigned long
> > i915_gem_shrink_all(struct drm_i915_private *dev_priv)
> > {
> > - struct drm_i915_gem_object *obj, *next;
> > - long freed = 0;
> > -
> > i915_gem_evict_everything(dev_priv->dev);
> > -
> > - list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list,
> > - global_list) {
> > - if (i915_gem_object_put_pages(obj) == 0)
> > - freed += obj->base.size >> PAGE_SHIFT;
> > - }
> > - return freed;
> > + return __i915_gem_shrink(dev_priv, LONG_MAX, false);
> > }
> >
> > static int
> > @@ -2265,6 +2263,19 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> > WARN_ON(i915_verify_lists(dev));
> > }
> >
> > +static void
> > +i915_gem_object_retire(struct drm_i915_gem_object *obj)
> > +{
> > + struct intel_ring_buffer *ring = obj->ring;
> > +
> > + if (ring == NULL)
> > + return;
> > +
> > + if (i915_seqno_passed(ring->get_seqno(ring, true),
> > + obj->last_read_seqno))
> > + i915_gem_object_move_to_inactive(obj);
> > +}
> > +
> > static int
> > i915_gem_init_seqno(struct drm_device *dev, u32 seqno)
> > {
> > @@ -3618,6 +3629,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > if (ret)
> > return ret;
> >
> > + i915_gem_object_retire(obj);
> > i915_gem_object_flush_cpu_write_domain(obj, false);
> >
> > /* Serialise direct access to this object with the barriers for
> > @@ -3716,6 +3728,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > * in obj->write_domain and have been skipping the clflushes.
> > * Just set it to the CPU cache for now.
> > */
> > + i915_gem_object_retire(obj);
> > WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> >
> > old_read_domains = obj->base.read_domains;
> > @@ -3938,6 +3951,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
> > if (ret)
> > return ret;
> >
> > + i915_gem_object_retire(obj);
> > i915_gem_object_flush_gtt_write_domain(obj);
> >
> > old_write_domain = obj->base.write_domain;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 3851a1b1dc88..6ec5d1d5c625 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -955,6 +955,9 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> > if (i915_gem_obj_ggtt_bound(obj) &&
> > i915_gem_obj_to_ggtt(obj)->pin_count)
> > intel_mark_fb_busy(obj, ring);
> > +
> > + /* update for the implicit flush after a batch */
> > + obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> > }
> >
> > trace_i915_gem_object_change_domain(obj, old_read, old_write);
> > --
> > 1.9.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list