Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap
Quoting Tvrtko Ursulin (2018-02-08 14:31:05) > > On 08/02/2018 14:06, Ville Syrjälä wrote: > > On Thu, Feb 08, 2018 at 08:18:42AM +, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin > >> > >> We can implement limited RC6 counter wrap-around protection under the > >> assumption that clients will be reading this value more frequently than > >> the wrap period on a given platform. > >> > >> With the typical wrap-around period being ~90 minutes, even with the > >> exception of Baytrail which wraps every 13 seconds, this sounds like a > >> reasonable assumption. > >> > >> Implementation works by storing a 64-bit software copy of a hardware RC6 > >> counter, along with the previous HW counter snapshot. This enables it to > >> detect wrap is polled frequently enough and keep the software copy > >> monotonically incrementing. > >> > >> v2: > >> * Missed GEN6_GT_GFX_RC6_LOCKED when considering slot sizing and > >> indexing. > >> * Fixed off-by-one in wrap-around handling. (Chris Wilson) > >> > >> v3: > >> * Simplify index checking by using unsigned int. (Chris Wilson) > >> * Expand the comment to explain why indexing works. > >> > >> Signed-off-by: Tvrtko Ursulin > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94852 > >> Cc: Chris Wilson > >> Reviewed-by: Chris Wilson > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ > >> drivers/gpu/drm/i915/intel_pm.c | 62 > >> +++-- > >> 2 files changed, 55 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> b/drivers/gpu/drm/i915/i915_drv.h > >> index ad1fc845cd1b..28a2671a26c7 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -946,6 +946,8 @@ struct intel_rps { > >> > >> struct intel_rc6 { > >> bool enabled; > >> +u64 prev_hw_residency[4]; > >> +u64 cur_residency[4]; > >> }; > >> > >> struct intel_llc_pstate { > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c > >> b/drivers/gpu/drm/i915/intel_pm.c > >> index 41f26ab46501..063c885175e7 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -9417,15 +9417,16 @@ static u64 vlv_residency_raw(struct > >> drm_i915_private *dev_priv, > >> const i915_reg_t reg) > >> { > >> u32 lower, upper, tmp; > >> -unsigned long flags; > >> int loop = 2; > >> > >> -/* The register accessed do not need forcewake. We borrow > >> +/* > >> + * The register accessed do not need forcewake. We borrow > >> * uncore lock to prevent concurrent access to range reg. > >> */ > >> -spin_lock_irqsave(&dev_priv->uncore.lock, flags); > >> +lockdep_assert_held(&dev_priv->uncore.lock); > >> > >> -/* vlv and chv residency counters are 40 bits in width. > >> +/* > >> + * vlv and chv residency counters are 40 bits in width. > >> * With a control bit, we can choose between upper or lower > >> * 32bit window into this counter. > >> * > >> @@ -9449,29 +9450,49 @@ static u64 vlv_residency_raw(struct > >> drm_i915_private *dev_priv, > >> upper = I915_READ_FW(reg); > >> } while (upper != tmp && --loop); > >> > >> -/* Everywhere else we always use VLV_COUNTER_CONTROL with the > >> +/* > >> + * Everywhere else we always use VLV_COUNTER_CONTROL with the > >> * VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set > >> * now. > >> */ > >> > >> -spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > >> - > >> return lower | (u64)upper << 8; > >> } > >> > >> u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, > >> const i915_reg_t reg) > >> { > >> -u64 time_hw; > >> +u64 time_hw, prev_hw, overflow_hw; > >> +unsigned int fw_domains; > >> +unsigned long flags; > >> +unsigned int i; > >> u32 mul, div; > >> > >> if (!HAS_RC6(dev_priv)) > >> return 0; > >> > >> +/* > >> + * Store previous hw counter values for counter wrap-around handling. > >> + * > >> + * There are only four interesting registers and they live next to > >> each > >> + * other so we can use the relative address, compared to the smallest > >> + * one as the index into driver storage. > >> + */ > >> +i = (i915_mmio_reg_offset(reg) - > >> + i915_mmio_reg_offset(GEN6_GT_GFX_RC6_LOCKED)) / sizeof(u32); > >> +if (WARN_ON_ONCE(i >= ARRAY_SIZE(dev_priv->gt_pm.rc6.cur_residency))) > >> +return 0; > >> + > >> +fw_domains = intel_uncore_forcewake_for_reg(dev_priv, reg, > >> FW_REG_READ); > >> + > >> +spin_lock_irqsave(&dev_priv->uncore.lock, flags); > >> +intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > >> + > >> /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ > >> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(d
Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap
On 08/02/2018 14:06, Ville Syrjälä wrote: On Thu, Feb 08, 2018 at 08:18:42AM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin We can implement limited RC6 counter wrap-around protection under the assumption that clients will be reading this value more frequently than the wrap period on a given platform. With the typical wrap-around period being ~90 minutes, even with the exception of Baytrail which wraps every 13 seconds, this sounds like a reasonable assumption. Implementation works by storing a 64-bit software copy of a hardware RC6 counter, along with the previous HW counter snapshot. This enables it to detect wrap is polled frequently enough and keep the software copy monotonically incrementing. v2: * Missed GEN6_GT_GFX_RC6_LOCKED when considering slot sizing and indexing. * Fixed off-by-one in wrap-around handling. (Chris Wilson) v3: * Simplify index checking by using unsigned int. (Chris Wilson) * Expand the comment to explain why indexing works. Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94852 Cc: Chris Wilson Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 62 +++-- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ad1fc845cd1b..28a2671a26c7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -946,6 +946,8 @@ struct intel_rps { struct intel_rc6 { bool enabled; + u64 prev_hw_residency[4]; + u64 cur_residency[4]; }; struct intel_llc_pstate { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 41f26ab46501..063c885175e7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -9417,15 +9417,16 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv, const i915_reg_t reg) { u32 lower, upper, tmp; - unsigned long flags; int loop = 2; - /* The register accessed do not need forcewake. We borrow + /* +* The register accessed do not need forcewake. We borrow * uncore lock to prevent concurrent access to range reg. */ - spin_lock_irqsave(&dev_priv->uncore.lock, flags); + lockdep_assert_held(&dev_priv->uncore.lock); - /* vlv and chv residency counters are 40 bits in width. + /* +* vlv and chv residency counters are 40 bits in width. * With a control bit, we can choose between upper or lower * 32bit window into this counter. * @@ -9449,29 +9450,49 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv, upper = I915_READ_FW(reg); } while (upper != tmp && --loop); - /* Everywhere else we always use VLV_COUNTER_CONTROL with the + /* +* Everywhere else we always use VLV_COUNTER_CONTROL with the * VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set * now. */ - spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); - return lower | (u64)upper << 8; } u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, const i915_reg_t reg) { - u64 time_hw; + u64 time_hw, prev_hw, overflow_hw; + unsigned int fw_domains; + unsigned long flags; + unsigned int i; u32 mul, div; if (!HAS_RC6(dev_priv)) return 0; + /* +* Store previous hw counter values for counter wrap-around handling. +* +* There are only four interesting registers and they live next to each +* other so we can use the relative address, compared to the smallest +* one as the index into driver storage. +*/ + i = (i915_mmio_reg_offset(reg) - +i915_mmio_reg_offset(GEN6_GT_GFX_RC6_LOCKED)) / sizeof(u32); + if (WARN_ON_ONCE(i >= ARRAY_SIZE(dev_priv->gt_pm.rc6.cur_residency))) + return 0; + + fw_domains = intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ); + + spin_lock_irqsave(&dev_priv->uncore.lock, flags); + intel_uncore_forcewake_get__locked(dev_priv, fw_domains); + /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { mul = 100; div = dev_priv->czclk_freq; + overflow_hw = BIT_ULL(40); time_hw = vlv_residency_raw(dev_priv, reg); } else { /* 833.33ns units on Gen9LP, 1.28us elsewhere. */ @@ -9483,9 +9504,32 @@ u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, div = 1; } - time_hw = I915_READ(reg); + overflow_hw = BIT_ULL(32); + time_hw = I915_READ_FW(reg);
Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap
On Thu, Feb 08, 2018 at 08:18:42AM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > We can implement limited RC6 counter wrap-around protection under the > assumption that clients will be reading this value more frequently than > the wrap period on a given platform. > > With the typical wrap-around period being ~90 minutes, even with the > exception of Baytrail which wraps every 13 seconds, this sounds like a > reasonable assumption. > > Implementation works by storing a 64-bit software copy of a hardware RC6 > counter, along with the previous HW counter snapshot. This enables it to > detect wrap is polled frequently enough and keep the software copy > monotonically incrementing. > > v2: > * Missed GEN6_GT_GFX_RC6_LOCKED when considering slot sizing and >indexing. > * Fixed off-by-one in wrap-around handling. (Chris Wilson) > > v3: > * Simplify index checking by using unsigned int. (Chris Wilson) > * Expand the comment to explain why indexing works. > > Signed-off-by: Tvrtko Ursulin > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94852 > Cc: Chris Wilson > Reviewed-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 62 > +++-- > 2 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ad1fc845cd1b..28a2671a26c7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -946,6 +946,8 @@ struct intel_rps { > > struct intel_rc6 { > bool enabled; > + u64 prev_hw_residency[4]; > + u64 cur_residency[4]; > }; > > struct intel_llc_pstate { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 41f26ab46501..063c885175e7 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -9417,15 +9417,16 @@ static u64 vlv_residency_raw(struct drm_i915_private > *dev_priv, >const i915_reg_t reg) > { > u32 lower, upper, tmp; > - unsigned long flags; > int loop = 2; > > - /* The register accessed do not need forcewake. We borrow > + /* > + * The register accessed do not need forcewake. We borrow >* uncore lock to prevent concurrent access to range reg. >*/ > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > + lockdep_assert_held(&dev_priv->uncore.lock); > > - /* vlv and chv residency counters are 40 bits in width. > + /* > + * vlv and chv residency counters are 40 bits in width. >* With a control bit, we can choose between upper or lower >* 32bit window into this counter. >* > @@ -9449,29 +9450,49 @@ static u64 vlv_residency_raw(struct drm_i915_private > *dev_priv, > upper = I915_READ_FW(reg); > } while (upper != tmp && --loop); > > - /* Everywhere else we always use VLV_COUNTER_CONTROL with the > + /* > + * Everywhere else we always use VLV_COUNTER_CONTROL with the >* VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set >* now. >*/ > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > - > return lower | (u64)upper << 8; > } > > u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, > const i915_reg_t reg) > { > - u64 time_hw; > + u64 time_hw, prev_hw, overflow_hw; > + unsigned int fw_domains; > + unsigned long flags; > + unsigned int i; > u32 mul, div; > > if (!HAS_RC6(dev_priv)) > return 0; > > + /* > + * Store previous hw counter values for counter wrap-around handling. > + * > + * There are only four interesting registers and they live next to each > + * other so we can use the relative address, compared to the smallest > + * one as the index into driver storage. > + */ > + i = (i915_mmio_reg_offset(reg) - > + i915_mmio_reg_offset(GEN6_GT_GFX_RC6_LOCKED)) / sizeof(u32); > + if (WARN_ON_ONCE(i >= ARRAY_SIZE(dev_priv->gt_pm.rc6.cur_residency))) > + return 0; > + > + fw_domains = intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ); > + > + spin_lock_irqsave(&dev_priv->uncore.lock, flags); > + intel_uncore_forcewake_get__locked(dev_priv, fw_domains); > + > /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > mul = 100; > div = dev_priv->czclk_freq; > + overflow_hw = BIT_ULL(40); > time_hw = vlv_residency_raw(dev_priv, reg); > } else { > /* 833.33ns units on Gen9LP, 1.28us elsewhere. */ > @@ -9483,9 +9504,32 @@ u64 intel_rc6_residency_ns(struct drm_i915_private > *dev_priv, > div = 1; > } > > - time_hw = I915
[Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap
From: Tvrtko Ursulin We can implement limited RC6 counter wrap-around protection under the assumption that clients will be reading this value more frequently than the wrap period on a given platform. With the typical wrap-around period being ~90 minutes, even with the exception of Baytrail which wraps every 13 seconds, this sounds like a reasonable assumption. Implementation works by storing a 64-bit software copy of a hardware RC6 counter, along with the previous HW counter snapshot. This enables it to detect wrap is polled frequently enough and keep the software copy monotonically incrementing. v2: * Missed GEN6_GT_GFX_RC6_LOCKED when considering slot sizing and indexing. * Fixed off-by-one in wrap-around handling. (Chris Wilson) v3: * Simplify index checking by using unsigned int. (Chris Wilson) * Expand the comment to explain why indexing works. Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94852 Cc: Chris Wilson Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 62 +++-- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ad1fc845cd1b..28a2671a26c7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -946,6 +946,8 @@ struct intel_rps { struct intel_rc6 { bool enabled; + u64 prev_hw_residency[4]; + u64 cur_residency[4]; }; struct intel_llc_pstate { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 41f26ab46501..063c885175e7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -9417,15 +9417,16 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv, const i915_reg_t reg) { u32 lower, upper, tmp; - unsigned long flags; int loop = 2; - /* The register accessed do not need forcewake. We borrow + /* +* The register accessed do not need forcewake. We borrow * uncore lock to prevent concurrent access to range reg. */ - spin_lock_irqsave(&dev_priv->uncore.lock, flags); + lockdep_assert_held(&dev_priv->uncore.lock); - /* vlv and chv residency counters are 40 bits in width. + /* +* vlv and chv residency counters are 40 bits in width. * With a control bit, we can choose between upper or lower * 32bit window into this counter. * @@ -9449,29 +9450,49 @@ static u64 vlv_residency_raw(struct drm_i915_private *dev_priv, upper = I915_READ_FW(reg); } while (upper != tmp && --loop); - /* Everywhere else we always use VLV_COUNTER_CONTROL with the + /* +* Everywhere else we always use VLV_COUNTER_CONTROL with the * VLV_COUNT_RANGE_HIGH bit set - so it is safe to leave it set * now. */ - spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); - return lower | (u64)upper << 8; } u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, const i915_reg_t reg) { - u64 time_hw; + u64 time_hw, prev_hw, overflow_hw; + unsigned int fw_domains; + unsigned long flags; + unsigned int i; u32 mul, div; if (!HAS_RC6(dev_priv)) return 0; + /* +* Store previous hw counter values for counter wrap-around handling. +* +* There are only four interesting registers and they live next to each +* other so we can use the relative address, compared to the smallest +* one as the index into driver storage. +*/ + i = (i915_mmio_reg_offset(reg) - +i915_mmio_reg_offset(GEN6_GT_GFX_RC6_LOCKED)) / sizeof(u32); + if (WARN_ON_ONCE(i >= ARRAY_SIZE(dev_priv->gt_pm.rc6.cur_residency))) + return 0; + + fw_domains = intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ); + + spin_lock_irqsave(&dev_priv->uncore.lock, flags); + intel_uncore_forcewake_get__locked(dev_priv, fw_domains); + /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { mul = 100; div = dev_priv->czclk_freq; + overflow_hw = BIT_ULL(40); time_hw = vlv_residency_raw(dev_priv, reg); } else { /* 833.33ns units on Gen9LP, 1.28us elsewhere. */ @@ -9483,9 +9504,32 @@ u64 intel_rc6_residency_ns(struct drm_i915_private *dev_priv, div = 1; } - time_hw = I915_READ(reg); + overflow_hw = BIT_ULL(32); + time_hw = I915_READ_FW(reg); } + /* +* Counter wrap handling. +* +* But relying on a sufficien