[Intel-gfx] [PATCH 2/3] drm/i915: Do not call retire_requests from wait_for_rendering

[Daniel Vetter]

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 at intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch