[PATCH 6/6] drm/i915: Parse command buffer earlier in eb_relocate(slow)
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Jan 21 16:38:20 UTC 2020
We want to introduce backoff logic, but we need to lock the
pool object as well for command parsing. Because of this, we
will need backoff logic for the engine pool obj, move the batch
validation up slightly to eb_lookup_vmas, and the actual command
parsing in a separate function which can get called from execbuf
relocation fast and slowpath.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 186 ++++++++++--------
1 file changed, 99 insertions(+), 87 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 207ba465aef3..fc2770677f67 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -251,6 +251,8 @@ struct i915_execbuffer {
/** list of vma that have execobj.relocation_count */
struct list_head relocs;
+ struct i915_gem_ww_ctx ww;
+
/**
* Track the most recently used object for relocations, as we
* frequently have to perform multiple relocations within the same
@@ -285,8 +287,11 @@ struct i915_execbuffer {
*/
int lut_size;
struct hlist_head *buckets; /** ht for relocation handles */
+ bool slowpath;
};
+static int eb_parse(struct i915_execbuffer *eb);
+
static inline bool eb_use_cmdparser(const struct i915_execbuffer *eb)
{
return intel_engine_requires_cmd_parser(eb->engine) ||
@@ -772,6 +777,21 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
mutex_unlock(&eb->gem_context->mutex);
+ if (unlikely(eb->batch->flags & EXEC_OBJECT_WRITE)) {
+ DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
+ return -EINVAL;
+ }
+
+ if (range_overflows_t(u64,
+ eb->batch_start_offset, eb->batch_len,
+ eb->batch->vma->size)) {
+ DRM_DEBUG("Attempting to use out-of-bounds batch\n");
+ return -EINVAL;
+ }
+
+ if (eb->batch_len == 0)
+ eb->batch_len = eb->batch->vma->size - eb->batch_start_offset;
+
return 0;
err_obj:
@@ -795,6 +815,10 @@ static int eb_validate_vmas(struct i915_execbuffer *eb)
struct eb_vma *ev = &eb->vma[i];
struct i915_vma *vma = ev->vma;
+ err = i915_gem_object_lock(vma->obj, &eb->ww);
+ if (err)
+ return err;
+
if (eb_pin_vma(eb, entry, ev)) {
if (entry->offset != vma->node.start) {
entry->offset = vma->node.start | UPDATE;
@@ -941,7 +965,6 @@ static void reloc_cache_reset(struct reloc_cache *cache)
kunmap_atomic(vaddr);
i915_gem_object_finish_access(obj);
- i915_gem_object_unlock(obj);
} else {
struct i915_ggtt *ggtt = cache_to_ggtt(cache);
@@ -976,15 +999,9 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
unsigned int flushes;
int err;
- err = i915_gem_object_lock_interruptible(obj, NULL);
- if (err)
- return ERR_PTR(err);
-
err = i915_gem_object_prepare_write(obj, &flushes);
- if (err) {
- i915_gem_object_unlock(obj);
+ if (err)
return ERR_PTR(err);
- }
BUILD_BUG_ON(KMAP & CLFLUSH_FLAGS);
BUILD_BUG_ON((KMAP | CLFLUSH_FLAGS) & PAGE_MASK);
@@ -1023,9 +1040,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
if (use_cpu_reloc(cache, obj))
return NULL;
- i915_gem_object_lock(obj, NULL);
err = i915_gem_object_set_to_gtt_domain(obj, true);
- i915_gem_object_unlock(obj);
if (err)
return ERR_PTR(err);
@@ -1114,7 +1129,7 @@ static int reloc_move_to_gpu(struct i915_request *rq, struct i915_vma *vma)
struct drm_i915_gem_object *obj = vma->obj;
int err;
- i915_vma_lock(vma);
+ assert_vma_held(vma);
if (obj->cache_dirty & ~obj->cache_coherent)
i915_gem_clflush_object(obj, 0);
@@ -1124,8 +1139,6 @@ static int reloc_move_to_gpu(struct i915_request *rq, struct i915_vma *vma)
if (err == 0)
err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
- i915_vma_unlock(vma);
-
return err;
}
@@ -1144,6 +1157,10 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
if (IS_ERR(pool))
return PTR_ERR(pool);
+ err = i915_gem_object_lock(pool->obj, &eb->ww);
+ if (err)
+ goto out_pool;
+
cmd = i915_gem_object_pin_map(pool->obj,
cache->has_llc ?
I915_MAP_FORCE_WB :
@@ -1183,11 +1200,10 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
if (err)
goto skip_request;
- i915_vma_lock(batch);
+ assert_vma_held(batch);
err = i915_request_await_object(rq, batch->obj, false);
if (err == 0)
err = i915_vma_move_to_active(batch, rq, 0);
- i915_vma_unlock(batch);
if (err)
goto skip_request;
@@ -1664,7 +1680,7 @@ static int eb_prefault_relocations(const struct i915_execbuffer *eb)
return 0;
}
-static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
+static noinline int eb_relocate_parse_slow(struct i915_execbuffer *eb)
{
struct drm_device *dev = &eb->i915->drm;
bool have_copy = false;
@@ -1679,6 +1695,7 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
/* We may process another execbuffer during the unlock... */
eb_release_vmas(eb);
+ i915_gem_ww_ctx_fini(&eb->ww);
mutex_unlock(&dev->struct_mutex);
/*
@@ -1705,6 +1722,7 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
}
if (err) {
mutex_lock(&dev->struct_mutex);
+ i915_gem_ww_ctx_init(&eb->ww, true);
goto out;
}
@@ -1716,8 +1734,10 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
mutex_lock(&dev->struct_mutex);
goto out;
}
+ i915_gem_ww_ctx_init(&eb->ww, true);
/* reacquire the objects */
+repeat_validate:
err = eb_validate_vmas(eb);
if (err)
goto err;
@@ -1729,7 +1749,9 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
pagefault_disable();
err = eb_relocate_vma(eb, ev);
pagefault_enable();
- if (err)
+ if (err == -EDEADLK)
+ goto err;
+ else if (err)
goto repeat;
} else {
err = eb_relocate_vma_slow(eb, ev);
@@ -1738,6 +1760,11 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
}
}
+ /* as last step, parse the command buffer */
+ err = eb_parse(eb);
+ if (err)
+ goto err;
+
/*
* Leave the user relocations as are, this is the painfully slow path,
* and we want to avoid the complication of dropping the lock whilst
@@ -1746,6 +1773,13 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
*/
err:
+ if (err == -EDEADLK) {
+ eb_release_vmas(eb);
+ err = i915_gem_ww_ctx_backoff(&eb->ww);
+ if (!err)
+ goto repeat_validate;
+ }
+
if (err == -EAGAIN)
goto repeat;
@@ -1770,7 +1804,7 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb)
return err;
}
-static int eb_relocate(struct i915_execbuffer *eb)
+static int eb_relocate_parse(struct i915_execbuffer *eb)
{
int err;
@@ -1778,59 +1812,58 @@ static int eb_relocate(struct i915_execbuffer *eb)
if (err)
return err;
- if (eb_validate_vmas(eb))
- goto slow;
+retry:
+ err = eb_validate_vmas(eb);
+ if (err)
+ goto err;
/* The objects are in their final locations, apply the relocations. */
if (eb->args->flags & __EXEC_HAS_RELOC) {
struct eb_vma *ev;
list_for_each_entry(ev, &eb->relocs, reloc_link) {
- if (eb_relocate_vma(eb, ev))
- goto slow;
+ err = eb_relocate_vma(eb, ev);
+ if (err)
+ goto err;
}
}
+ err = eb_parse(eb);
+err:
+ if (err == -EDEADLK) {
+ err = i915_gem_ww_ctx_backoff(&eb->ww);
+ if (err)
+ return err;
+
+ goto retry;
+ }
+ else if (err)
+ goto slow;
+
return 0;
slow:
- return eb_relocate_slow(eb);
+ eb->slowpath = true;
+ err = eb_relocate_parse_slow(eb);
+ if (err)
+ /*
+ * If the user expects the execobject.offset and
+ * reloc.presumed_offset to be an exact match,
+ * as for using NO_RELOC, then we cannot update
+ * the execobject.offset until we have completed
+ * relocation.
+ */
+ eb->args->flags &= ~__EXEC_HAS_RELOC;
+
+ return err;
}
static int eb_move_to_gpu(struct i915_execbuffer *eb)
{
const unsigned int count = eb->buffer_count;
- struct ww_acquire_ctx acquire;
- unsigned int i;
+ unsigned int i = count;
int err = 0;
- ww_acquire_init(&acquire, &reservation_ww_class);
-
- for (i = 0; i < count; i++) {
- struct eb_vma *ev = &eb->vma[i];
- struct i915_vma *vma = ev->vma;
-
- err = ww_mutex_lock_interruptible(&vma->resv->lock, &acquire);
- if (err == -EDEADLK) {
- GEM_BUG_ON(i == 0);
- do {
- int j = i - 1;
-
- ww_mutex_unlock(&eb->vma[j].vma->resv->lock);
-
- swap(eb->vma[i], eb->vma[j]);
- } while (--i);
-
- err = ww_mutex_lock_slow_interruptible(&vma->resv->lock,
- &acquire);
- }
- if (err == -EALREADY)
- err = 0;
- if (err)
- break;
- }
- ww_acquire_done(&acquire);
-
while (i--) {
struct eb_vma *ev = &eb->vma[i];
struct i915_vma *vma = ev->vma;
@@ -1875,15 +1908,12 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
if (err == 0)
err = i915_vma_move_to_active(vma, eb->request, flags);
- i915_vma_unlock(vma);
-
__eb_unreserve_vma(vma, flags);
if (unlikely(flags & __EXEC_OBJECT_HAS_REF))
i915_vma_put(vma);
ev->vma = NULL;
}
- ww_acquire_fini(&acquire);
if (unlikely(err))
goto err_skip;
@@ -2039,10 +2069,6 @@ static int eb_parse_pipeline(struct i915_execbuffer *eb,
pw->shadow = shadow;
pw->trampoline = trampoline;
- err = dma_resv_lock_interruptible(pw->batch->resv, NULL);
- if (err)
- goto err_trampoline;
-
err = dma_resv_reserve_shared(pw->batch->resv, 1);
if (err)
goto err_batch_unlock;
@@ -2057,19 +2083,14 @@ static int eb_parse_pipeline(struct i915_execbuffer *eb,
/* Keep the batch alive and unwritten as we parse */
dma_resv_add_shared_fence(pw->batch->resv, &pw->base.dma);
- dma_resv_unlock(pw->batch->resv);
-
/* Force execution to wait for completion of the parser */
- dma_resv_lock(shadow->resv, NULL);
dma_resv_add_excl_fence(shadow->resv, &pw->base.dma);
- dma_resv_unlock(shadow->resv);
dma_fence_work_commit(&pw->base);
return 0;
err_batch_unlock:
dma_resv_unlock(pw->batch->resv);
-err_trampoline:
if (trampoline)
i915_active_release(&trampoline->active);
err_shadow:
@@ -2109,6 +2130,10 @@ static int eb_parse(struct i915_execbuffer *eb)
if (IS_ERR(pool))
return PTR_ERR(pool);
+ err = i915_gem_object_lock(pool->obj, &eb->ww);
+ if (err)
+ goto err;
+
shadow = shadow_batch_pin(pool->obj, eb->context->vm, PIN_USER);
if (IS_ERR(shadow)) {
err = PTR_ERR(shadow);
@@ -2581,6 +2606,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS &
~__EXEC_OBJECT_UNKNOWN_FLAGS);
+ eb.slowpath = false;
eb.i915 = i915;
eb.file = file;
eb.args = args;
@@ -2660,8 +2686,9 @@ i915_gem_do_execbuffer(struct drm_device *dev,
err = mutex_lock_interruptible(&dev->struct_mutex);
if (err)
goto err_engine;
+ i915_gem_ww_ctx_init(&eb.ww, true);
- err = eb_relocate(&eb);
+ err = eb_relocate_parse(&eb);
if (err) {
/*
* If the user expects the execobject.offset and
@@ -2674,32 +2701,13 @@ i915_gem_do_execbuffer(struct drm_device *dev,
goto err_vma;
}
- if (unlikely(eb.batch->flags & EXEC_OBJECT_WRITE)) {
- DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
- err = -EINVAL;
- goto err_vma;
- }
-
- if (range_overflows_t(u64,
- eb.batch_start_offset, eb.batch_len,
- eb.batch->vma->size)) {
- DRM_DEBUG("Attempting to use out-of-bounds batch\n");
- err = -EINVAL;
- goto err_vma;
- }
-
- if (eb.batch_len == 0)
- eb.batch_len = eb.batch->vma->size - eb.batch_start_offset;
-
- err = eb_parse(&eb);
- if (err)
- goto err_vma;
+ WARN(eb.ww.contended, "contended, slow path: %i\n", eb.slowpath);
+ ww_acquire_done(&eb.ww.ctx);
/*
* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
* batch" bit. Hence we need to pin secure batches into the global gtt.
* hsw should have this fixed, but bdw mucks it up again. */
- batch = eb.batch->vma;
if (eb.batch_flags & I915_DISPATCH_SECURE) {
struct i915_vma *vma;
@@ -2713,13 +2721,15 @@ i915_gem_do_execbuffer(struct drm_device *dev,
* fitting due to fragmentation.
* So this is actually safe.
*/
- vma = i915_gem_object_ggtt_pin(batch->obj, NULL, 0, 0, 0);
+ vma = i915_gem_object_ggtt_pin(eb.batch->vma->obj, NULL, 0, 0, 0);
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
goto err_vma;
}
batch = vma;
+ } else {
+ batch = eb.batch->vma;
}
/* All GPU relocation batches must be submitted prior to the user rq */
@@ -2802,6 +2812,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
eb_release_vmas(&eb);
if (eb.trampoline)
i915_vma_unpin(eb.trampoline);
+ WARN_ON(err == -EDEADLK);
+ i915_gem_ww_ctx_fini(&eb.ww);
mutex_unlock(&dev->struct_mutex);
err_engine:
eb_unpin_engine(&eb);
--
2.24.1
More information about the Intel-gfx-trybot
mailing list