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

[Volkin, Bradley D]

On Thu, Oct 23, 2014 at 08:52:59AM -0700, Volkin, Bradley D wrote:
> On Thu, Oct 23, 2014 at 05:31:12AM -0700, Daniel Vetter wrote:
> > On Wed, Oct 22, 2014 at 09:04:32AM -0700, Volkin, Bradley D wrote:
> > > [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.
> > 
> > Yeah I've figured that this makes more sense with the actual batch copy.
> > Hence the suggestion to just leave out this hunk for now - that shouldn't
> > have a functional impact at this stage if I'm not again blind?
> 
> Oh, I see. Yeah, we can probably leave this part out of this patch and
> just put it in with batch copy. I'll do a quick test and send an updated
> patch if it looks good.

I take that back. Reading this again, with appropriate levels of coffee in
my system this time :), there is a functional impact to this hunk.

We need the

	if (ret) {
		if (ret != -EACCES)
			goto err;
	}

piece because i915_parse_cmds() will now return -EACCES for libva batches.
If we don't filter -EACCES and instead propagate the error, we're basically
rejecting all of their batches. Not exactly what we wanted. Beyond that, yes
the other behavioral differences only come in with the batch copy series.

We obviously don't need the empty else clause though. So if you agree with
the patch otherwise then I'd say frob the else clause however you like when
applying and I'll rebase the batch copy patches as needed. I'd prefer that
you leave the -EACCES filter as written though because the final logic is

	if ret {
		if -EACCES
		else
	} else {
	}

Thanks,
Brad