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

[Kenneth Graunke <kenneth@whitecape.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 <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.

>  			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.