Re: [PATCH] drm/i915: Add soft-pinning API for execbuffer

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Daniel, Thomas" <thomas.daniel@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: Add soft-pinning API for execbuffer
Date: Wed, 29 Apr 2015 15:01:49 +0100	[thread overview]
Message-ID: <20150429140149.GR599@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <BFEE8FEC12424048AF1805991D65FA911978D56C@irsmsx105.ger.corp.intel.com>

On Wed, Apr 29, 2015 at 01:28:19PM +0000, Daniel, Thomas wrote:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > Chris Wilson
> > Sent: Friday, March 6, 2015 9:44 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH] 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.	
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
> >  drivers/gpu/drm/i915/i915_gem.c            | 53 ++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/i915_gem_evict.c      | 52
> > +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  9 ++++-
> >  include/uapi/drm/i915_drm.h                |  3 +-
> >  5 files changed, 106 insertions(+), 16 deletions(-)
> > 
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 9d0df4d85693..b266b31690e4 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3592,22 +3592,43 @@ i915_gem_object_bind_to_vm(struct
> > drm_i915_gem_object *obj,
> >  	if (IS_ERR(vma))
> >  		goto err_unpin;
> > 
> > +	if (flags & PIN_OFFSET_FIXED) {
> > +		uint64_t offset = flags & PIN_OFFSET_MASK;
> > +		if (offset & (alignment - 1)) {
> > +			vma = ERR_PTR(-EINVAL);
> > +			goto err_free_vma;
> > +		}
> > +		vma->node.start = offset;
> > +		vma->node.size = size;
> > +		vma->node.color = obj->cache_level;
> > +		ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> > +		if (ret) {
> > +			ret = i915_gem_evict_range(dev, vm, start, end);
> Did you mean i915_gem_evict_range(dev, vm, offset, offset+size) ?

Whoops. I guess i915_gem_evict_vma() would be a better interface.

> > +int
> > +i915_gem_evict_range(struct drm_device *dev, struct i915_address_space
> > *vm,
> > +		     unsigned long start, unsigned long end)
> > +{
> > +	struct drm_mm_node *node;
> > +	struct list_head eviction_list;
> > +	int ret = 0;
> > +
> > +	INIT_LIST_HEAD(&eviction_list);
> > +	drm_mm_for_each_node(node, &vm->mm) {
> > +		struct i915_vma *vma;
> > +
> > +		if (node->start + node->size <= start)
> > +			continue;
> > +		if (node->start >= end)
> > +			break;
> > +
> > +		vma = container_of(node, typeof(*vma), node);
> > +		if (vma->pin_count) {
> > +			ret = -EBUSY;
> > +			break;
> > +		}
> > +
> > +		if (WARN_ON(!list_empty(&vma->exec_list))) { 
> So if an execbuffer uses both EXEC_OBJECT_PINNED and ordinary buffers in its exec_list then the ordinary buffers cannot be relocated if they are in the range of the pinned buffer.  Was this your intention?

The desired behaviour should be to restart the reservation process with
bind the fixed objects first, then bind the rest.

The reservation logic is design around not moving objects, with the
expectation that we can reuse the layout from the last batch and
optimised for that. We should return ENOSPC here which causes the
reservation logic to unpin everything, evict and try again. It will be
easy enough to place the fixed objects at the head of the second pass.

I choose EINVAL initially thinking it should catch the condition of two
overlapping fixed objects without consider evicting ordinary bo (I was
caught thinking of userspace doing an all or nothing approach).

> 
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		drm_gem_object_reference(&vma->obj->base);
> > +		list_add(&vma->exec_list, &eviction_list);
> I guess we need another list_head if we want to support both types of object in the same execbuffer call and allow relocation.

Actually, here since we are walking the drm_mm rather than using the
scanner, we can switch over to list_for_each_entry_safe() and do the
unbinding inline.

Reduces to
int
i915_gem_evict_for_vma(struct i915_vma *target)
{
        struct drm_mm_node *node, *next;
        int ret;

        list_for_each_entry_safe(node, next,
                                 &target->vm->mm.head_node.node_list,
                                 node_list) {
                struct i915_vma *vma;

                if (node->start + node->size <= target->node.start)
                        continue;
                if (node->start >= target->node.start + target->node.size)
                        break;

                vma = container_of(node, typeof(*vma), node);
                if (vma->exec_entry &&
                    vma->exec_entry->flags & EXEC_OBJECT_PINNED)
                        /* Overlapping fixed objects in the same batch */
                        return -EINVAL;

                if (vma->pin_count)
                        /* We may need to evict an buffer in the same batch */
                        return vma->exec_entry ? -ENOSPC : -EBUSY;

                ret = i915_vma_unbind(vma);
                if (ret)
                        return ret;
        }

        return 0;
}

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx