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

[Chris Wilson]

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 at chris-wilson.co.uk>
> 
> <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