On Tue, 11 Oct 2011 12:18:15 -0700
Kenneth Graunke <kenn...@whitecape.org> wrote:
> 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 <kenn...@whitecape.org>
> > Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> > ---
> > 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.
I prefer this as well.
>
> > 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.
I would prefer to keep for gen6 for two reasons:
1 - paranoia
2 - user space is going to be toggling a bit which it doesn't mean to
toggle and that has a who knows what impact on gen6.
Ben
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
