[Intel-gfx] [PATCH v4] drm/i915: Add soft-pinning API for execbuffer

[Daniel, Thomas]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Saturday, July 4, 2015 1:24 PM
> To: Kristian Høgsberg
> Cc: Daniel, Thomas; Belgaumkar, Vinay; intel-gfx at lists.freedesktop.org; Michal
> Winiarsky; Goel, Akash
> Subject: Re: [Intel-gfx] [PATCH v4] drm/i915: Add soft-pinning API for execbuffer
> 
> On Fri, Jul 03, 2015 at 10:29:44PM -0700, Kristian Høgsberg wrote:
> > On Tue, Jun 30, 2015 at 7:13 AM, Thomas Daniel <thomas.daniel at intel.com>
> wrote:
> > > From: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > > 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.
> > >
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > > v2: Fixed incorrect eviction found by Michal Winiarski - fix suggested by Chris
> > > Wilson.  Fixed incorrect error paths causing crash found by Michal Winiarski.
> > > (Not published externally)
> > >
> > > v3: Rebased because of trivial conflict in object_bind_to_vm.  Fixed eviction
> > > to allow eviction of soft-pinned objects when another soft-pinned object
> used
> > > by a subsequent execbuffer overlaps reported by Michal Winiarski.
> > > (Not published externally)
> > >
> > > v4: Moved soft-pinned objects to the front of ordered_vmas so that they are
> > > pinned first after an address conflict happens to avoid repeated conflicts in
> > > rare cases (Suggested by Chris Wilson).  Expanded comment on
> > > drm_i915_gem_exec_object2.offset to cover this new API.
> 
> Note this patch is outdated compared to the one I have in my tree. There
> are also requirements to improve drm_mm_reserve_node().
What requirements are these?

> > Can we add an I915_PARAM_HAS_EXEC_PINNED param for this feature?
> 
> Yeah, it's not that difficult to test,
> 
> static bool test_has_softpin(int fd)
> {
>    struct drm_i915_gem_create create;
>    struct drm_i915_gem_exec_object2 object;
>    struct drm_i915_gem_pwrite pwrite;
>    struct drm_i915_gem_execbuffer2 execbuf;
>    uint32_t batch[2] = { 0xa << 23 };
>    bool ret = false;
> 
>    if (DBG_NO_SOFTPIN)
>       return DBG_NO_SOFTPIN < 0;
> 
>    if (gem_param(fd, I915_PARAM_HAS_ALIASING_PPGTT) < 2)
>       return false;
> 
>    memset(&create, 0, sizeof(create));
>    create.size = 4096;
>    drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> 
>    memset(&pwrite, 0, sizeof(pwrite));
>    pwrite.handle = create.handle;
>    pwrite.offset = 0;
>    pwrite.size = sizeof(batch);
>    pwrite.data_ptr = (uintptr_t)batch;
>    if (drmIoctl(fd, DRM_IOCTL_I915_GEM_PWRITE, &pwrite))
>       goto fail;
> 
>    object.handle = create.handle;
> 
>    memset(&execbuf, 0, sizeof(execbuf));
>    execbuf.buffers_ptr = (uintptr_t)&object;
>    execbuf.buffer_count = 1;
>    if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf))
>       goto fail;
> 
>    object.flags = EXEC_OBJECT_PINNED;
>    ret  = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0;
> fail:
>    drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, &create);
> 
>    return ret;
> }
> 
> but compares to
> 
> static bool test_has_softpin(int fd)
> {
>    if (DBG_NO_SOFTPIN)
>       return DBG_NO_SOFTPIN < 0;
> 
>    if (gem_param(fd, I915_PARAM_HAS_ALIASING_PPGTT) < 2)
>       return false;
> 
>    return gem_param(fd, I915_PARAM_HAS_EXEC_SOFTPIN) > 0;
> }
> 
> with a parameter. At some point, we probably want to add a GETPARAMV!
Yes, a parameter would be cleaner.

Thomas.