drm/i915: Abort command parsing for chained batches

Commit Message

[e-mail] Oct. 16, 2014, 7:24 p.m. UTC

From: Brad Volkin <[e-mail]>

libva uses chained batch buffers in a way that the command parser
can't generally handle. Fortunately, libva doesn't need to write
registers from batch buffers in the way that mesa does, so this
patch causes the driver to fall back to non-secure dispatch if
the parser detects a chained batch buffer.

Testcase: igt/gem_exec_parse/chained-batch
Signed-off-by: Brad Volkin <[e-mail]>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c     | 18 +++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 24 +++++++++++++-----------
 2 files changed, 30 insertions(+), 12 deletions(-)

Comments

Shuang He Oct. 16, 2014, 7:26 p.m. UTC | #1

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: [e-mail])
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=271/271->270/271
PNV: pass/total=269/271->271/271
ILK: pass/total=269/271->271/271
IVB: pass/total=271/271->271/271
SNB: pass/total=271/271->271/271
HSW: pass/total=271/271->271/271
BDW: pass/total=270/271->271/271
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly->result_with_patch_applied
BYT: Intel_gpu_tools, igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible, PASS->DMESG_WARN
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-forked, TIMEOUT->PASS
PNV: Intel_gpu_tools, igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-forked, TIMEOUT->PASS
ILK: Intel_gpu_tools, igt_kms_pipe_crc_basic_bad-nb-words-3, DMESG_WARN->PASS
ILK: Intel_gpu_tools, igt_kms_setmode_invalid-clone-single-crtc, DMESG_WARN->PASS
BDW: Intel_gpu_tools, igt_drv_missed_irq_hang, DMESG_WARN->PASS

Daniel Vetter Oct. 21, 2014, 3:50 p.m. UTC | #2

On Thu, Oct 16, 2014 at 12:24:42PM -0700, [e-mail] wrote:
> From: Brad Volkin <[e-mail]>
> 
> libva uses chained batch buffers in a way that the command parser
> can't generally handle. Fortunately, libva doesn't need to write
> registers from batch buffers in the way that mesa does, so this
> patch causes the driver to fall back to non-secure dispatch if
> the parser detects a chained batch buffer.
> 
> Testcase: igt/gem_exec_parse/chained-batch
> Signed-off-by: Brad Volkin <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c     | 18 +++++++++++++++++-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 24 +++++++++++++-----------
>  2 files changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 86b3ae0..ef38915 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -138,6 +138,11 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
>  			.mask = MI_GLOBAL_GTT,
>  			.expected = 0,
>  	      }},						       ),
> +	/*
> +	 * MI_BATCH_BUFFER_START requires some special handling. It's not
> +	 * really a 'skip' action but it doesn't seem like it's worth adding
> +	 * a new action. See i915_parse_cmds().
> +	 */
>  	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
>  };
>  
> @@ -955,7 +960,8 @@ static bool check_cmd(const struct intel_engine_cs *ring,
>   * Parses the specified batch buffer looking for privilege violations as
>   * described in the overview.
>   *
> - * Return: non-zero if the parser finds violations or otherwise fails
> + * Return: non-zero if the parser finds violations or otherwise fails; -EACCES
> + * if the batch appears legal but should use hardware parsing
>   */
>  int i915_parse_cmds(struct intel_engine_cs *ring,
>  		    struct drm_i915_gem_object *batch_obj,
> @@ -1002,6 +1008,16 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
>  			break;
>  		}
>  
> +		/*
> +		 * If the batch buffer contains a chained batch, return an
> +		 * error that tells the caller to abort and dispatch the
> +		 * workload as a non-secure batch.
> +		 */
> +		if (desc->cmd.value == MI_BATCH_BUFFER_START) {
> +			ret = -EACCES;
> +			break;
> +		}
> +
>  		if (desc->flags & CMD_DESC_FIXED)
>  			length = desc->length.fixed;
>  		else
> 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?

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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[e-mail] Oct. 22, 2014, 4:04 p.m. UTC | #3

[snip]

On Tue, Oct 21, 2014 at 08:50:33AM -0700, Daniel Vetter wrote:
> On Thu, Oct 16, 2014 at 12:24:42PM -0700, [e-mail] 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
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Daniel Vetter Oct. 23, 2014, 12:31 p.m. UTC | #4

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, [e-mail] 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?
> 
> 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.

Hm yeah, that's a good reason to just go ahead. If people still hate the
overhead too much we can just add more flags to execbuf ;-)
-Daniel

[e-mail] Oct. 23, 2014, 3:52 p.m. UTC | #5

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, [e-mail] 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.

Brad

> > 
> > 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.
> 
> Hm yeah, that's a good reason to just go ahead. If people still hate the
> overhead too much we can just add more flags to execbuf ;-)
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

[e-mail] Oct. 24, 2014, 5:17 p.m. UTC | #6

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, [e-mail] 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

Daniel Vetter Oct. 27, 2014, 8:58 a.m. UTC | #7

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, [e-mail] 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

Patch

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 86b3ae0..ef38915 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -138,6 +138,11 @@  static const struct drm_i915_cmd_descriptor common_cmds[] = {
 			.mask = MI_GLOBAL_GTT,
 			.expected = 0,
 	      }},						       ),
+	/*
+	 * MI_BATCH_BUFFER_START requires some special handling. It's not
+	 * really a 'skip' action but it doesn't seem like it's worth adding
+	 * a new action. See i915_parse_cmds().
+	 */
 	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
 };
 
@@ -955,7 +960,8 @@  static bool check_cmd(const struct intel_engine_cs *ring,
  * Parses the specified batch buffer looking for privilege violations as
  * described in the overview.
  *
- * Return: non-zero if the parser finds violations or otherwise fails
+ * Return: non-zero if the parser finds violations or otherwise fails; -EACCES
+ * if the batch appears legal but should use hardware parsing
  */
 int i915_parse_cmds(struct intel_engine_cs *ring,
 		    struct drm_i915_gem_object *batch_obj,
@@ -1002,6 +1008,16 @@  int i915_parse_cmds(struct intel_engine_cs *ring,
 			break;
 		}
 
+		/*
+		 * If the batch buffer contains a chained batch, return an
+		 * error that tells the caller to abort and dispatch the
+		 * workload as a non-secure batch.
+		 */
+		if (desc->cmd.value == MI_BATCH_BUFFER_START) {
+			ret = -EACCES;
+			break;
+		}
+
 		if (desc->flags & CMD_DESC_FIXED)
 			length = desc->length.fixed;
 		else
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.
+			 */
+		}
 	}
 
 	/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure