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

[Chris Wilson]

On Thu, Nov 24, 2016 at 01:45:12PM +0200, Joonas Lahtinen wrote:
> On ke, 2016-11-23 at 14:11 +0000, Chris Wilson wrote:
> > Soft-pinning depends upon being able to check for availabilty of an
> > interval and evict overlapping object from a drm_mm range manager very
> > quickly. Currently it uses a linear list, and so performance is dire and
> > not suitable as a general replacement. Worse, the current code will oops
> > if it tries to evict an active buffer.
> > 
> > It also helps if the routine reports the correct error codes as expected
> > by its callers and emits a tracepoint upon use.
> > 
> > For posterity since the wrong patch was pushed (i.e. that missed these
> > key points and had known bugs), this is the changelog that should have
> > been on commit 506a8e87d8d2 ("drm/i915: Add soft-pinning API for
> > execbuffer"):
> > 
> > Userspace can pass in an offset that it presumes the object is located
> > at. The kernel will then do its utmost to fit the object into that
> > location. The assumption is that userspace is handling its own object
> > locations (for example along with full-ppgtt) and that the kernel will
> > rarely have to make space for the user's requests.
> > 
> > This extends the DRM_IOCTL_I915_GEM_EXECBUFFER2 to do the following:
> > * if the user supplies a virtual address via the execobject->offset
> >   *and* sets the EXEC_OBJECT_PINNED flag in execobject->flags, then
> >   that object is placed at that offset in the address space selected
> >   by the context specifier in execbuffer.
> > * the location must be aligned to the GTT page size, 4096 bytes
> > * as the object is placed exactly as specified, it may be used by this
> >   execbuffer call without relocations pointing to it
> > 
> > It may fail to do so if:
> > * EINVAL is returned if the object does not have a 4096 byte aligned
> >   address
> > * the object conflicts with another pinned object (either pinned by
> >   hardware in that address space, e.g. scanouts in the aliasing ppgtt)
> >   or within the same batch.
> >   EBUSY is returned if the location is pinned by hardware
> >   EINVAL is returned if the location is already in use by the batch
> > * EINVAL is returned if the object conflicts with its own alignment (as meets
> >   the hardware requirements) or if the placement of the object does not fit
> >   within the address space
> > 
> > All other execbuffer errors apply.
> > 
> > Presence of this execbuf extension may be queried by passing
> > I915_PARAM_HAS_EXEC_SOFTPIN to DRM_IOCTL_I915_GETPARAM and checking for
> > a reported value of 1 (or greater).
> > 
> > v2: Combine the hole/adjusted-hole ENOSPC checks
> > v3: More color, more splitting, more blurb.
> > 
> > Fixes: 506a8e87d8d2 ("drm/i915: Add soft-pinning API for execbuffer")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +TRACE_EVENT(i915_gem_evict_vma,
> > +	    TP_PROTO(struct i915_vma *vma, unsigned int flags),
> > +	    TP_ARGS(vma, flags),
> > +
> > +	    TP_STRUCT__entry(
> > +			     __field(u32, dev)
> > +			     __field(struct i915_address_space *, vm)
> > +			     __field(u64, start)
> > +			     __field(u64, size)
> > +			     __field(unsigned int, flags)
> > +			    ),
> > +
> > +	    TP_fast_assign(
> > +			   __entry->dev = vma->vm->dev->primary->index;
> > +			   __entry->vm = vma->vm;
> > +			   __entry->start = vma->node.start;
> > +			   __entry->size = vma->node.size;
> > +			   __entry->flags = flags;
> > +			  ),
> > +
> > +	    TP_printk("dev=%d, vm=%p, start=%llx size=%llx, flags=%x", __entry->dev, __entry->vm, (long long)__entry->start, (long long)__entry->size, __entry->flags)
> 
> A few newlines could be useful. (long long) explicit conversion is not
> done elsewhere in the func but should not hurt. Can't complain on lack
> of consistency as it's wild already :P

The (long long) is wrong. u64 should be %llx. Can't remember if there is
a good excuse or not (it was almost certainly copied from elsewhere ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre