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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 11, 2014 at 05:41:06AM -0700, Jani Nikula wrote:
> 
> Hi Bradley -
> 
> I've now rather meticulously reviewed what *is* in the command and
> register tables, and didn't spot any obvious errors.

Thanks Jani! I know it's a huge pain, so I appreciate you taking the
time for it.

> 
> 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.

There was some discussion on these issues previously. I think Daniel's
response here covers it.

http://lists.freedesktop.org/archives/intel-gfx/2014-January/039209.html

> 
> 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?! ;)

Yes, I'm afraid that I've taken all the fun for myself :p

Brad

> 
> 
> BR,
> Jani.
> 
> 
> On Tue, 18 Feb 2014, bradley.d.volkin@xxxxxxxxx wrote:
> > From: Brad Volkin <bradley.d.volkin@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux