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@xxxxxxxxxxxxxxxxxx>
> Tested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
