| Message ID | 1365012390-6465-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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!
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
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
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.
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.
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.
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),
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(-)