[PATCH 1/2] drm/i915/gem: Excise the per-batch whitelist from the context
Mika Kuoppala
mika.kuoppala at linux.intel.com
Thu Nov 28 11:18:54 UTC 2019
Chris Wilson <chris at chris-wilson.co.uk> writes:
> One does not lightly add a new hidden struct_mutex dependency deep within
> the execbuf bowels! The immediate suspicion in seeing the whitelist
> cached on the context, is that it is intended to be preserved between
> batches, as the kernel is quite adept at caching small allocations
> itself. But no, it's sole purpose is to serialise command submission in
> order to save a kmalloc on a slow, slow path!
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 --
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 7 --
> drivers/gpu/drm/i915/i915_cmd_parser.c | 65 +++++++------------
> 3 files changed, 25 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index c94ac838401a..a179e170c936 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -275,8 +275,6 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> free_engines(rcu_access_pointer(ctx->engines));
> mutex_destroy(&ctx->engines_mutex);
>
> - kfree(ctx->jump_whitelist);
> -
> if (ctx->timeline)
> intel_timeline_put(ctx->timeline);
>
> @@ -584,9 +582,6 @@ __create_context(struct drm_i915_private *i915)
> for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
> ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>
> - ctx->jump_whitelist = NULL;
> - ctx->jump_whitelist_cmds = 0;
> -
> spin_lock(&i915->gem.contexts.lock);
> list_add_tail(&ctx->link, &i915->gem.contexts.list);
> spin_unlock(&i915->gem.contexts.lock);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index c060bc428f49..69df5459c350 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -168,13 +168,6 @@ struct i915_gem_context {
> */
> struct radix_tree_root handles_vma;
>
> - /** jump_whitelist: Bit array for tracking cmds during cmdparsing
> - * Guarded by struct_mutex
> - */
> - unsigned long *jump_whitelist;
> - /** jump_whitelist_cmds: No of cmd slots available */
> - u32 jump_whitelist_cmds;
> -
> /**
> * @name: arbitrary name, used for user debug
> *
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index f24096e27bef..84fe2723be1d 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1310,13 +1310,14 @@ static int check_bbstart(const struct i915_gem_context *ctx,
> u32 *cmd, u32 offset, u32 length,
> u32 batch_len,
> u64 batch_start,
> - u64 shadow_batch_start)
> + u64 shadow_batch_start,
> + unsigned long *jump_whitelist)
> {
> u64 jump_offset, jump_target;
> u32 target_cmd_offset, target_cmd_index;
>
> /* For igt compatibility on older platforms */
> - if (CMDPARSER_USES_GGTT(ctx->i915)) {
> + if (!jump_whitelist) {
> DRM_DEBUG("CMD: Rejecting BB_START for ggtt based submission\n");
> return -EACCES;
> }
> @@ -1352,10 +1353,10 @@ static int check_bbstart(const struct i915_gem_context *ctx,
> if (target_cmd_index == offset)
> return 0;
>
> - if (ctx->jump_whitelist_cmds <= target_cmd_index) {
> - DRM_DEBUG("CMD: Rejecting BB_START - truncated whitelist array\n");
> - return -EINVAL;
> - } else if (!test_bit(target_cmd_index, ctx->jump_whitelist)) {
> + if (IS_ERR(jump_whitelist))
> + return PTR_ERR(jump_whitelist);
Failing in here is nicer. And bringing the whitelist in scope is a nice
too. This would have saved us from few headaches in past.
Thanks,
Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> +
> + if (!test_bit(target_cmd_index, jump_whitelist)) {
> DRM_DEBUG("CMD: BB_START to 0x%llx not a previously executed cmd\n",
> jump_target);
> return -EINVAL;
> @@ -1364,40 +1365,19 @@ static int check_bbstart(const struct i915_gem_context *ctx,
> return 0;
> }
>
> -static void init_whitelist(struct i915_gem_context *ctx, u32 batch_len)
> +static unsigned long *
> +alloc_whitelist(struct drm_i915_private *i915, u32 batch_len)
> {
> - const u32 batch_cmds = DIV_ROUND_UP(batch_len, sizeof(u32));
> - const u32 exact_size = BITS_TO_LONGS(batch_cmds);
> - u32 next_size = BITS_TO_LONGS(roundup_pow_of_two(batch_cmds));
> - unsigned long *next_whitelist;
> -
> - if (CMDPARSER_USES_GGTT(ctx->i915))
> - return;
> -
> - if (batch_cmds <= ctx->jump_whitelist_cmds) {
> - bitmap_zero(ctx->jump_whitelist, batch_cmds);
> - return;
> - }
> -
> -again:
> - next_whitelist = kcalloc(next_size, sizeof(long), GFP_KERNEL);
> - if (next_whitelist) {
> - kfree(ctx->jump_whitelist);
> - ctx->jump_whitelist = next_whitelist;
> - ctx->jump_whitelist_cmds =
> - next_size * BITS_PER_BYTE * sizeof(long);
> - return;
> - }
> + unsigned long *jmp;
>
> - if (next_size > exact_size) {
> - next_size = exact_size;
> - goto again;
> - }
> + if (CMDPARSER_USES_GGTT(i915))
> + return NULL;
>
> - DRM_DEBUG("CMD: Failed to extend whitelist. BB_START may be disallowed\n");
> - bitmap_zero(ctx->jump_whitelist, ctx->jump_whitelist_cmds);
> + jmp = bitmap_zalloc(DIV_ROUND_UP(batch_len, sizeof(u32)), GFP_KERNEL);
> + if (!jmp)
> + return ERR_PTR(-ENOMEM);
>
> - return;
> + return jmp;
> }
>
> #define LENGTH_BIAS 2
> @@ -1433,6 +1413,7 @@ int intel_engine_cmd_parser(struct i915_gem_context *ctx,
> struct drm_i915_cmd_descriptor default_desc = noop_desc;
> const struct drm_i915_cmd_descriptor *desc = &default_desc;
> bool needs_clflush_after = false;
> + unsigned long *jump_whitelist;
> int ret = 0;
>
> cmd = copy_batch(shadow_batch_obj, batch_obj,
> @@ -1443,7 +1424,8 @@ int intel_engine_cmd_parser(struct i915_gem_context *ctx,
> return PTR_ERR(cmd);
> }
>
> - init_whitelist(ctx, batch_len);
> + /* Defer failure until attempted use */
> + jump_whitelist = alloc_whitelist(ctx->i915, batch_len);
>
> /*
> * We use the batch length as size because the shadow object is as
> @@ -1487,15 +1469,16 @@ int intel_engine_cmd_parser(struct i915_gem_context *ctx,
> if (desc->cmd.value == MI_BATCH_BUFFER_START) {
> ret = check_bbstart(ctx, cmd, offset, length,
> batch_len, batch_start,
> - shadow_batch_start);
> + shadow_batch_start,
> + jump_whitelist);
>
> if (ret)
> goto err;
> break;
> }
>
> - if (ctx->jump_whitelist_cmds > offset)
> - set_bit(offset, ctx->jump_whitelist);
> + if (!IS_ERR_OR_NULL(jump_whitelist))
> + set_bit(offset, jump_whitelist);
>
> cmd += length;
> offset += length;
> @@ -1513,6 +1496,8 @@ int intel_engine_cmd_parser(struct i915_gem_context *ctx,
> }
>
> err:
> + if (!IS_ERR_OR_NULL(jump_whitelist))
> + kfree(jump_whitelist);
> i915_gem_object_unpin_map(shadow_batch_obj);
> return ret;
> }
> --
> 2.24.0
>
> _______________________________________________
> Intel-gfx-trybot mailing list
> Intel-gfx-trybot at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx-trybot
More information about the Intel-gfx-trybot
mailing list