Re: [Intel-gfx] [PATCH] drm/i915/cnl: Get RC6 working.
On Tue, Oct 24, 2017 at 12:50:13PM +, David Weinehall wrote: > On Mon, Oct 23, 2017 at 03:46:12PM -0700, Rodrigo Vivi wrote: > > On CNL, individual wake rate limit was added to each engine. > > > > GT can only go to RC6 if both Render and Media engines are > > individually qualified. So we need to set their individual > > wake rate limit. > > > > +-+---+--+--+ > > | |GT RC6 | Render C6 | Media C6 | > > +-+---+--+--+ > > | Wake rate limit | 0xA09C[31:16] | 0xA09C[15:0] | 0xA0A0[15:0] | > > +-+---+--+--+ > > > > v2: - Tune Render and Media wake rate values according to some extra > > info I got from HW engineers. Value can be tuned, but for now > > these are the recommended values. > > - Fix typos pointed by James. > > > > Cc: Nathan Ciobanu > > Cc: Wayne Boyer > > Cc: Joe Konno > > Cc: David Weinehall > > Signed-off-by: Rodrigo Vivi > > Reviewed-by: James Ausmus > > I've verified that RC6 works with your patch applied. > Minor comments below, but nothing major. Great work! > > > Reviewed-by: David Weinehall thanks. merged to dinq. > > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 15 +++ > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 68a58cce6ab1..f138eae82bf0 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7905,6 +7905,7 @@ enum { > > #define GEN6_RC1_WAKE_RATE_LIMIT _MMIO(0xA098) > > #define GEN6_RC6_WAKE_RATE_LIMIT _MMIO(0xA09C) > > #define GEN6_RC6pp_WAKE_RATE_LIMIT _MMIO(0xA0A0) > > +#define GEN10_MEDIA_WAKE_RATE_LIMIT_MMIO(0xA0A0) > > #define GEN6_RC_EVALUATION_INTERVAL_MMIO(0xA0A8) > > #define GEN6_RC_IDLE_HYSTERSIS _MMIO(0xA0AC) > > #define GEN6_RC_SLEEP _MMIO(0xA0B0) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 5fdae39b1969..742d5455b201 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -6605,12 +6605,19 @@ static void gen9_enable_rc6(struct drm_i915_private > > *dev_priv) > > I915_WRITE(GEN6_RC_CONTROL, 0); > > > > /* 2b: Program RC6 thresholds.*/ > > - > > - /* WaRsDoubleRc6WrlWithCoarsePowerGating: Doubling WRL only when CPG is > > enabled */ > > - if (IS_SKYLAKE(dev_priv)) > > + if (INTEL_GEN(dev_priv) >= 10) { > > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16 | 85); > > + I915_WRITE(GEN10_MEDIA_WAKE_RATE_LIMIT, 150); > > + } else if (IS_SKYLAKE(dev_priv)) { > > How about: > > } else if (NEEDS_WaRsDisableCoarsePowerGating(dev_priv)) { I believe if IS_SKYLAKE(dev_priv) && *!* NEEDS_WaRsDisableCoarsePowerGating since by name it seems WaRsDoubleRc6WrlWithCoarsePowerGating is only needed with coarsepowergating on and the other one is disable course power gating. right? > > I realise that this isn't code you're introducing, but fixing it at the > same time might make sense. We have a few other cases elsewhere with > where we apply Coarse PG even though (at least according to that > WA-test) we only need it on some Skylakes. Since this would change the behaviour of the wa on few SKL skus I believe it deserves a different patch. Also on that patch we would need to check if we need to extend this Wa to other gen9 platforms. I'd say we probably need this on kbl and cfl :/ Thanks, Rodrigo. > > > + /* > > +* WaRsDoubleRc6WrlWithCoarsePowerGating:skl Doubling WRL only > > +* when CPG is enabled > > +*/ > > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 108 << 16); > > - else > > + } else { > > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16); > > + } > > + > > I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > > I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > > for_each_engine(engine, dev_priv, id) > > -- > > 2.13.5 > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/cnl: Get RC6 working.
On Mon, Oct 23, 2017 at 03:46:12PM -0700, Rodrigo Vivi wrote: > On CNL, individual wake rate limit was added to each engine. > > GT can only go to RC6 if both Render and Media engines are > individually qualified. So we need to set their individual > wake rate limit. > > +-+---+--+--+ > | |GT RC6 | Render C6 | Media C6 | > +-+---+--+--+ > | Wake rate limit | 0xA09C[31:16] | 0xA09C[15:0] | 0xA0A0[15:0] | > +-+---+--+--+ > > v2: - Tune Render and Media wake rate values according to some extra > info I got from HW engineers. Value can be tuned, but for now > these are the recommended values. > - Fix typos pointed by James. > > Cc: Nathan Ciobanu > Cc: Wayne Boyer > Cc: Joe Konno > Cc: David Weinehall > Signed-off-by: Rodrigo Vivi > Reviewed-by: James Ausmus I've verified that RC6 works with your patch applied. Minor comments below, but nothing major. Great work! Reviewed-by: David Weinehall > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 15 +++ > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 68a58cce6ab1..f138eae82bf0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7905,6 +7905,7 @@ enum { > #define GEN6_RC1_WAKE_RATE_LIMIT _MMIO(0xA098) > #define GEN6_RC6_WAKE_RATE_LIMIT _MMIO(0xA09C) > #define GEN6_RC6pp_WAKE_RATE_LIMIT _MMIO(0xA0A0) > +#define GEN10_MEDIA_WAKE_RATE_LIMIT _MMIO(0xA0A0) > #define GEN6_RC_EVALUATION_INTERVAL _MMIO(0xA0A8) > #define GEN6_RC_IDLE_HYSTERSIS _MMIO(0xA0AC) > #define GEN6_RC_SLEEP_MMIO(0xA0B0) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 5fdae39b1969..742d5455b201 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -6605,12 +6605,19 @@ static void gen9_enable_rc6(struct drm_i915_private > *dev_priv) > I915_WRITE(GEN6_RC_CONTROL, 0); > > /* 2b: Program RC6 thresholds.*/ > - > - /* WaRsDoubleRc6WrlWithCoarsePowerGating: Doubling WRL only when CPG is > enabled */ > - if (IS_SKYLAKE(dev_priv)) > + if (INTEL_GEN(dev_priv) >= 10) { > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16 | 85); > + I915_WRITE(GEN10_MEDIA_WAKE_RATE_LIMIT, 150); > + } else if (IS_SKYLAKE(dev_priv)) { How about: } else if (NEEDS_WaRsDisableCoarsePowerGating(dev_priv)) { I realise that this isn't code you're introducing, but fixing it at the same time might make sense. We have a few other cases elsewhere with where we apply Coarse PG even though (at least according to that WA-test) we only need it on some Skylakes. > + /* > + * WaRsDoubleRc6WrlWithCoarsePowerGating:skl Doubling WRL only > + * when CPG is enabled > + */ > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 108 << 16); > - else > + } else { > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16); > + } > + > I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > for_each_engine(engine, dev_priv, id) > -- > 2.13.5 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/cnl: Get RC6 working.
On Mon, Oct 23, 2017 at 02:20:01PM -0700, Rodrigo Vivi wrote: > On CNL, individual ware rate limit was added to each engine. s/ware/wake/ > > GT can only go to RC6 if both Render and Media engines are > idividually qualified. So we need to set their individual s/idividually/individually/ > wake rate limit. > > +-+---+--+--+ > | |GT RC6 | Render C6 | Media C6 | > +-+---+--+--+ > | Wake rate limit | 0xA09C[31:16] | 0xA09C[15:0] | 0xA0A0[15:0] | > +-+---+--+--+ > > Cc: Nathan Ciobanu > Cc: Wayne Boyer > Cc: Joe Konno > Cc: David Weinehall > Signed-off-by: Rodrigo Vivi Matches my read of the spec. With the commit message nitpicking fixed :) Reviewed-by: James Ausmus > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 15 +++ > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 68a58cce6ab1..f138eae82bf0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7905,6 +7905,7 @@ enum { > #define GEN6_RC1_WAKE_RATE_LIMIT _MMIO(0xA098) > #define GEN6_RC6_WAKE_RATE_LIMIT _MMIO(0xA09C) > #define GEN6_RC6pp_WAKE_RATE_LIMIT _MMIO(0xA0A0) > +#define GEN10_MEDIA_WAKE_RATE_LIMIT _MMIO(0xA0A0) > #define GEN6_RC_EVALUATION_INTERVAL _MMIO(0xA0A8) > #define GEN6_RC_IDLE_HYSTERSIS _MMIO(0xA0AC) > #define GEN6_RC_SLEEP_MMIO(0xA0B0) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 5fdae39b1969..b193eda65e81 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -6605,12 +6605,19 @@ static void gen9_enable_rc6(struct drm_i915_private > *dev_priv) > I915_WRITE(GEN6_RC_CONTROL, 0); > > /* 2b: Program RC6 thresholds.*/ > - > - /* WaRsDoubleRc6WrlWithCoarsePowerGating: Doubling WRL only when CPG is > enabled */ > - if (IS_SKYLAKE(dev_priv)) > + if (INTEL_GEN(dev_priv) >= 10) { > + I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16 | 54); > + I915_WRITE(GEN10_MEDIA_WAKE_RATE_LIMIT, 54); > + } else if (IS_SKYLAKE(dev_priv)) { > + /* > + * WaRsDoubleRc6WrlWithCoarsePowerGating:skl Doubling WRL only > + * when CPG is enabled > + */ > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 108 << 16); > - else > + } else { > I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16); > + } > + > I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */ > I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */ > for_each_engine(engine, dev_priv, id) > -- > 2.13.5 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx