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 +0000, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> >>
> >> 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 <tvrtko.ursu...@intel.com>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94852
> >> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> >> Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> >> ---
> >>   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 = 1000000;
> >>              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 sufficient frequency of queries otherwise counters
> >> +     * can still wrap.
> >> +     */
> >> +    prev_hw = dev_priv->gt_pm.rc6.prev_hw_residency[i];
> >> +    dev_priv->gt_pm.rc6.prev_hw_residency[i] = time_hw;
> >> +
> >> +    /* RC6 delta from last sample. */
> >> +    if (time_hw >= prev_hw)
> >> +            time_hw -= prev_hw;
> >> +    else
> >> +            time_hw += overflow_hw - prev_hw;
> >> +
> >> +    /* Add delta to RC6 extended raw driver copy. */
> >> +    time_hw += dev_priv->gt_pm.rc6.cur_residency[i];
> >> +    dev_priv->gt_pm.rc6.cur_residency[i] = time_hw;
> >> +
> >> +    intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
> >> +    spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> >> +
> >>      return DIV_ROUND_UP_ULL(time_hw * mul, div);
> > 
> > What happens when time_hw*mul+div-1 overflows?
> 
> It overflows. :I
> 
> Apart on Valleyview multipler is 14 bits max, so we still get 64 - 14 - 
> 1 = 49 bits of software stored RC6 before it happens. Given that the 
> info if 32-bits is 50 minutes or so, 49 bits sounds good enough.
> 
> On Valleyview multiplier is 20 bits so 64 - 20 - 1 = 43 bits before 
> overflow. A bit less than above, and I am not sure what the 40-bit hw 
> counter overflow period is there to start with? Is 8 times that enough? 
> Or not?

Off the top of my head, 8 hours. Definitely within a timescale that
someone could be monitoring.
 
> Suggestions?

We can extend to uint128_t quite easily on 64b platforms (hmm, but how
much is allowed for freestanding?), otherwise time to whip up a 128b
math library ;)
https://cgit.freedesktop.org/cairo/tree/src/cairo-wideint.c

u64_mul_u64_u32_div_u32_roundup ?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to