Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap

[Chris Wilson]

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(_priv->uncore.lock, flags);
> >> +lockdep_assert_held(_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(_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(_priv->uncore.lock, flags);
> >> +intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
> >> +
> >>  /* On VLV and CHV, residency time is 

Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap

[Tvrtko Ursulin]

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(_priv->uncore.lock, flags);
+   lockdep_assert_held(_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(_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(_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);

+   

Re: [Intel-gfx] [PATCH v3] drm/i915: Handle RC6 counter wrap

[Ville Syrjälä]

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(_priv->uncore.lock, flags);
> + lockdep_assert_held(_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(_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(_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 
>