Re: [PATCH 11/11] drm/i915: Move active to vma

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Jul 09, 2013 at 09:45:09AM +0200, Daniel Vetter wrote:
> On Mon, Jul 08, 2013 at 11:08:42PM -0700, Ben Widawsky wrote:
> > Probably need to squash whole thing, or just the inactive part, tbd...
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I agree that we need vma->active, but I'm not sold on removing
> obj->active. Atm we have to use-cases for checking obj->active:
> - In the evict/unbind code to check whether the gpu is still using this
>   specific mapping. This use-case nicely fits into checking vma->active.
> - In the shrinker code and everywhere we want to do cpu access we only
>   care about whether the gpu is accessing the object, not at all through
>   which mapping precisely. There a vma-independant obj->active sounds much
>   saner.
> 
> Note though that just keeping track of vma->active isn't too useful, since
> if some other vma is keeping the object busy we'll still stall on that one
> for eviction. So we'd need a vma->ring and vma->last_rendering_seqno, too.
> 
> At that point I wonder a bit whether all this complexity is worth it ...
> 
> I need to ponder this some more.
> -Daniel

I think eventually the complexity might prove worthwhile, it might not.

In the meanwhile, I see vma->active as just a bookkeeping thing, and not
really useful in determining what we actually care about. As you mention
obj->active is really what we care about, and I used the getter
i915_gem_object_is_active() as a way to avoid the confusion of having
two active members.

I think we're in the same state of mind on this, and I've picked what I
consider to be a less offensive solution which is easy to clean up
later.

Let me know when you make a decision.

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 14 ++++++------
> >  drivers/gpu/drm/i915/i915_gem.c | 47 ++++++++++++++++++++++++-----------------
> >  2 files changed, 35 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 38d07f2..e6694ae 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -541,6 +541,13 @@ struct i915_vma {
> >  	struct drm_i915_gem_object *obj;
> >  	struct i915_address_space *vm;
> >  
> > +	/**
> > +	 * This is set if the object is on the active lists (has pending
> > +	 * rendering and so a non-zero seqno), and is not set if it i s on
> > +	 * inactive (ready to be unbound) list.
> > +	 */
> > +	unsigned int active:1;
> > +
> >  	/** This object's place on the active/inactive lists */
> >  	struct list_head mm_list;
> >  
> > @@ -1250,13 +1257,6 @@ struct drm_i915_gem_object {
> >  	struct list_head exec_list;
> >  
> >  	/**
> > -	 * This is set if the object is on the active lists (has pending
> > -	 * rendering and so a non-zero seqno), and is not set if it i s on
> > -	 * inactive (ready to be unbound) list.
> > -	 */
> > -	unsigned int active:1;
> > -
> > -	/**
> >  	 * This is set if the object has been written to since last bound
> >  	 * to the GTT
> >  	 */
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index c2ecb78..b87073b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -137,7 +137,13 @@ int i915_mutex_lock_interruptible(struct drm_device *dev)
> >  /* NB: Not the same as !i915_gem_object_is_inactive */
> >  bool i915_gem_object_is_active(struct drm_i915_gem_object *obj)
> >  {
> > -	return obj->active;
> > +	struct i915_vma *vma;
> > +
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > +		if (vma->active)
> > +			return true;
> > +
> > +	return false;
> >  }
> >  
> >  static inline bool
> > @@ -1899,14 +1905,14 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> >  	BUG_ON(ring == NULL);
> >  	obj->ring = ring;
> >  
> > +	/* Move from whatever list we were on to the tail of execution. */
> > +	vma = i915_gem_obj_to_vma(obj, vm);
> >  	/* Add a reference if we're newly entering the active list. */
> > -	if (!i915_gem_object_is_active(obj)) {
> > +	if (!vma->active) {
> >  		drm_gem_object_reference(&obj->base);
> > -		obj->active = 1;
> > +		vma->active = 1;
> >  	}
> >  
> > -	/* Move from whatever list we were on to the tail of execution. */
> > -	vma = i915_gem_obj_to_vma(obj, vm);
> >  	list_move_tail(&vma->mm_list, &vm->active_list);
> >  	list_move_tail(&obj->ring_list, &ring->active_list);
> >  
> > @@ -1927,16 +1933,23 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
> >  }
> >  
> >  static void
> > -i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj,
> > -				 struct i915_address_space *vm)
> > +i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
> >  {
> > +	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > +	struct i915_address_space *vm;
> >  	struct i915_vma *vma;
> > +	int i = 0;
> >  
> >  	BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
> > -	BUG_ON(!i915_gem_object_is_active(obj));
> >  
> > -	vma = i915_gem_obj_to_vma(obj, vm);
> > -	list_move_tail(&vma->mm_list, &vm->inactive_list);
> > +	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > +		vma = i915_gem_obj_to_vma(obj, vm);
> For paranoia we might want to track the vm used to run a batch in it
> request struct, then we
> 
> > +		if (!vma || !vma->active)
> > +			continue;
> > +		list_move_tail(&vma->mm_list, &vm->inactive_list);
> > +		vma->active = 0;
> > +		i++;
> > +	}
> >  
> >  	list_del_init(&obj->ring_list);
> >  	obj->ring = NULL;
> > @@ -1948,8 +1961,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj,
> >  	obj->last_fenced_seqno = 0;
> >  	obj->fenced_gpu_access = false;
> >  
> > -	obj->active = 0;
> > -	drm_gem_object_unreference(&obj->base);
> > +	while (i--)
> > +		drm_gem_object_unreference(&obj->base);
> >  
> >  	WARN_ON(i915_verify_lists(dev));
> >  }
> > @@ -2272,15 +2285,13 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
> >  	}
> >  
> >  	while (!list_empty(&ring->active_list)) {
> > -		struct i915_address_space *vm;
> >  		struct drm_i915_gem_object *obj;
> >  
> >  		obj = list_first_entry(&ring->active_list,
> >  				       struct drm_i915_gem_object,
> >  				       ring_list);
> >  
> > -		list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > -			i915_gem_object_move_to_inactive(obj, vm);
> > +		i915_gem_object_move_to_inactive(obj);
> >  	}
> >  }
> >  
> > @@ -2356,8 +2367,6 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> >  	 * by the ringbuffer to the flushing/inactive lists as appropriate.
> >  	 */
> >  	while (!list_empty(&ring->active_list)) {
> > -		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > -		struct i915_address_space *vm;
> >  		struct drm_i915_gem_object *obj;
> >  
> >  		obj = list_first_entry(&ring->active_list,
> > @@ -2367,8 +2376,8 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> >  		if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> >  			break;
> >  
> > -		list_for_each_entry(vm, &dev_priv->vm_list, global_link)
> > -			i915_gem_object_move_to_inactive(obj, vm);
> > +		BUG_ON(!i915_gem_object_is_active(obj));
> > +		i915_gem_object_move_to_inactive(obj);
> >  	}
> >  
> >  	if (unlikely(ring->trace_irq_seqno &&
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > 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

-- 
Ben Widawsky, Intel Open Source Technology Center