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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@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

With the newlines, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx