From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 00/13] Gen7 batch buffer command parser Date: Wed, 5 Mar 2014 11:46:35 +0100 Message-ID: <20140305104635.GY17001@phenom.ffwll.local> References: <1392747357-25703-1-git-send-email-bradley.d.volkin@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f51.google.com (mail-ee0-f51.google.com [74.125.83.51]) by gabe.freedesktop.org (Postfix) with ESMTP id 834D1FA304 for ; Wed, 5 Mar 2014 02:46:41 -0800 (PST) Received: by mail-ee0-f51.google.com with SMTP id c13so359609eek.10 for ; Wed, 05 Mar 2014 02:46:40 -0800 (PST) Content-Disposition: inline In-Reply-To: <1392747357-25703-1-git-send-email-bradley.d.volkin@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: bradley.d.volkin@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Feb 18, 2014 at 10:15:44AM -0800, bradley.d.volkin@intel.com wrote: > From: Brad Volkin > > 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. Is it still true that we need to test this on vlv? The shmem_pread path really should have fixed this ... Otherwise I think this looks ready to go in, I'll pester Jani for the review. Thanks, Daniel > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch