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

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Aug 06, 2013 at 11:29:49PM +0200, Daniel Vetter wrote:
> On Tue, Aug 06, 2013 at 02:27:39PM -0700, Ben Widawsky wrote:
> > 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:
> > > > @@ -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?
> 
> Yes, when we've terminally fragmented the gtt we kick out everything and
> start over. That's the 3rd usecase.
> -Daniel

I am not seeing it. To me evict_something is what you want, and the fix
for wherever the 3rd usecase is (please point it out, I'm dense) is it
should call evict_something, not evict_everything.

If by GTT you mean the aperture... that's kind of a different can of
worms completely. In that case I don't think you want to do anything per
VM, though potentially you can do it that way and be a little fairer.


-- 
Ben Widawsky, Intel Open Source Technology Center