diff mbox

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

Message ID 1491506163-14587-22-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi April 6, 2017, 7:15 p.m. UTC 
From: Ben Widawsky <benjamin.widawsky@intel.com>

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin June 2, 2017, 10:01 a.m. UTC | #1 
On 06/04/2017 20:15, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>

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

> 
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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
Rodrigo Vivi June 5, 2017, 5:17 p.m. UTC | #2 
On Fri, 2017-06-02 at 11:01 +0100, Tvrtko Ursulin wrote:
> On 06/04/2017 20:15, Rodrigo Vivi wrote:

> > From: Ben Widawsky <benjamin.widawsky@intel.com>

> 

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



> > 

> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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?


Wa:#1034
WaPCFlushBeforeRTCacheFlush

But Oh! It is only for A0. So just ignore this patch for now...
I will re-test without this patch and probably skip it.

> 

> > +		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
diff mbox

Patch

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."
+		 */
+		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);
+	}
 
 	if (dc_flush_wa)
 		cs = gen8_emit_pipe_control(cs, PIPE_CONTROL_DC_FLUSH_ENABLE,