Re: [PATCH 18/29] drm/i915: Use new bind/unbind in eviction code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 06, 2013 at 08:39:50PM +0200, Daniel Vetter wrote:
> On Wed, Jul 31, 2013 at 05:00:11PM -0700, Ben Widawsky wrote:
> > Eviction code, like the rest of the converted code needs to be aware of
> > the address space for which it is evicting (or the everything case, all
> > addresses). With the updated bind/unbind interfaces of the last patch,
> > we can now safely move the eviction code over.
> > 
> > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> 
> Two comments below.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  4 ++-
> >  drivers/gpu/drm/i915/i915_gem.c       |  2 +-
> >  drivers/gpu/drm/i915/i915_gem_evict.c | 53 +++++++++++++++++++----------------
> >  3 files changed, 33 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0610588..bf1ecef 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1946,7 +1946,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
> >  
> >  
> >  /* i915_gem_evict.c */
> > -int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
> > +int __must_check i915_gem_evict_something(struct drm_device *dev,
> > +					  struct i915_address_space *vm,
> > +					  int min_size,
> >  					  unsigned alignment,
> >  					  unsigned cache_level,
> >  					  bool mappable,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0cb36c2..1013105 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3159,7 +3159,7 @@ search_free:
> >  						  size, alignment,
> >  						  obj->cache_level, 0, gtt_max);
> >  	if (ret) {
> > -		ret = i915_gem_evict_something(dev, size, alignment,
> > +		ret = i915_gem_evict_something(dev, vm, size, alignment,
> >  					       obj->cache_level,
> >  					       map_and_fenceable,
> >  					       nonblocking);
> > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> > index 9205a41..61bf5e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -32,26 +32,21 @@
> >  #include "i915_trace.h"
> >  
> >  static bool
> > -mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
> > +mark_free(struct i915_vma *vma, struct list_head *unwind)
> >  {
> > -	struct drm_device *dev = obj->base.dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct i915_vma *vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base);
> > -
> > -	if (obj->pin_count)
> > +	if (vma->obj->pin_count)
> >  		return false;
> >  
> > -	list_add(&obj->exec_list, unwind);
> > +	list_add(&vma->obj->exec_list, unwind);
> >  	return drm_mm_scan_add_block(&vma->node);
> >  }
> >  
> >  int
> > -i915_gem_evict_something(struct drm_device *dev, int min_size,
> > -			 unsigned alignment, unsigned cache_level,
> > +i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,
> > +			 int min_size, unsigned alignment, unsigned cache_level,
> >  			 bool mappable, bool nonblocking)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > -	struct i915_address_space *vm = &dev_priv->gtt.base;
> >  	struct list_head eviction_list, unwind_list;
> >  	struct i915_vma *vma;
> >  	struct drm_i915_gem_object *obj;
> > @@ -83,16 +78,18 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> >  	 */
> >  
> >  	INIT_LIST_HEAD(&unwind_list);
> > -	if (mappable)
> > +	if (mappable) {
> > +		BUG_ON(!i915_is_ggtt(vm));
> >  		drm_mm_init_scan_with_range(&vm->mm, min_size,
> >  					    alignment, cache_level, 0,
> >  					    dev_priv->gtt.mappable_end);
> > -	else
> > +	} else
> >  		drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level);
> >  
> >  	/* First see if there is a large enough contiguous idle region... */
> >  	list_for_each_entry(obj, &vm->inactive_list, mm_list) {
> > -		if (mark_free(obj, &unwind_list))
> > +		struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > +		if (mark_free(vma, &unwind_list))
> >  			goto found;
> >  	}
> >  
> > @@ -101,7 +98,8 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
> >  
> >  	/* Now merge in the soon-to-be-expired objects... */
> >  	list_for_each_entry(obj, &vm->active_list, mm_list) {
> > -		if (mark_free(obj, &unwind_list))
> > +		struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm);
> > +		if (mark_free(vma, &unwind_list))
> >  			goto found;
> >  	}
> >  
> > @@ -111,7 +109,7 @@ none:
> >  		obj = list_first_entry(&unwind_list,
> >  				       struct drm_i915_gem_object,
> >  				       exec_list);
> > -		vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base);
> > +		vma = i915_gem_obj_to_vma(obj, vm);
> >  		ret = drm_mm_scan_remove_block(&vma->node);
> >  		BUG_ON(ret);
> >  
> > @@ -132,7 +130,7 @@ found:
> >  		obj = list_first_entry(&unwind_list,
> >  				       struct drm_i915_gem_object,
> >  				       exec_list);
> > -		vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base);
> > +		vma = i915_gem_obj_to_vma(obj, vm);
> >  		if (drm_mm_scan_remove_block(&vma->node)) {
> >  			list_move(&obj->exec_list, &eviction_list);
> >  			drm_gem_object_reference(&obj->base);
> > @@ -147,7 +145,7 @@ found:
> >  				       struct drm_i915_gem_object,
> >  				       exec_list);
> >  		if (ret == 0)
> > -			ret = i915_gem_object_ggtt_unbind(obj);
> > +			ret = i915_vma_unbind(i915_gem_obj_to_vma(obj, vm));
> 
> Again I think the ggtt_unbind->vma_unbind conversion seems to leak the
> vma. It feels like vma_unbind should call vma_destroy?
> 

