Re: [PATCH 00/13] Gen7 batch buffer command parser

[Jani Nikula <jani.nikula@linux.intel.com>]

Hi Bradley -

I've now rather meticulously reviewed what *is* in the command and
register tables, and didn't spot any obvious errors.

There is still the review of what is *not* in the command and register
tables, but perhaps should be. This is important for two reasons:

1) Any command that should fail that's missing from the tables will
   pass.

2) Any command that has a non-standard length encoding that's missing
   from the tables will confuse the parser. Going out-of-sync with the
   commands may fail a good batch, or, worse, a command that should fail
   may pass.

I'm thinking there's a certain fragility in this. In a sense, it would
be better to whitelist everything, and fail everything else, to err on
the safe side. But I'm guessing that's not really feasible.

Part of the fragility is the immense tediousness of the review... I know
it gets easier with the incremental updates, but this first series is
not fun. (You know, you could have planted a bug to reward me! Or did I
miss it?! ;)


BR,
Jani.


On Tue, 18 Feb 2014, bradley.d.volkin@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
>
> Certain OpenGL features (e.g. transform feedback, performance monitoring)
> require userspace code to submit batches containing commands such as
> MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some
> generations of the hardware will noop these commands in "unsecure" batches
> (which includes all userspace batches submitted via i915) even though the
> commands may be safe and represent the intended programming model of the device.
>
> This series introduces a software command parser similar in operation to the
> command parsing done in hardware for unsecure batches. However, the software
> parser allows some operations that would be noop'd by hardware, if the parser
> determines the operation is safe, and submits the batch as "secure" to prevent
> hardware parsing. Currently the series implements this on IVB and HSW.
>
> The series has one piece of prep work, one patch for the parser logic, and a
> handful of patches to fill out the tables which drive the parser. There are
> follow-up patches to libdrm and to i-g-t. The i-g-t tests are basic and do not
> test all of the commands used by the parser on the assumption that I'm likely
> to make the same mistakes in both the parser and the test.
>
> I've previously run the i-g-t gem_* tests, the piglit quick tests, and generally
> used Ubuntu 13.10 IVB and HSW systems with the parser running. Aside from a
> failure described below, I did not see any regressions.
>
> At this point there are a couple of required/potential improvements.
>
> 1) Chained batches. The parser currently allows MI_BATCH_BUFFER_START commands
>    in userspace batches without parsing them. The media driver uses chained
>    batches, so a solution is required. I'm still working through the
>    requirements but don't want to continue delaying the review process for what
>    I have so far.
> 2) Command buffer copy. To avoid CPU modifications to buffers after parsing, and
>    to avoid GPU modifications to buffers via EUs or commands in the batch, we
>    should copy the userspace batch buffer to memory that userspace does not
>    have access to, map it into GGTT, and execute that batch buffer. I have a
>    sense of how to do this for 1st-level batches, but it may need changes to
>    tie in with the chained batch parsing, so I've again held off.
> 3) Coherency. I've previously found a coherency issue on VLV when reading the
>    batch buffer from the CPU during execbuffer2. Userspace writes the batch via
>    pwrite fast path before calling execbuffer2. The parser reads stale data.
>    This works fine on IVB and HSW, so I believe it's an LLC vs. non-LLC issue.
>    It's possible that the shmem pread refactoring fixes this, I just have not
>    been able to retest due to lack of a VLV system.
>
> v2:
> - Significantly reorder series
> - Scan secure batches (i.e. I915_EXEC_SECURE)
> - Check that parser tables are sorted during init
> - Fixed gem_cpu_reloc regression
> - HAS_CMD_PARSER -> CMD_PARSER_VERSION getparam
> - Additional tests
>
> v3:
> - Don't actually send batches as secure yet
> - Improved documentation and commenting
> - Many other small cleanups throughout
>
> Brad Volkin (13):
>   drm/i915: Refactor shmem pread setup
>   drm/i915: Implement command buffer parsing logic
>   drm/i915: Initial command parser table definitions
>   drm/i915: Reject privileged commands
>   drm/i915: Allow some privileged commands from master
>   drm/i915: Add register whitelists for mesa
>   drm/i915: Add register whitelist for DRM master
>   drm/i915: Enable register whitelist checks
>   drm/i915: Reject commands that explicitly generate interrupts
>   drm/i915: Enable PPGTT command parser checks
>   drm/i915: Reject commands that would store to global HWS page
>   drm/i915: Add a CMD_PARSER_VERSION getparam
>   drm/i915: Enable command parsing by default
>
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/i915_cmd_parser.c     | 918 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_dma.c            |   3 +
>  drivers/gpu/drm/i915/i915_drv.h            | 103 ++++
>  drivers/gpu/drm/i915/i915_gem.c            |  51 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  18 +
>  drivers/gpu/drm/i915/i915_params.c         |   5 +
>  drivers/gpu/drm/i915/i915_reg.h            |  96 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |   2 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  32 +
>  include/uapi/drm/i915_drm.h                |   1 +
>  11 files changed, 1216 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c
>
> -- 
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center