[PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Ben Widawsky]

Apparently these ECOCHK bits changed on HSW and the behavior is not what
we want. I've not been able to find VLV definition specifically so I'll
assume it's the same as IVB.

(Only compile tested)

Reported-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4cbae7b..01cf805 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -322,11 +322,11 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
 		I915_WRITE(GAM_ECOCHK, ecochk | ECOCHK_SNB_BIT |
 				       ECOCHK_PPGTT_CACHE64B);
 		I915_WRITE(GFX_MODE, _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
-	} else if (INTEL_INFO(dev)->gen >= 7) {
+	} else if (INTEL_INFO(dev)->gen >= 7 && !IS_HASWELL(dev)) {
 		I915_WRITE(GAM_ECOCHK, ECOCHK_PPGTT_CACHE64B);
-		/* GFX_MODE is per-ring on gen7+ */
 	}
 
+	/* GFX_MODE is per-ring on gen7+ */
 	for_each_ring(ring, dev_priv, i) {
 		if (INTEL_INFO(dev)->gen >= 7)
 			I915_WRITE(RING_MODE_GEN7(ring),
-- 
1.8.2

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Kenneth Graunke]

On 04/03/2013 11:06 AM, Ben Widawsky wrote:
> Apparently these ECOCHK bits changed on HSW and the behavior is not what
> we want. I've not been able to find VLV definition specifically so I'll
> assume it's the same as IVB.
>
> (Only compile tested)
>
> Reported-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

The behavior isn't particularly bad, but the PTEs already take care of 
this for us, so it doesn't do anything.  That said, having random 
override bits set for no purpose is bad...we should just let the PTEs do 
their job.

Reviewed-and-tested-by: Kenneth Graunke <kenneth@whitecape.org>

Thanks Ben!

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Daniel Vetter]

On Wed, Apr 3, 2013 at 9:17 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> On 04/03/2013 11:06 AM, Ben Widawsky wrote:
>>
>> Apparently these ECOCHK bits changed on HSW and the behavior is not what
>> we want. I've not been able to find VLV definition specifically so I'll
>> assume it's the same as IVB.
>>
>> (Only compile tested)
>>
>> Reported-by: Kenneth Graunke <kenneth@whitecape.org>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
>
> The behavior isn't particularly bad, but the PTEs already take care of this
> for us, so it doesn't do anything.  That said, having random override bits
> set for no purpose is bad...we should just let the PTEs do their job.
>
> Reviewed-and-tested-by: Kenneth Graunke <kenneth@whitecape.org>

This bit is used to set the cachability of the ppgtt PTEs themselves,
_not_ the data pointed at by the ptes. We very much want that, it's
after all the only reason we have enabled aliasing ppgtt support on
snb/ivb. Iirc the speedup was around 10-20% on some benchmarks,
disabling this bit here complety killed these improvements.

Also, if you disable llc caching, the cpu pte writes aren't coherent
any more with what the gt reads, so I'm pretty sure that this will
blow up somewhere on one of our more nasty igt tests.

So I've checked hsw bspec and the problem is that hw guys again
changed the bits around a bit, and I think on HSW we actually want
(0x8 << 3) instead of what's currently there.

Could also be that on hsw the hw now supports pte cachability control
in the pdes, but at least on snb/ivb that part didn't really work. And
bspec is a bit an unclear mess in that area ... Since we should be ok
with the override I don't think it's worth time to investigate this
more.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Daniel Vetter]

On Wed, Apr 3, 2013 at 9:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> So I've checked hsw bspec and the problem is that hw guys again
> changed the bits around a bit, and I think on HSW we actually want
> (0x8 << 3) instead of what's currently there.

Meh, I've screwed up reading the tables, 0x3 << 3 is what we imo want,
so nothing needs to be changed. Sorry for the confusion.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Ben Widawsky]

On Wed, Apr 03, 2013 at 10:08:26PM +0200, Daniel Vetter wrote:
> On Wed, Apr 3, 2013 at 9:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > So I've checked hsw bspec and the problem is that hw guys again
> > changed the bits around a bit, and I think on HSW we actually want
> > (0x8 << 3) instead of what's currently there.
> 
> Meh, I've screwed up reading the tables, 0x3 << 3 is what we imo want,
> so nothing needs to be changed. Sorry for the confusion.
> -Daniel

I think the existing code is magic that we don't/shouldn't need. But I
also see no reason to change it.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Ben Widawsky]

On Wed, Apr 03, 2013 at 01:41:05PM -0700, Ben Widawsky wrote:
> On Wed, Apr 03, 2013 at 10:08:26PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 3, 2013 at 9:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > So I've checked hsw bspec and the problem is that hw guys again
> > > changed the bits around a bit, and I think on HSW we actually want
> > > (0x8 << 3) instead of what's currently there.
> > 
> > Meh, I've screwed up reading the tables, 0x3 << 3 is what we imo want,
> > so nothing needs to be changed. Sorry for the confusion.
> > -Daniel
> 
> I think the existing code is magic that we don't/shouldn't need. But I
> also see no reason to change it.

Digging a bit more, it seems we want this for GEN6 (setting 3<<3), but
not GEN7. I'm done digging now.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Don't override PPGTT cacheability on HSW

[Ville Syrjälä]

On Wed, Apr 03, 2013 at 10:08:26PM +0200, Daniel Vetter wrote:
> On Wed, Apr 3, 2013 at 9:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > So I've checked hsw bspec and the problem is that hw guys again
> > changed the bits around a bit, and I think on HSW we actually want
> > (0x8 << 3) instead of what's currently there.
> 
> Meh, I've screwed up reading the tables, 0x3 << 3 is what we imo want,
> so nothing needs to be changed. Sorry for the confusion.

Shouldn't it be (1<<3) on IVB (for just LLC w/o GFDT), and (3<<3) on
the rest?

Also what about the GAC_ECO_BITS register? BSpec tells me it exists on
IVB and HSW as well. It also seems to have a bit very similar to
ECOCHK_SNB_BIT but we don't actually set it on SNB.

-- 
Ville Syrjälä
Intel OTC