diff mbox

[2/3] drm/i915: Do not call retire_requests from wait_for_rendering

Message ID 1395058916-31920-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson March 17, 2014, 12:21 p.m. UTC
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@chris-wilson.co.uk>
Tested-by: Ville Syrjälä <ville.syrjala@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(-)

Comments

bradley.d.volkin@intel.com March 28, 2014, 10:58 p.m. UTC | #1
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@chris-wilson.co.uk>
> Tested-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
bradley.d.volkin@intel.com April 17, 2014, 10:20 p.m. UTC | #2
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@chris-wilson.co.uk>
> > Tested-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson April 24, 2014, 9:22 a.m. UTC | #3
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:
> > @@ -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?

Yes, as wait_rendering doesn't retire all the requests, we may still have
a request associated with the bo. This will then cause us to call
i915_gem_object_retire() during i915_gem_object_put_pages() (through
i915_gem_object_set_to_cpu_domain) thereby discard the last active
reference and destroying the object unless we take care.
-Chris
bradley.d.volkin@intel.com April 24, 2014, 4:20 p.m. UTC | #4
On Thu, Apr 24, 2014 at 02:22:39AM -0700, Chris Wilson wrote:
> 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:
> > > @@ -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?
> 
> Yes, as wait_rendering doesn't retire all the requests, we may still have
> a request associated with the bo. This will then cause us to call
> i915_gem_object_retire() during i915_gem_object_put_pages() (through
> i915_gem_object_set_to_cpu_domain) thereby discard the last active
> reference and destroying the object unless we take care.

Ok, I see it now. Thanks. This one is
Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Daniel Vetter April 25, 2014, 2:18 p.m. UTC | #5
On Thu, Apr 24, 2014 at 09:20:35AM -0700, Volkin, Bradley D wrote:
> On Thu, Apr 24, 2014 at 02:22:39AM -0700, Chris Wilson wrote:
> > 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:
> > > > @@ -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?
> > 
> > Yes, as wait_rendering doesn't retire all the requests, we may still have
> > a request associated with the bo. This will then cause us to call
> > i915_gem_object_retire() during i915_gem_object_put_pages() (through
> > i915_gem_object_set_to_cpu_domain) thereby discard the last active
> > reference and destroying the object unless we take care.
> 
> Ok, I see it now. Thanks. This one is
> Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>

Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

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.
+	 *
+	 * 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);