[Intel-gfx] [PATCH 22/67] drm/i915/cnl: Add RT cache flush pipe control w/a

[Tvrtko Ursulin]

On 06/04/2017 20:15, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky at intel.com>

Some commit message is needed, maybe just copy&paste the comment from below.

> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 058d5f2..f2d57b4 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1389,7 +1389,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>   	struct intel_engine_cs *engine = request->engine;
>   	u32 scratch_addr =
>   		i915_ggtt_offset(engine->scratch) + 2 * CACHELINE_BYTES;
> -	bool vf_flush_wa = false, dc_flush_wa = false;
> +	bool vf_flush_wa = false, dc_flush_wa = false, rt_flush_wa = false;
>   	u32 *cs, flags = 0;
>   	int len;
>   
> @@ -1400,6 +1400,15 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>   		flags |= PIPE_CONTROL_DEPTH_CACHE_FLUSH;
>   		flags |= PIPE_CONTROL_DC_FLUSH_ENABLE;
>   		flags |= PIPE_CONTROL_FLUSH_ENABLE;
> +
> +		/*
> +		 * "Before sending a PIPE_CONTROL command with bit 12 set, SW
> +		 * must issue another PIPE_CONTROL with Render Target Cache
> +		 * Flush Enable (bit 12) = 0 and Pipe Control Flush Enable (bit
> +		 * 7) = 1."
> +		 */

Is there a "Wa***:platform" designation for this one?

> +		if (IS_GEN10(request->i915))
> +			rt_flush_wa = true;
>   	}
>   
>   	if (mode & EMIT_INVALIDATE) {
> @@ -1426,7 +1435,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>   
>   	len = 6;
>   
> -	if (vf_flush_wa)
> +	if (vf_flush_wa || rt_flush_wa)
>   		len += 6;
>   
>   	if (dc_flush_wa)
> @@ -1436,8 +1445,10 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	if (vf_flush_wa)
> -		cs = gen8_emit_pipe_control(cs, 0, 0);
> +	if (vf_flush_wa || rt_flush_wa) {
> +		u32 pc_flags = rt_flush_wa ? PIPE_CONTROL_FLUSH_ENABLE : 0;
> +		cs = gen8_emit_pipe_control(cs, pc_flags, 0);

Maybe we could save a conditional by renaming vf_flush_wa to flush_wa, 
enabling it in both relevant cases, and adding u32 flush_wa_flags at the 
top level, defaulting to zero and setting it to 
PIPE_CONTROL_FLUSH_ENABLE. Could be worth trying but can be a followup 
as well since gains would be marginal.

> +	}
>   
>   	if (dc_flush_wa)
>   		cs = gen8_emit_pipe_control(cs, PIPE_CONTROL_DC_FLUSH_ENABLE,
> 

Otherwise looks fine - just needs a commit msg and I'm wondering if the 
official Wa designation exists.

Regards,

Tvrtko