| Message ID | 1349217826-2538-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > 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
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. > > > 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 >
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 > > > > >
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); I915_WRITE(GEN7_L3_CHICKEN_MODE_REGISTER, GEN7_WA_L3_CHICKEN_MODE); /* This is required by WaCatErrorRejectionIssue */
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(-)