The VMA should get cleaned up when an object backing the vmas is
destroyed also. I agree that at present, unbind and destroy have little
distinction, but I can foresee that changing. Proper faulting is one
such case OTTOMH

Anyway, let me know if your leak concern is addressed.

> >  
> >  		list_del_init(&obj->exec_list);
> >  		drm_gem_object_unreference(&obj->base);
> > @@ -160,13 +158,18 @@ int
> >  i915_gem_evict_everything(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *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, *next;
> > -	bool lists_empty;
> > +	bool lists_empty = true;
> >  	int ret;
> >  
> > -	lists_empty = (list_empty(&vm->inactive_list) &&
> > -		       list_empty(&vm->active_list));
> > +	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > +		lists_empty = (list_empty(&vm->inactive_list) &&
> > +			       list_empty(&vm->active_list));
> > +		if (!lists_empty)
> > +			lists_empty = false;
> > +	}
> > +
> >  	if (lists_empty)
> >  		return -ENOSPC;
> >  
> > @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev)
> >  	i915_gem_retire_requests(dev);
> >  
> >  	/* Having flushed everything, unbind() should never raise an error */
> > -	list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list)
> > -		if (obj->pin_count == 0)
> > -			WARN_ON(i915_gem_object_ggtt_unbind(obj));
> > +	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> > +		list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list)
> > +			if (obj->pin_count == 0)
> > +				WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm)));
> > +	}
> 
> The conversion of evict_everything looks a bit strange. Essentially we
> have tree callers:
> - ums+gem support code in leavevt to rid the gtt of all gem objects when
>   the userspace X ums ddx stops controlling the hw.
> - When we seriously ran out of memory in, shrink_all.
> - In execbuf when we've fragmented the gtt address space so badly that we
>   need to start over completely fresh.
> 
> With this it imo would make sense to just loop over the global bound
> object lists. But for the execbuf caller adding a vm parameter (and only
> evicting from that special vm, skipping all others) would make sense.
> Other callers would pass NULL since they want everything to get evicted.
> Volunteered for that follow-up?
> 

We need evict_everything for #1. For #2, we call evict_something already
for the vm when we go through the out of space path. If that failed,
evicting everything for a specific VM is just the same operation. But
maybe I've glossed over something in the details. Please correct if I'm
wrong. Is there a case where evict something might fail with ENOSPC, and
evict everything in a VM would help?

> >  
> >  	return 0;
> >  }
-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux