[Intel-gfx] [PATCH v2 4/4] drm/i915: Fix i915_gem_evict_for_vma (soft-pinning)

[Chris Wilson]

On Fri, Nov 18, 2016 at 11:18:09AM +0200, Joonas Lahtinen wrote:
> On to, 2016-11-17 at 12:08 +0000, Chris Wilson wrote: 
> > +		if (flags & PIN_NONBLOCK &&
> > +		    (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))) {
> > +			ret = -ENOSPC;
> > +			break;
> > +		}
> 
> i915_vma_is_pinned() being true will exit this loop with -ENOSPC with
> or without NOBLOCK, just skipping the exec_entry test without it. I
> would clarify that. Now it's bit odd.

It's a necessary test for use by execbuf. The interface is that it tests
a location first with NONBLOCK before deciding on whether it is a good
final location. (With various other hints as to whether any eviction is
a good idea, vs whether it mandatory to use this location.)

> > -			return -ENOSPC;
> > +		/* Overlap of objects in the same batch? */
> > +		if (i915_vma_is_pinned(vma)) {
> > +			ret = -ENOSPC;
> > +			if (vma->exec_entry &&
> > +			    vma->exec_entry->flags & EXEC_OBJECT_PINNED)
> > +				ret = -EINVAL;
> > +			break;
> >  		}
> >  
> > -		ret = i915_vma_unbind(vma);
> > -		if (ret)
> > -			return ret;
> > +		__i915_vma_pin(vma);
> 
> I don't quite see why? Are you expecting the iteration to hit same vma
> twice? Or somebody moving it while we iterate.

The unbind may causes a free of any member on this list, so the pinning
prevents other vma from being unbound whilst waiting on this one. It
used to be a big deal, but since the various reworking the deferred free
hides the oops.
 
> > +		list_add(&vma->exec_list, &eviction_list);
> 
> I'd prefer an union instead of brutally reusing member for other
> purposes.

There have been patches to add evict_link :-p
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre