From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kenneth Graunke Subject: Re: [PATCH 3/3] drm/i915: Force sync command ordering Date: Tue, 11 Oct 2011 12:18:15 -0700 Message-ID: <4E949677.9000600@whitecape.org> References: <1318357916-12661-1-git-send-email-ben@bwidawsk.net> <1318357916-12661-4-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from homiemail-a38.g.dreamhost.com (mailbigip.dreamhost.com [208.97.132.5]) by gabe.freedesktop.org (Postfix) with ESMTP id 0CA7FA0992 for ; Tue, 11 Oct 2011 12:17:15 -0700 (PDT) In-Reply-To: <1318357916-12661-4-git-send-email-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 10/11/2011 11:31 AM, Ben Widawsky wrote: > This will strongly order synchronization commands with respect to 3d > state and 3d primitive commands. AFAIK, this shouldn't impact anything > as these sync commands are all for privileged (or ppgtt) batches only, > so user space should not be relying on this, and the kernel wouldn't be > relying on 3d state or primitive commands. > > This will help when we enable PPGTT, and perhaps this synchronization is > currently useful and I just don't realize it. > > This was found through doc inspection by Ken and applies to Gen6+; > > Reported-by: Kenneth Graunke > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 ++++++-- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 9572e52..1d55842 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -976,6 +976,7 @@ i915_set_constant_offset(struct intel_ring_buffer *ring, int mode) > { > struct drm_device *dev = ring->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t mask = I915_EXEC_CONSTANTS_MASK; > int ret; > > switch (mode) { > @@ -991,6 +992,10 @@ i915_set_constant_offset(struct intel_ring_buffer *ring, int mode) > mode == I915_EXEC_CONSTANTS_REL_SURFACE) > return -EINVAL; > > + /* Never clear this bit because of execbuffer */ > + if (INTEL_INFO(dev)->gen >= 6) > + mask &= ~(INSTPM_FORCE_ORDERING); > + I might do: /* This bit doesn't exist on Gen6+. */ if (INTEL_INFO(dev)->gen >= 6) mask &= ~I915_EXEC_CONSTANTS_REL_SURFACE; ...but, I don't mind either way. > ret = intel_ring_begin(ring, 4); > if (ret) > return ret; > @@ -998,8 +1003,7 @@ i915_set_constant_offset(struct intel_ring_buffer *ring, int mode) > intel_ring_emit(ring, MI_NOOP); > intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > intel_ring_emit(ring, INSTPM); > - intel_ring_emit(ring, > - I915_EXEC_CONSTANTS_MASK << 16 | mode); > + intel_ring_emit(ring, mask << 16 | mode); > intel_ring_advance(ring); > > dev_priv->relative_constants_mode = mode; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 138eae1..51569f2 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -436,6 +436,7 @@ > #define INSTPM_AGPBUSY_DIS (1<<11) /* gen3: when disabled, pending interrupts > will not assert AGPBUSY# and will only > be delivered when out of C3. */ > +#define INSTPM_FORCE_ORDERING (1<<7) /* GEN6+ */ > #define ACTHD 0x020c8 > #define FW_BLC 0x020d8 > #define FW_BLC2 0x020dc > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 0e99589..b1d312f 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -297,6 +297,8 @@ static int init_render_ring(struct intel_ring_buffer *ring) > } > > if (INTEL_INFO(dev)->gen >= 6) { > + I915_WRITE(INSTPM, INSTPM_FORCE_ORDERING << 16 | > + INSTPM_FORCE_ORDERING); > } else if (IS_GEN5(dev)) { > ret = init_pipe_control(ring); > if (ret) I might only enable this on Gen7 for now, unless it actually fixes something on Sandybridge. It's not listed as required for Gen6.