On Thu, Oct 16, 2014 at 12:24:42PM -0700, bradley.d.volkin@xxxxxxxxx wrote:
> From: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
>
> 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 <bradley.d.volkin@xxxxxxxxx>
> ---
> 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
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
