[PATCH] drm/i915: Limit VF cache invalidate workaround usage to gen9

[PATCH] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Ben Widawsky]

It is unclear if this is even required on BXT.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 519cea32..0d35e94 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
 		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
 		flags |= PIPE_CONTROL_QW_WRITE;
 		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
-	}
 
-	/*
-	 * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe
-	 * control.
-	 */
-	vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 &&
-		      flags & PIPE_CONTROL_VF_CACHE_INVALIDATE;
+		/*
+		 * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL
+		 * pipe control.
+		 */
+		if (IS_GEN9(ring->dev))
+			vf_flush_wa = true;
+	}
 
 	ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
 	if (ret)
-- 
2.6.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Ben Widawsky]

It is unclear if this is even required on BXT.

v2: Make sure to set the default value to false. Uncertain how my compiler
doesn't complain with v1.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 519cea32..af1e001 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct intel_engine_cs *ring = ringbuf->ring;
 	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
-	bool vf_flush_wa;
+	bool vf_flush_wa = false;
 	u32 flags = 0;
 	int ret;
 
@@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
 		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
 		flags |= PIPE_CONTROL_QW_WRITE;
 		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
-	}
 
-	/*
-	 * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe
-	 * control.
-	 */
-	vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 &&
-		      flags & PIPE_CONTROL_VF_CACHE_INVALIDATE;
+		/*
+		 * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL
+		 * pipe control.
+		 */
+		if (IS_GEN9(ring->dev))
+			vf_flush_wa = true;
+	}
 
 	ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
 	if (ret)
-- 
2.6.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Imre Deak]

On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
> It is unclear if this is even required on BXT.

I'm not sure either, I only added it on the premise that it was marked
as SKL+ originally in BSpec. The revision log entry in BSpec has this
much to say:
"""
The workaround that requires an empty PIPE_CONTROL before a
PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and
not SKL+. The bug was fixed in CNL with the following HSD:...
"""
Which doesn't make this clear either imo.

> v2: Make sure to set the default value to false. Uncertain how my compiler
> doesn't complain with v1.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 519cea32..af1e001 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>  	struct intel_ringbuffer *ringbuf = request->ringbuf;
>  	struct intel_engine_cs *ring = ringbuf->ring;
>  	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
> -	bool vf_flush_wa;
> +	bool vf_flush_wa = false;
>  	u32 flags = 0;
>  	int ret;
>  
> @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_QW_WRITE;
>  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> -	}
>  
> -	/*
> -	 * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe
> -	 * control.
> -	 */
> -	vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 &&
> -		      flags & PIPE_CONTROL_VF_CACHE_INVALIDATE;
> +		/*
> +		 * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL
> +		 * pipe control.
> +		 */
> +> 		if (IS_GEN9(ring->dev))

Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> +			vf_flush_wa = true;
> +	}
>  
>  	ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
>  	if (ret)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Ben Widawsky]

On Thu, Dec 17, 2015 at 10:49:24PM +0200, Imre Deak wrote:
> On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
> > It is unclear if this is even required on BXT.
> 
> I'm not sure either, I only added it on the premise that it was marked
> as SKL+ originally in BSpec. The revision log entry in BSpec has this
> much to say:
> """
> The workaround that requires an empty PIPE_CONTROL before a
> PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and
> not SKL+. The bug was fixed in CNL with the following HSD:...
> """
> Which doesn't make this clear either imo.
> 
> > v2: Make sure to set the default value to false. Uncertain how my compiler
> > doesn't complain with v1.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 519cea32..af1e001 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> >  	struct intel_ringbuffer *ringbuf = request->ringbuf;
> >  	struct intel_engine_cs *ring = ringbuf->ring;
> >  	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
> > -	bool vf_flush_wa;
> > +	bool vf_flush_wa = false;
> >  	u32 flags = 0;
> >  	int ret;
> >  
> > @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> >  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
> >  		flags |= PIPE_CONTROL_QW_WRITE;
> >  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> > -	}
> >  
> > -	/*
> > -	 * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe
> > -	 * control.
> > -	 */
> > -	vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 &&
> > -		      flags & PIPE_CONTROL_VF_CACHE_INVALIDATE;
> > +		/*
> > +		 * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL
> > +		 * pipe control.
> > +		 */
> > +> 		if (IS_GEN9(ring->dev))
> 
> Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 

Sounds good to me. Does someone mind fixing this up when they push? This patch
is a pre-requisite to another internal patch I have at the moment.

> > +			vf_flush_wa = true;
> > +	}
> >  
> >  	ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
> >  	if (ret)

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on ac2305b6c91b9a84cc12566016ece257c3ebcba3 drm-intel-nightly: 2015y-12m-17d-16h-19m-23s UTC integration manifest

