Re: [PATCH 13/29] drm/i915: clear domains for all objects on reset

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Aug 03, 2013 at 03:24:47PM -0700, Ben Widawsky wrote:
> On Sat, Aug 03, 2013 at 11:59:42AM +0100, Chris Wilson wrote:
> > On Wed, Jul 31, 2013 at 05:00:06PM -0700, Ben Widawsky wrote:
> > > Simply iterating over 1 inactive list is insufficient for the way we now
> > > track inactive (1 list per address space). We could alternatively do
> > > this with bound + unbound lists, and an inactive check. To me, this way
> > > is a bit easier to understand.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index b4c35f0..8ce3545 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -2282,7 +2282,7 @@ void i915_gem_restore_fences(struct drm_device *dev)
> > >  void i915_gem_reset(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	struct i915_address_space *vm = &dev_priv->gtt.base;
> > > +	struct i915_address_space *vm;
> > >  	struct drm_i915_gem_object *obj;
> > >  	struct intel_ring_buffer *ring;
> > >  	int i;
> > > @@ -2293,8 +2293,9 @@ void i915_gem_reset(struct drm_device *dev)
> > >  	/* Move everything out of the GPU domains to ensure we do any
> > >  	 * necessary invalidation upon reuse.
> > >  	 */
> > > -	list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > > -		obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
> > > +	list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > > +		list_for_each_entry(obj, &vm->inactive_list, mm_list)
> > > +			obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
> > 
> > This code is dead. Just remove it rather than port it to vma.
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> 
> Got it, and moved to the front of the series.
> 
> commit 8472f08863da69159aa0a7555836ca0511754877
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Sat Aug 3 15:22:17 2013 -0700
> 
>     drm/i915: eliminate dead domain clearing on reset
>     
>     The code itself is no longer accurate without updating once we have
>     multiple address space since clearing the domains of every object
>     requires scanning the inactive list for all VMs.
>     
>     "This code is dead. Just remove it rather than port it to vma." - Chris
>     Wilson
>     
>     Recommended-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

That's not a properly formatted patch, so I've stopped merging for now.

/me loves cheap excuses

But overall I _really_ like what the series looks now, I can dwell in the
cozy feeling that I actually understand what's going on. So if the name of
the game is to keep your maintainer happy I think the goal is unlocked ;-)

Cheers, Daniel

> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3a5d4ba..c7e3cee 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2277,12 +2277,6 @@ void i915_gem_reset(struct drm_device *dev)
>         for_each_ring(ring, dev_priv, i)
>                 i915_gem_reset_ring_lists(dev_priv, ring);
>  
> -       /* Move everything out of the GPU domains to ensure we do any
> -        * necessary invalidation upon reuse.
> -        */
> -       list_for_each_entry(obj, &vm->inactive_list, mm_list)
> -               obj->base.read_domains &= ~I915_GEM_GPU_DOMAINS;
> -
>         i915_gem_restore_fences(dev);
>  }
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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