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
