Re: [PATCH] drm/i915: Abort command parsing for chained batches

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 24, 2014 at 10:17:51AM -0700, Volkin, Bradley D wrote:
> 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@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.

Oh indeed, now I see clearer ;-)

> 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 {
> 	}

I've merged the patch as-is with an improved commit message to explain
this a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx