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

[Vivi, Rodrigo]

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

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