Re: [PATCH 02/12] drm/i915: implement WaDisableL3CacheAging on VLV

From: Ben Widawsky <ben@bwidawsk.net>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 02/12] drm/i915: implement WaDisableL3CacheAging on VLV
Date: Tue, 2 Oct 2012 16:14:06 -0700	[thread overview]
Message-ID: <20121002161406.339db11d@bwidawsk.net> (raw)
In-Reply-To: <20121002160919.4de9efe3@bwidawsk.net>

On Tue, 2 Oct 2012 16:09:19 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, 3 Oct 2012 01:01:07 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Oct 02, 2012 at 05:43:36PM -0500, Jesse Barnes wrote:
> > > Needs to be set on every context restore as well, so set it as
> > > part of the initial state so we can save/restore it.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > >  drivers/gpu/drm/i915/intel_pm.c |    2 +-
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 7d133a1..58935a3 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3442,6 +3442,7 @@
> > >  
> > >  #define GEN7_L3CNTLREG1				0xB01C
> > >  #define  GEN7_WA_FOR_GEN7_L3_CONTROL
> > > 0x3C4FFF8C +#define  GEN7_L3AGDIS
> > > (1<<19) 
> > >  #define GEN7_L3_CHICKEN_MODE_REGISTER		0xB030
> > >  #define  GEN7_WA_L3_CHICKEN_MODE
> > > 0x20000000 diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c index 82ca172..f7344c9 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3614,7 +3614,7 @@ static void
> > > valleyview_init_clock_gating(struct drm_device *dev)
> > > GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC); 
> > >  	/* WaApplyL3ControlAndL3ChickenMode requires those two on
> > > Ivy Bridge */
> > > -	I915_WRITE(GEN7_L3CNTLREG1, GEN7_WA_FOR_GEN7_L3_CONTROL);
> > > +	I915_WRITE(GEN7_L3CNTLREG1, I915_READ(GEN7_L3CNTLREG1) |
> > > GEN7_L3AGDIS);
> > 
> > tbh I don't like rmw magic register values, since that essentially
> > means that for suspend resume support we need either
> > - the bios restoring these magic values for us
> > - have a separate piece of save/restore code
> > - actually the read isn't required and there's nothing to restore in
> >   additions to the bits we're setting
> > 
> > All of which are a royal pain for long-term maintaince, especially
> > since our current suspend/resume code is already a fragile mess.
> > 
> > Can we do better and just write the right settings to this register
> > unconditionally?
> > -Daniel
> 
> In theory that's true, but even the docs change occasionally on what
> values we should set, so I don't think either is great. Also a comment
> on one of the bits seem to suggest we should expect BIOS to be setting
> this for us, however Jesse missed another workaround which I'll point
> out to him now in person.
> 

On looking further it seems the magic bit Jesse is trying to set is
already set, but the original magic number came from Eugeni, and it
strays from the register default quite a bit.

> > 
> > >  	I915_WRITE(GEN7_L3_CHICKEN_MODE_REGISTER,
> > > GEN7_WA_L3_CHICKEN_MODE); 
> > >  	/* This is required by WaCatErrorRejectionIssue */
> > > -- 
> > > 1.7.9.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> 
> 

-- 
Ben Widawsky, Intel Open Source Technology Center