Test igt@kms_flip@basic-flip-vs-wf_vblank on snb-x220t dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-wf_vblank on bsw-nuc-2 pass -> dmesg-warn
Test igt@kms_flip@basic-flip-vs-wf_vblank on skl-i7k-2 dmesg-fail -> dmesg-warn
Test igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence on bsw-nuc-2 dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@hang-read-crc-pipe-a on snb-x220t pass -> dmesg-warn
Test igt@kms_flip@basic-flip-vs-dpms on ilk-hp8440p pass -> dmesg-warn
Test igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b on snb-x220t dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@read-crc-pipe-a on snb-x220t pass -> dmesg-warn
Test igt@kms_pipe_crc_basic@read-crc-pipe-c on skl-i7k-2 dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@read-crc-pipe-b on snb-dellxps dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-modeset on bsw-nuc-2 dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-modeset on skl-i5k-2 dmesg-warn -> pass

bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:99   dwarn:1   dfail:0   fail:2   skip:33 
ivb-t430s        total:135  pass:128  dwarn:1   dfail:1   fail:1   skip:4  
skl-i5k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:122  dwarn:1   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_706/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Jani Nikula]

On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
>> +> 		if (IS_GEN9(ring->dev))
>
> Nitpick: INTEL_INFO()->gen == 9 is the preferred way.

Oh? Since when and says who?


BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Imre Deak]

On pe, 2015-12-18 at 11:59 +0200, Jani Nikula wrote:
> On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
> > > +> 		if (IS_GEN9(ring->dev))
> > 
> > Nitpick: INTEL_INFO()->gen == 9 is the preferred way.
> 
> Oh? Since when and says who?

Haven't found  it after some digging, but I do remember someone having
this comment earlier.

My opinion is that we should only use one form, and since INTEL_INFO()-
gen can be used in a more generic manner I would prefer that. Currently
we have it both ways in the code.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Daniel Vetter]

On Fri, Dec 18, 2015 at 12:34:16PM +0200, Imre Deak wrote:
> On pe, 2015-12-18 at 11:59 +0200, Jani Nikula wrote:
> > On Thu, 17 Dec 2015, Imre Deak <imre.deak@intel.com> wrote:
> > > On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
> > > > +> 		if (IS_GEN9(ring->dev))
> > > 
> > > Nitpick: INTEL_INFO()->gen == 9 is the preferred way.
> > 
> > Oh? Since when and says who?
> 
> Haven't found  it after some digging, but I do remember someone having
> this comment earlier.
> 
> My opinion is that we should only use one form, and since INTEL_INFO()-
> gen can be used in a more generic manner I would prefer that. Currently
> we have it both ways in the code.

We use IS_GENx for individual gen checks and INTEL_INFO()->gen when
checking for ranges. At least that's the usage I inferred ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] [v2] drm/i915: Limit VF cache invalidate workaround usage to gen9

[Daniel Vetter]

On Thu, Dec 17, 2015 at 10:49:24PM +0200, Imre Deak wrote:
> On Thu, 2015-12-17 at 09:49 -0800, Ben Widawsky wrote:
> > It is unclear if this is even required on BXT.
> 
> I'm not sure either, I only added it on the premise that it was marked
> as SKL+ originally in BSpec. The revision log entry in BSpec has this
> much to say:
> """
> The workaround that requires an empty PIPE_CONTROL before a
> PIPE_CONTROL with a VF Cache Invalidation Enable is only for SKL and
> not SKL+. The bug was fixed in CNL with the following HSD:...
> """
> Which doesn't make this clear either imo.
> 
> > v2: Make sure to set the default value to false. Uncertain how my compiler
> > doesn't complain with v1.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 519cea32..af1e001 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1695,7 +1695,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> >  	struct intel_ringbuffer *ringbuf = request->ringbuf;
> >  	struct intel_engine_cs *ring = ringbuf->ring;
> >  	u32 scratch_addr = ring->scratch.gtt_offset + 2 * CACHELINE_BYTES;
> > -	bool vf_flush_wa;
> > +	bool vf_flush_wa = false;
> >  	u32 flags = 0;
> >  	int ret;
> >  
> > @@ -1716,14 +1716,14 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
> >  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
> >  		flags |= PIPE_CONTROL_QW_WRITE;
> >  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> > -	}
> >  
> > -	/*
> > -	 * On GEN9+ Before VF_CACHE_INVALIDATE we need to emit a NULL pipe
> > -	 * control.
> > -	 */
> > -	vf_flush_wa = INTEL_INFO(ring->dev)->gen >= 9 &&
> > -		      flags & PIPE_CONTROL_VF_CACHE_INVALIDATE;
> > +		/*
> > +		 * On GEN9: before VF_CACHE_INVALIDATE we need to emit a NULL
> > +		 * pipe control.
> > +		 */
> > +> 		if (IS_GEN9(ring->dev))
> 
> Nitpick: INTEL_INFO()->gen == 9 is the preferred way. Either way:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> 
> > +			vf_flush_wa = true;
> > +	}
> >  
> >  	ret = intel_logical_ring_begin(request, vf_flush_wa ? 12 : 6);
> >  	if (ret)
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx