Re: [PATCH 3/3] drm/i915: Force sync command ordering

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, 11 Oct 2011 12:18:15 -0700
Kenneth Graunke <kenneth@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 <kenneth@whitecape.org>
> > Signed-off-by: Ben Widawsky <ben@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