Skip to content

Commit

Permalink
drm/i915: Revert "drm/i915/gem: Asynchronous cmdparser"
Browse files Browse the repository at this point in the history 
commit c9d9fdb upstream.

This reverts 686c7c3 ("drm/i915/gem: Asynchronous cmdparser").  The
justification for this commit in the git history was a vague comment
about getting it out from under the struct_mutex.  While this may
improve perf for some workloads on Gen7 platforms where we rely on the
command parser for features such as indirect rendering, no numbers were
provided to prove such an improvement.  It claims to closed two
gitlab/bugzilla issues but with no explanation whatsoever as to why or
what bug it's fixing.

Meanwhile, by moving command parsing off to an async callback, it leaves
us with a problem of what to do on error.  When things were synchronous,
EXECBUFFER2 would fail with an error code if parsing failed.  When
moving it to async, we needed another way to handle that error and the
solution employed was to set an error on the dma_fence and then trust
that said error gets propagated to the client eventually.  Moving back
to synchronous will help us untangle the fence error propagation mess.

This also reverts most of 0edbb9b ("drm/i915: Move cmd parser
pinning to execbuffer") which is a refactor of some of our allocation
paths for asynchronous parsing.  Now that everything is synchronous, we
don't need it.

v2 (Daniel Vetter):
 - Add stabel Cc and Fixes tag

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Cc: <stable@vger.kernel.org> # v5.6+
Fixes: 9e31c1f ("drm/i915: Propagate errors on awaiting already signaled fences")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210714193419.1459723-2-jason@jlekstrand.net
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
@gfxstrand @gregkh
gfxstrand authored and gregkh committed Aug 8, 2021
1 parent 3cfdd72 commit 15c8463
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 265 deletions.
227 changes: 13 additions & 214 deletions drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
View file
Expand Up @@ -25,10 +25,8 @@
#include "i915_gem_clflush.h"
#include "i915_gem_context.h"
#include "i915_gem_ioctls.h"
#include "i915_sw_fence_work.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
#include "i915_memcpy.h"

struct eb_vma {
struct i915_vma *vma;
Expand Down Expand Up @@ -1456,6 +1454,10 @@ static u32 *reloc_gpu(struct i915_execbuffer *eb,
int err;
struct intel_engine_cs *engine = eb->engine;

/* If we need to copy for the cmdparser, we will stall anyway */
if (eb_use_cmdparser(eb))
return ERR_PTR(-EWOULDBLOCK);

if (!reloc_can_use_engine(engine)) {
engine = engine->gt->engine_class[COPY_ENGINE_CLASS][0];
if (!engine)
Expand Down Expand Up @@ -2372,217 +2374,6 @@ shadow_batch_pin(struct i915_execbuffer *eb,
return vma;
}

struct eb_parse_work {
struct dma_fence_work base;
struct intel_engine_cs *engine;
struct i915_vma *batch;
struct i915_vma *shadow;
struct i915_vma *trampoline;
unsigned long batch_offset;
unsigned long batch_length;
unsigned long *jump_whitelist;
const void *batch_map;
void *shadow_map;
};

static int __eb_parse(struct dma_fence_work *work)
{
struct eb_parse_work *pw = container_of(work, typeof(*pw), base);
int ret;
bool cookie;

cookie = dma_fence_begin_signalling();
ret = intel_engine_cmd_parser(pw->engine,
pw->batch,
pw->batch_offset,
pw->batch_length,
pw->shadow,
pw->jump_whitelist,
pw->shadow_map,
pw->batch_map);
dma_fence_end_signalling(cookie);

return ret;
}

static void __eb_parse_release(struct dma_fence_work *work)
{
struct eb_parse_work *pw = container_of(work, typeof(*pw), base);

if (!IS_ERR_OR_NULL(pw->jump_whitelist))
kfree(pw->jump_whitelist);

if (pw->batch_map)
i915_gem_object_unpin_map(pw->batch->obj);
else
i915_gem_object_unpin_pages(pw->batch->obj);

i915_gem_object_unpin_map(pw->shadow->obj);

if (pw->trampoline)
i915_active_release(&pw->trampoline->active);
i915_active_release(&pw->shadow->active);
i915_active_release(&pw->batch->active);
}

static const struct dma_fence_work_ops eb_parse_ops = {
.name = "eb_parse",
.work = __eb_parse,
.release = __eb_parse_release,
};

static inline int
__parser_mark_active(struct i915_vma *vma,
struct intel_timeline *tl,
struct dma_fence *fence)
{
struct intel_gt_buffer_pool_node *node = vma->private;

return i915_active_ref(&node->active, tl->fence_context, fence);
}

static int
parser_mark_active(struct eb_parse_work *pw, struct intel_timeline *tl)
{
int err;

mutex_lock(&tl->mutex);

err = __parser_mark_active(pw->shadow, tl, &pw->base.dma);
if (err)
goto unlock;

if (pw->trampoline) {
err = __parser_mark_active(pw->trampoline, tl, &pw->base.dma);
if (err)
goto unlock;
}

unlock:
mutex_unlock(&tl->mutex);
return err;
}

static int eb_parse_pipeline(struct i915_execbuffer *eb,
struct i915_vma *shadow,
struct i915_vma *trampoline)
{
struct eb_parse_work *pw;
struct drm_i915_gem_object *batch = eb->batch->vma->obj;
bool needs_clflush;
int err;

GEM_BUG_ON(overflows_type(eb->batch_start_offset, pw->batch_offset));
GEM_BUG_ON(overflows_type(eb->batch_len, pw->batch_length));

pw = kzalloc(sizeof(*pw), GFP_KERNEL);
if (!pw)
return -ENOMEM;

err = i915_active_acquire(&eb->batch->vma->active);
if (err)
goto err_free;

err = i915_active_acquire(&shadow->active);
if (err)
goto err_batch;

if (trampoline) {
err = i915_active_acquire(&trampoline->active);
if (err)
goto err_shadow;
}

pw->shadow_map = i915_gem_object_pin_map(shadow->obj, I915_MAP_WB);
if (IS_ERR(pw->shadow_map)) {
err = PTR_ERR(pw->shadow_map);
goto err_trampoline;
}

needs_clflush =
!(batch->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ);

pw->batch_map = ERR_PTR(-ENODEV);
if (needs_clflush && i915_has_memcpy_from_wc())
pw->batch_map = i915_gem_object_pin_map(batch, I915_MAP_WC);

if (IS_ERR(pw->batch_map)) {
err = i915_gem_object_pin_pages(batch);
if (err)
goto err_unmap_shadow;
pw->batch_map = NULL;
}

pw->jump_whitelist =
intel_engine_cmd_parser_alloc_jump_whitelist(eb->batch_len,
trampoline);
if (IS_ERR(pw->jump_whitelist)) {
err = PTR_ERR(pw->jump_whitelist);
goto err_unmap_batch;
}

dma_fence_work_init(&pw->base, &eb_parse_ops);

pw->engine = eb->engine;
pw->batch = eb->batch->vma;
pw->batch_offset = eb->batch_start_offset;
pw->batch_length = eb->batch_len;
pw->shadow = shadow;
pw->trampoline = trampoline;

/* Mark active refs early for this worker, in case we get interrupted */
err = parser_mark_active(pw, eb->context->timeline);
if (err)
goto err_commit;

err = dma_resv_reserve_shared(pw->batch->resv, 1);
if (err)
goto err_commit;

err = dma_resv_reserve_shared(shadow->resv, 1);
if (err)
goto err_commit;

/* Wait for all writes (and relocs) into the batch to complete */
err = i915_sw_fence_await_reservation(&pw->base.chain,
pw->batch->resv, NULL, false,
0, I915_FENCE_GFP);
if (err < 0)
goto err_commit;

/* Keep the batch alive and unwritten as we parse */
dma_resv_add_shared_fence(pw->batch->resv, &pw->base.dma);

/* Force execution to wait for completion of the parser */
dma_resv_add_excl_fence(shadow->resv, &pw->base.dma);

dma_fence_work_commit_imm(&pw->base);
return 0;

err_commit:
i915_sw_fence_set_error_once(&pw->base.chain, err);
dma_fence_work_commit_imm(&pw->base);
return err;

err_unmap_batch:
if (pw->batch_map)
i915_gem_object_unpin_map(batch);
else
i915_gem_object_unpin_pages(batch);
err_unmap_shadow:
i915_gem_object_unpin_map(shadow->obj);
err_trampoline:
if (trampoline)
i915_active_release(&trampoline->active);
err_shadow:
i915_active_release(&shadow->active);
err_batch:
i915_active_release(&eb->batch->vma->active);
err_free:
kfree(pw);
return err;
}

static struct i915_vma *eb_dispatch_secure(struct i915_execbuffer *eb, struct i915_vma *vma)
{
/*
Expand Down Expand Up @@ -2672,7 +2463,15 @@ static int eb_parse(struct i915_execbuffer *eb)
goto err_trampoline;
}

err = eb_parse_pipeline(eb, shadow, trampoline);
err = dma_resv_reserve_shared(shadow->resv, 1);
if (err)
goto err_trampoline;

err = intel_engine_cmd_parser(eb->engine,
eb->batch->vma,
eb->batch_start_offset,
eb->batch_len,
shadow, trampoline);
if (err)
goto err_unpin_batch;

Expand Down
4 changes: 4 additions & 0 deletions drivers/gpu/drm/i915/gem/selftests/i915_gem_execbuffer.c
View file
Expand Up @@ -125,6 +125,10 @@ static int igt_gpu_reloc(void *arg)
intel_gt_pm_get(&eb.i915->gt);

for_each_uabi_engine(eb.engine, eb.i915) {
if (intel_engine_requires_cmd_parser(eb.engine) ||
intel_engine_using_cmd_parser(eb.engine))
continue;

reloc_cache_init(&eb.reloc_cache, eb.i915);
memset(map, POISON_INUSE, 4096);

Expand Down

0 comments on commit 15c8463

Please sign in to comment.