[Intel-gfx] [PATCH 6/8] drm/i915: Prepare gen7 cmdparser for async execution

[Joonas Lahtinen]

Quoting Chris Wilson (2019-12-07 19:01:08)
> The gen7 cmdparser is primarily a promotion-based system to allow access
> to additional registers beyond the HW validation, and allows fallback to
> normal execution of the user batch buffer if valid and requires
> chaining. In the next patch, we will do the cmdparser validation in the
> pipeline asynchronously and so at the point of request construction we
> will not know if we want to execute the privileged and validated batch,
> or the original user batch. The solution employed here is to execute
> both batches, one with raised privileges and one as normal. This is
> because the gen7 MI_BATCH_BUFFER_START command cannot change privilege
> level within a batch and must strictly use the current privilege level
> (or undefined behaviour kills the GPU). So in order to execute the
> original batch, we need a second non-priviledged batch buffer chain from
> the ring, i.e. we need to emit two batches for each user batch. Inside
> the two batches we determine which one should actually execute, we
> provide a conditional trampoline to call the original batch.

It's only a single batch executed twice from different offsets. I would
rephrase the commit message to reflect that.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>

<SNIP>

> #define NUM_EXTRA 2

Looks like BOs, we should have a more descriptive name.

#define NUM_KERNEL_BUFFERS?

> @@ -1985,59 +1970,80 @@ shadow_batch_pin(struct i915_execbuffer *eb, struct drm_i915_gem_object *obj)
>  static int eb_parse(struct i915_execbuffer *eb)
>  {
>         struct intel_engine_pool_node *pool;
> -       struct i915_vma *vma;
> +       struct i915_vma *shadow, *trampoline;
> +       unsigned int len;
>         int err;
>  
>         if (!eb_use_cmdparser(eb))
>                 return 0;
>  
> -       pool = intel_engine_get_pool(eb->engine, eb->batch_len);
> +       len = eb->batch_len;
> +       if (!CMDPARSER_USES_GGTT(eb->i915)) {
> +               /*
> +                * PPGTT backed shadow buffers must be mapped RO, to prevent
> +                * post-scan tampering
> +                */
> +               if (!eb->context->vm->has_read_only) {
> +                       DRM_DEBUG("Cannot prevent post-scan tampering without RO capable vm\n");
> +                       return -EINVAL;
> +               }
> +       } else {
> +               len += 8;

Magic number. #define TRAMPOLINE_SOMETHING ?

> @@ -2089,6 +2095,16 @@ static int eb_submit(struct i915_execbuffer *eb)
>         if (err)
>                 return err;
>  
> +       if (eb->trampoline) {
> +               GEM_BUG_ON(eb->batch_start_offset);
> +               err = eb->engine->emit_bb_start(eb->request,
> +                                               eb->trampoline->node.start +
> +                                               eb->batch_len,
> +                                               8, 0);

Magic 8 detected.

I'd emphasis that we're jumping to the end, either by computing start +
batch_len separately or bringing them to same line.

> @@ -1504,6 +1504,33 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>                 }
>         } while (1);
>  
> +       if (!jump_whitelist) { /* setup up the trampoline for chaining */

I think we should hoist the CMDPARSER_USES_GGTT check from
alloc_whitelist function. It's quite misleading now, and I
spent quite some time wondering why we would only do this on
out of memory.

Especially as there is a comment "defer failure until attempted use".

> +               cmd = page_mask_bits(shadow->obj->mm.mapping);
> +               if (!ret) {
> +                       cmd += batch_length / sizeof(*cmd);

This could use sharing the offset through variable/helper function
to tie this to be overwriting the trampoline jump.

Helper func maybe to compute the offset of trampoline, even if it
happens to be right at the end.

> +                       *cmd = MI_BATCH_BUFFER_END;
> +               } else {
> +                       *cmd = MI_BATCH_BUFFER_END;
> +                       cmd += batch_length / sizeof(*cmd);

Again using the helper function would help tracing that each BB
is jumped to twice, and this is about the second jump.

> +
> +                       if (ret == -EACCES) {
> +                               u32 bbs;
> +
> +                               bbs = MI_BATCH_BUFFER_START;
> +                               bbs |= MI_BATCH_NON_SECURE_I965;
> +                               if (IS_HASWELL(engine->i915))
> +                                       bbs |= MI_BATCH_NON_SECURE_HSW;
> +
> +                               cmd[0] = bbs;
> +                               cmd[1] = batch_addr;

__{gen6,hsw}_bb_start helper?

With the magics removed this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Regards, Joonas