[Intel-gfx] [PATCH] drm/i915: Abort command parsing for chained batches

[Volkin, Bradley D]

[snip]

On Tue, Oct 21, 2014 at 08:50:33AM -0700, Daniel Vetter wrote:
> On Thu, Oct 16, 2014 at 12:24:42PM -0700, bradley.d.volkin at intel.com wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 1a0611b..1ed5702 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1368,17 +1368,19 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  				      batch_obj,
> >  				      args->batch_start_offset,
> >  				      file->is_master);
> > -		if (ret)
> > -			goto err;
> > -
> > -		/*
> > -		 * XXX: Actually do this when enabling batch copy...
> > -		 *
> > -		 * Set the DISPATCH_SECURE bit to remove the NON_SECURE bit
> > -		 * from MI_BATCH_BUFFER_START commands issued in the
> > -		 * dispatch_execbuffer implementations. We specifically don't
> > -		 * want that set when the command parser is enabled.
> > -		 */
> > +		if (ret) {
> > +			if (ret != -EACCES)
> > +				goto err;
> > +		} else {
> > +			/*
> > +			 * XXX: Actually do this when enabling batch copy...
> > +			 *
> > +			 * Set the DISPATCH_SECURE bit to remove the NON_SECURE bit
> > +			 * from MI_BATCH_BUFFER_START commands issued in the
> > +			 * dispatch_execbuffer implementations. We specifically don't
> > +			 * want that set when the command parser is enabled.
> > +			 */
> > +		}
> 
> Tbh this hunk here confuses me ... Why do we need to change anything here?

Yeah, it makes more sense with the batch copy code, it's just that this
patch has to go in before the patch where we set I915_DISPATCH_SECURE.
The final logic basically goes like this:

ret = i915_parse_cmds()
if ret == 0
    dispatch shadow_batch_obj, flags = I915_DISPATCH_SECURE
else if ret == -EACCES // i.e. i915_parse_cmds() found an MI_BB_S
    dispatch batch_obj, flags = 0
else
    return error

The point is that there's a restriction that chained batches must have
the AddressSpace bit set to the same value as the parent batch (i.e.
GGTT when batch copy is present). But because of the way libva uses
chained batches we can't parse or copy the chained batch to safely put
it into GGTT. So we fall back to dispatching the userspace-supplied
batch from PPGTT. I should probably have mentioned this restriction in
the commit message.

As you suggest below, we could instead start to conditionally check
batches based on the flag from userspace. However, I thought we had
decided not to take that approach in general. Mesa already implements
all of the code that they need the command parser for, with a runtime
check as to whether hardware will nop their LRI, etc commands. So if
we run the parser on all batches, then once we switch to enabling mode
features magically work for them. Also, the I915_EXEC_SECURE flag is
currently root-only, so there's a bit of a semantic/API/whatever change
that we'd have to make there, or add a new flag I suppose. Maybe not a
big deal, but I think that the choice of running the parser on all
batches is the right direction.

Brad

> 
> And since we we currently scan batches unconditionally: Shouldn't we
> filter out the -EACCESS at a higher level?
> 
> In the end I imagine the logic will be:
> 
> a) Userspace asks for secure batch
>  -> Scan and reject or copy and run.
> b) Userspace asks for normal batch
>  -> Don't scan, but run without additional hw privs.
> 
> Or am I again completely missing the point?
> 
> Thanks, Daniel
> 
> >  	}
> >  
> >  	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch