Quoting Joonas Lahtinen (2019-12-11 11:27:17)
> 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@xxxxxxxxxxxxxxxxxx>
>
> <SNIP>
>
> > #define NUM_EXTRA 2
>
> Looks like BOs, we should have a more descriptive name.
It's not just bo, it's the execbuf state.
> #define NUM_KERNEL_BUFFERS?
I'll go one better, and drop it since it ended up not being used.
> > @@ -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.
Fwiw, I kept the line split to match the original eb->engine->emit_bb_start() call.
You can't see that in the diff
I'll replace the magic 8 with the even more magic 0 :-p
The rest will take some time to polish up.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
