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

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

Hi Bradley -

Apologies for my procrastination with the review; I don't easily recall
as tedious a review as the command and register tables. And I sure have
reviewed a lot of miserable stuff in the past.

Most infuriatingly, I did not find a single real bug in the code!

I think we'll need to automate some things going forward, for example
listing the non-conforming length encoding with Damien's tools for cross
checking.

There are a few subtle points we need to discuss (separate mails
internally) but all in all this series is:

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



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