[Intel-gfx] [PATCH] drm/i915/gem: Only call eb_lookup_vma once during execbuf ioctl
Chris Wilson
chris at chris-wilson.co.uk
Fri Feb 21 13:36:15 UTC 2020
Quoting Maarten Lankhorst (2020-02-21 13:31:44)
> Op 21-02-2020 om 14:15 schreef Chris Wilson:
> > As we no longer stash anything inside i915_vma under the exclusive
> > protection of struct_mutex, we do not need to revoke the i915_vma
> > stashes before dropping struct_mutex to handle pagefaults. Knowing that
> > we must drop the struct_mutex while keeping the eb->vma around, means
> > that we are required to hold onto to the object reference until we have
> > marked the vma as active.
> >
> > Fixes: 155ab8836caa ("drm/i915: Move object close under its own lock")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > ---
> > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 109 +++++++-----------
> > 1 file changed, 43 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 3ccd3a096486..e4e50155145e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -48,17 +48,15 @@ enum {
> > #define DBG_FORCE_RELOC 0 /* choose one of the above! */
> > };
> >
> > -#define __EXEC_OBJECT_HAS_REF BIT(31)
> > -#define __EXEC_OBJECT_HAS_PIN BIT(30)
> > -#define __EXEC_OBJECT_HAS_FENCE BIT(29)
> > -#define __EXEC_OBJECT_NEEDS_MAP BIT(28)
> > -#define __EXEC_OBJECT_NEEDS_BIAS BIT(27)
> > -#define __EXEC_OBJECT_INTERNAL_FLAGS (~0u << 27) /* all of the above */
> > +#define __EXEC_OBJECT_HAS_PIN BIT(31)
> > +#define __EXEC_OBJECT_HAS_FENCE BIT(30)
> > +#define __EXEC_OBJECT_NEEDS_MAP BIT(29)
> > +#define __EXEC_OBJECT_NEEDS_BIAS BIT(28)
> > +#define __EXEC_OBJECT_INTERNAL_FLAGS (~0u << 28) /* all of the above */
> > #define __EXEC_OBJECT_RESERVED (__EXEC_OBJECT_HAS_PIN | __EXEC_OBJECT_HAS_FENCE)
> >
> > #define __EXEC_HAS_RELOC BIT(31)
> > -#define __EXEC_VALIDATED BIT(30)
> > -#define __EXEC_INTERNAL_FLAGS (~0u << 30)
> > +#define __EXEC_INTERNAL_FLAGS (~0u << 31)
> > #define UPDATE PIN_OFFSET_FIXED
> >
> > #define BATCH_OFFSET_BIAS (256*1024)
> > @@ -473,24 +471,17 @@ eb_validate_vma(struct i915_execbuffer *eb,
> > return 0;
> > }
> >
> > -static int
> > +static void
> > eb_add_vma(struct i915_execbuffer *eb,
> > unsigned int i, unsigned batch_idx,
> > struct i915_vma *vma)
> > {
> > struct drm_i915_gem_exec_object2 *entry = &eb->exec[i];
> > struct eb_vma *ev = &eb->vma[i];
> > - int err;
> >
> > GEM_BUG_ON(i915_vma_is_closed(vma));
> >
> > - if (!(eb->args->flags & __EXEC_VALIDATED)) {
> > - err = eb_validate_vma(eb, entry, vma);
> > - if (unlikely(err))
> > - return err;
> > - }
> > -
> > - ev->vma = vma;
> > + ev->vma = i915_vma_get(vma);
> > ev->exec = entry;
> > ev->flags = entry->flags;
> >
> > @@ -523,20 +514,14 @@ eb_add_vma(struct i915_execbuffer *eb,
> > eb->batch = ev;
> > }
> >
> > - err = 0;
> > if (eb_pin_vma(eb, entry, ev)) {
> > if (entry->offset != vma->node.start) {
> > entry->offset = vma->node.start | UPDATE;
> > eb->args->flags |= __EXEC_HAS_RELOC;
> > }
> > } else {
> > - eb_unreserve_vma(ev);
> > -
> > list_add_tail(&ev->bind_link, &eb->unbound);
> > - if (drm_mm_node_allocated(&vma->node))
> > - err = i915_vma_unbind(vma);
> > }
> > - return err;
> > }
> >
> > static inline int use_cpu_reloc(const struct reloc_cache *cache,
> > @@ -585,6 +570,14 @@ static int eb_reserve_vma(const struct i915_execbuffer *eb, struct eb_vma *ev)
> > pin_flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS;
> > }
> >
> > + if (drm_mm_node_allocated(&vma->node) &&
> > + eb_vma_misplaced(entry, vma, ev->flags)) {
> > + eb_unreserve_vma(ev);
> > + err = i915_vma_unbind(vma);
> > + if (err)
> > + return err;
> > + }
> > +
> > err = i915_vma_pin(vma,
> > entry->pad_to_size, entry->alignment,
> > pin_flags);
> > @@ -643,7 +636,7 @@ static int eb_reserve(struct i915_execbuffer *eb)
> > if (err)
> > break;
> > }
> > - if (err != -ENOSPC)
> > + if (!(err == -ENOSPC || err == -EAGAIN))
> > return err;
> >
> > /* Resort *all* the objects into priority order */
> > @@ -673,6 +666,11 @@ static int eb_reserve(struct i915_execbuffer *eb)
> > }
> > list_splice_tail(&last, &eb->unbound);
> >
> > + if (err == -EAGAIN) {
> > + flush_workqueue(eb->i915->mm.userptr_wq);
> > + continue;
> > + }
> > +
> > switch (pass++) {
> > case 0:
> > break;
> > @@ -726,17 +724,14 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > unsigned int i, batch;
> > int err;
> >
> > + if (unlikely(i915_gem_context_is_closed(eb->gem_context)))
> > + return -ENOENT;
> > +
> > INIT_LIST_HEAD(&eb->relocs);
> > INIT_LIST_HEAD(&eb->unbound);
> >
> > batch = eb_batch_index(eb);
> >
> > - mutex_lock(&eb->gem_context->mutex);
> > - if (unlikely(i915_gem_context_is_closed(eb->gem_context))) {
> > - err = -ENOENT;
> > - goto err_ctx;
> > - }
> > -
> > for (i = 0; i < eb->buffer_count; i++) {
> > u32 handle = eb->exec[i].handle;
> > struct i915_lut_handle *lut;
> > @@ -781,25 +776,19 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > i915_gem_object_unlock(obj);
> >
> > add_vma:
> > - err = eb_add_vma(eb, i, batch, vma);
> > + err = eb_validate_vma(eb, &eb->exec[i], vma);
> > if (unlikely(err))
> > goto err_vma;
> >
> > - GEM_BUG_ON(drm_mm_node_allocated(&vma->node) &&
> > - eb_vma_misplaced(&eb->exec[i], vma, eb->vma[i].flags));
> > + eb_add_vma(eb, i, batch, vma);
> > }
> >
> > - mutex_unlock(&eb->gem_context->mutex);
> > -
> > - eb->args->flags |= __EXEC_VALIDATED;
> > - return eb_reserve(eb);
> > + return 0;
> >
> > err_obj:
> > i915_gem_object_put(obj);
> > err_vma:
> > eb->vma[i].vma = NULL;
> > -err_ctx:
> > - mutex_unlock(&eb->gem_context->mutex);
> > return err;
> > }
> >
> > @@ -840,19 +829,10 @@ static void eb_release_vmas(const struct i915_execbuffer *eb)
> > if (ev->flags & __EXEC_OBJECT_HAS_PIN)
> > __eb_unreserve_vma(vma, ev->flags);
> >
> > - if (ev->flags & __EXEC_OBJECT_HAS_REF)
> > - i915_vma_put(vma);
> > + i915_vma_put(vma);
> > }
> > }
> >
> > -static void eb_reset_vmas(const struct i915_execbuffer *eb)
> > -{
> > - eb_release_vmas(eb);
> > - if (eb->lut_size > 0)
> > - memset(eb->buckets, 0,
> > - sizeof(struct hlist_head) << eb->lut_size);
> > -}
> > -
> > static void eb_destroy(const struct i915_execbuffer *eb)
> > {
> > GEM_BUG_ON(eb->reloc_cache.rq);
> > @@ -1661,8 +1641,6 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
> > goto out;
> > }
> >
> > - /* We may process another execbuffer during the unlock... */
> > - eb_reset_vmas(eb);
> > mutex_unlock(&dev->struct_mutex);
> >
> > /*
> > @@ -1701,11 +1679,6 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
> > goto out;
> > }
> >
> > - /* reacquire the objects */
> > - err = eb_lookup_vmas(eb);
> > - if (err)
> > - goto err;
> > -
> > GEM_BUG_ON(!eb->batch);
> >
> > list_for_each_entry(ev, &eb->relocs, reloc_link) {
> > @@ -1756,8 +1729,17 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
> >
> > static int eb_relocate(struct i915_execbuffer *eb)
> > {
> > - if (eb_lookup_vmas(eb))
> > - goto slow;
> > + int err;
> > +
> > + mutex_lock(&eb->gem_context->mutex);
> > + err = eb_lookup_vmas(eb);
> > + mutex_unlock(&eb->gem_context->mutex);
> > + if (err)
> > + return err;
> > +
> > + err = eb_reserve(eb);
> > + if (err)
> > + return err;
> >
> > /* The objects are in their final locations, apply the relocations. */
> > if (eb->args->flags & __EXEC_HAS_RELOC) {
> > @@ -1765,14 +1747,11 @@ static int eb_relocate(struct i915_execbuffer *eb)
> >
> > list_for_each_entry(ev, &eb->relocs, reloc_link) {
> > if (eb_relocate_vma(eb, ev))
> > - goto slow;
> > + return eb_relocate_slow(eb);
> > }
> > }
> >
> > return 0;
> > -
> > -slow:
> > - return eb_relocate_slow(eb);
> > }
> >
> > static int eb_move_to_gpu(struct i915_execbuffer *eb)
> > @@ -1854,8 +1833,7 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
> > i915_vma_unlock(vma);
> >
> > __eb_unreserve_vma(vma, flags);
> > - if (unlikely(flags & __EXEC_OBJECT_HAS_REF))
> > - i915_vma_put(vma);
> > + i915_vma_put(vma);
> >
> > ev->vma = NULL;
> > }
> > @@ -2115,8 +2093,7 @@ static int eb_parse(struct i915_execbuffer *eb)
> > goto err_trampoline;
> >
> > eb->vma[eb->buffer_count].vma = i915_vma_get(shadow);
> > - eb->vma[eb->buffer_count].flags =
> > - __EXEC_OBJECT_HAS_PIN | __EXEC_OBJECT_HAS_REF;
> > + eb->vma[eb->buffer_count].flags = __EXEC_OBJECT_HAS_PIN;
> > eb->batch = &eb->vma[eb->buffer_count++];
> >
> > eb->trampoline = trampoline;
>
> Did you steal this from me? :P
It was in the series that removes struct_mutex. I think we should just
remove struct_mutex right away so that we have parallel execbuf as a
baseline.
-Chris
More information about the Intel-gfx
mailing list