[Intel-gfx] [PATCH] drm/i915/gem: Only call eb_lookup_vma once during execbuf ioctl
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Fri Feb 21 13:31:44 UTC 2020
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
I Ihave a similar patch in my series..
More information about the Intel-gfx
mailing list