Re: [PATCH 2/6] drm/i915/vlv: Added a rendering specific Hw WA 'WaSendDummy3dPrimitveAfterSetContext'

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 24, 2014 at 12:19:20PM +0530, sourab.gupta@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This workaround is needed on VLV for the HW context feature.
> It is used after adding the mi_set_context command in ring buffer
> for Hw context switch. As per the spec
> "The software must send a pipe_control with a CS stall and a post sync
> operation and then a dummy DRAW after every MI_SET_CONTEXT and after any
> PIPELINE_SELECT that is enabling 3D mode".
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 65 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  3 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  9 +++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>  4 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6043062..544fc2d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -584,6 +584,58 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  	return ctx;
>  }
>  
> +static inline void
> +mi_set_context_dummy3d_prim_wa(struct intel_ring_buffer *ring)
> +{
> +	u32 scratch_addr;
> +	u32 flags = 0;
> +
> +	/*
> +	 * Check if we have the scratch page allocated needed
> +	 * for the Pipe Control command, otherwise don't apply
> +	 * the dummmy 3d primitive workaround & add NOOPs instead
> +	 */
> +	if (get_pipe_control_scratch_addr(ring)) {

This check here looks like it papers over a bug (or at least rather
serious init ordering confusion). Can you please rework this to avoid this
condition?

> +		/* Actual scratch location is at 128 bytes offset */
> +		scratch_addr = get_pipe_control_scratch_addr(ring) + 128;
> +
> +		/*
> +		 * WaSendDummy3dPrimitveAfterSetContext:vlv
> +		 * Software must send a pipe_control with a CS stall
> +		 * and a post sync operation and then a dummy DRAW after
> +		 * every MI_SET_CONTEXT and after any PIPELINE_SELECT that
> +		 * is enabling 3D mode. A dummy draw is a 3DPRIMITIVE command
> +		 * with Indirect Parameter Enable set to 0, UAV Coherency
> +		 * Required set to 0, Predicate Enable set to 0,
> +		 * End Offset Enable set to 0, and Vertex Count Per Instance
> +		 * set to 0, All other parameters are a don't care.
> +		 */
> +
> +		/*
> +		 * Add a pipe control with CS Stall and postsync op
> +		 * before dummy 3D_PRIMITIVE
> +		 */
> +		flags |= PIPE_CONTROL_QW_WRITE | PIPE_CONTROL_CS_STALL;
> +		intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4));
> +		intel_ring_emit(ring, flags);
> +		intel_ring_emit(ring, scratch_addr | PIPE_CONTROL_GLOBAL_GTT);
> +		intel_ring_emit(ring, 0);
> +
> +		/* Add a dummy 3D_PRIMITVE */
> +		intel_ring_emit(ring, GFX_OP_3DPRIMITIVE());
> +		intel_ring_emit(ring, 4); /* PrimTopoType*/
> +		intel_ring_emit(ring, 0); /* VertexCountPerInstance */
> +		intel_ring_emit(ring, 0); /* StartVertexLocation */
> +		intel_ring_emit(ring, 0); /* InstanceCount */
> +		intel_ring_emit(ring, 0); /* StartInstanceLocation */
> +		intel_ring_emit(ring, 0); /* BaseVertexLocation  */

Is this save even in a pristine context when the gpu comes right out of
reset? E.g. on resume or after gpu reset. We've had tons of fun in this
area, see e.g. also Ben's latest bdw patch.
-Daniel

> +	} else {
> +		int i;
> +		for (i = 0; i < 11; i++)
> +			intel_ring_emit(ring, MI_NOOP);
> +	}
> +}
> +
>  static inline int
>  mi_set_context(struct intel_ring_buffer *ring,
>  	       struct i915_hw_context *new_context,
> @@ -602,7 +654,10 @@ mi_set_context(struct intel_ring_buffer *ring,
>  			return ret;
>  	}
>  
> -	ret = intel_ring_begin(ring, 6);
> +	if (IS_VALLEYVIEW(ring->dev))
> +		ret = intel_ring_begin(ring, 6+4+8);
> +	else
> +		ret = intel_ring_begin(ring, 6);
>  	if (ret)
>  		return ret;
>  
> @@ -626,7 +681,13 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	intel_ring_emit(ring, MI_NOOP);
>  
>  	if (IS_GEN7(ring->dev))
> -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
> +		if (IS_VALLEYVIEW(ring->dev)) {
> +			/* FIXME, should also apply to ivb */
> +			mi_set_context_dummy3d_prim_wa(ring);
> +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
> +			intel_ring_emit(ring, MI_NOOP);
> +		} else
> +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
>  	else
>  		intel_ring_emit(ring, MI_NOOP);
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index adcb9c7..fd25e8e4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -348,6 +348,9 @@
>  #define   PIPE_CONTROL_DEPTH_CACHE_FLUSH		(1<<0)
>  #define   PIPE_CONTROL_GLOBAL_GTT (1<<2) /* in addr dword */
>  
> +#define GFX_OP_3DPRIMITIVE()              \
> +	((0x3<<29)|(0x3<<27)|(0x3<<24)|       \
> +	 (0x0<<16)|(0x0<<10)|(0x0<<8)|(7-2))
>  
>  /*
>   * Reset registers
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2812384..98ddfec 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -560,6 +560,15 @@ err:
>  	return ret;
>  }
>  
> +u32
> +get_pipe_control_scratch_addr(struct intel_ring_buffer *ring)
> +{
> +	if (ring->scratch.obj == NULL)
> +		return 0;
> +
> +	return ring->scratch.gtt_offset;
> +}
> +
>  static int init_render_ring(struct intel_ring_buffer *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f11ceb2..640c56d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -294,6 +294,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev);
>  
>  u32 intel_ring_get_active_head(struct intel_ring_buffer *ring);
>  void intel_ring_setup_status_page(struct intel_ring_buffer *ring);
> +u32 get_pipe_control_scratch_addr(struct intel_ring_buffer *ring);
>  
>  static inline u32 intel_ring_get_tail(struct intel_ring_buffer *ring)
>  {
> -- 
> 1.8.5.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch