| Message ID | 1460028253-9861-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 07, 2016 at 12:24:13PM +0100, Chris Wilson wrote: > RC6 residency counters are kept by the hardware in 32bit registers, > which then overflow in a couple of hours. Ideally, we want a continuous > count of the residency since boot and so we must maintain an overflow > counter ourselves. Utilities such as powertop read these registers > through sysfs and the unexpected wrapping produces inconsistent results > and garbage for the user. > > In order to detect the overflow, without intervention, we keep a list of > the interesting registers and read them every few seconds. > > RFC: Every 14 seconds!!! Is it worth imposing that timer on everybody > for the few users of sysfs? Does userspace only truly care about the > relative value and not the absolute value since boot? If so, should we > just limit the timer to when the sysfs file is open? That would need > userspace to keep said file open for as long as it wants accurate > results. At least I don't want to have my VLV wake up every 16 seconds. My idea to handle this was to expose the max counter value in sysfs and let powertop deal with it. Unfortunately when I looked at powertop there didn't seem to a clear way to influence the polling interval from the gfx related code. > > Reported-by: Len Brown <lenb@kernel.org> > Refernces: https://bugs.freedesktop.org/show_bug.cgi?id=94852 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Len Brown <lenb@kernel.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 12 ++++++ > drivers/gpu/drm/i915/i915_sysfs.c | 56 ++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_uncore.c | 80 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 131 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a1f78f275c55..313d21ec39b7 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -659,6 +659,16 @@ struct intel_uncore { > > struct intel_uncore_funcs funcs; > > + /* A few interesting registers track variables over time, > + * such as RC6 residencies, in 32bit registers which overflow within > + * a matter of seconds (~14s). However, we want to present them as a > + * continuous quantity to the user and so we need to keep an overflow > + * counter. > + */ > + struct list_head reg64_emu_list; > + struct timer_list reg64_emu_timer; > + unsigned long reg64_emu_timeout; > + > unsigned fifo_count; > enum forcewake_domains fw_domains; > > @@ -3580,6 +3590,8 @@ __raw_write(64, q) > #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__)) > #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__) > > +u64 i915_read64emu(struct drm_i915_private *dev_priv, i915_reg_t reg); > + > /* "Broadcast RGB" property */ > #define INTEL_BROADCAST_RGB_AUTO 0 > #define INTEL_BROADCAST_RGB_FULL 1 > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index 2d576b7ff299..6073aa4deaba 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -35,13 +35,27 @@ > #define dev_to_drm_minor(d) dev_get_drvdata((d)) > > #ifdef CONFIG_PM > -static u32 calc_residency(struct drm_device *dev, > - i915_reg_t reg) > +static unsigned long calc_overflow_jiffies(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_private *dev_priv = to_i915(dev); > + u32 overflow_ms; > + > + /* How many ticks per millisecond? */ > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > + overflow_ms = ~0u / dev_priv->czclk_freq; Needs to account for the high range bit. > + else if (IS_BROXTON(dev_priv)) > + overflow_ms = ~0u / 1200; /* tick every 833ns */ > + else > + overflow_ms = ~0u / 780; /* tick every 1.28us */ > + > + return msecs_to_jiffies(overflow_ms); > +} > + > +static unsigned long long calc_residency(struct drm_device *dev, i915_reg_t reg) > +{ > + struct drm_i915_private *dev_priv = to_i915(dev); > u64 raw_time; /* 32b value may overflow during fixed point math */ > u64 units = 128ULL, div = 100000ULL; > - u32 ret; > > if (!intel_enable_rc6(dev)) > return 0; > @@ -49,22 +63,22 @@ static u32 calc_residency(struct drm_device *dev, > intel_runtime_pm_get(dev_priv); > > /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ > - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > units = 1; > div = dev_priv->czclk_freq; > > if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH) > units <<= 8; > - } else if (IS_BROXTON(dev)) { > + } else if (IS_BROXTON(dev_priv)) { > units = 1; > div = 1200; /* 833.33ns */ > } > > - raw_time = I915_READ(reg) * units; > - ret = DIV_ROUND_UP_ULL(raw_time, div); > + raw_time = i915_read64emu(dev_priv, reg) * units; > > intel_runtime_pm_put(dev_priv); > - return ret; > + > + return DIV_ROUND_UP_ULL(raw_time, div); > } > > static ssize_t > @@ -78,32 +92,32 @@ static ssize_t > show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf) > { > struct drm_minor *dminor = dev_get_drvdata(kdev); > - u32 rc6_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6); > - return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency); > + return snprintf(buf, PAGE_SIZE, "%llu\n", > + calc_residency(dminor->dev, GEN6_GT_GFX_RC6)); > } > > static ssize_t > show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf) > { > struct drm_minor *dminor = dev_to_drm_minor(kdev); > - u32 rc6p_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6p); > - return snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency); > + return snprintf(buf, PAGE_SIZE, "%llu\n", > + calc_residency(dminor->dev, GEN6_GT_GFX_RC6p)); > } > > static ssize_t > show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf) > { > struct drm_minor *dminor = dev_to_drm_minor(kdev); > - u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp); > - return snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency); > + return snprintf(buf, PAGE_SIZE, "%llu\n", > + calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp)); > } > > static ssize_t > show_media_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf) > { > struct drm_minor *dminor = dev_get_drvdata(kdev); > - u32 rc6_residency = calc_residency(dminor->dev, VLV_GT_MEDIA_RC6); > - return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency); > + return snprintf(buf, PAGE_SIZE, "%llu\n", > + calc_residency(dminor->dev, VLV_GT_MEDIA_RC6)); > } > > static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL); > @@ -593,22 +607,30 @@ static struct bin_attribute error_state_attr = { > > void i915_setup_sysfs(struct drm_device *dev) > { > + struct drm_i915_private *dev_priv = to_i915(dev); > int ret; > > #ifdef CONFIG_PM > if (HAS_RC6(dev)) { > + dev_priv->uncore.reg64_emu_timeout = > + min(dev_priv->uncore.reg64_emu_timeout, > + calc_overflow_jiffies(dev) / 2); > + calc_residency(dev, GEN6_GT_GFX_RC6); > ret = sysfs_merge_group(&dev->primary->kdev->kobj, > &rc6_attr_group); > if (ret) > DRM_ERROR("RC6 residency sysfs setup failed\n"); > } > if (HAS_RC6p(dev)) { > + calc_residency(dev, GEN6_GT_GFX_RC6p); > + calc_residency(dev, GEN6_GT_GFX_RC6pp); > ret = sysfs_merge_group(&dev->primary->kdev->kobj, > &rc6p_attr_group); > if (ret) > DRM_ERROR("RC6p residency sysfs setup failed\n"); > } > if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > + calc_residency(dev, VLV_GT_MEDIA_RC6); > ret = sysfs_merge_group(&dev->primary->kdev->kobj, > &media_rc6_attr_group); > if (ret) > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index fbc1d215ca67..74b3468c3b1a 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1247,6 +1247,70 @@ static void intel_uncore_fw_domains_init(struct drm_device *dev) > WARN_ON(dev_priv->uncore.fw_domains == 0); > } > > +struct reg64emu { > + i915_reg_t reg; > + u64 last; > + struct list_head link; > +}; > + > +static u64 __read64emu(struct drm_i915_private *dev_priv, struct reg64emu *emu) > +{ > + u64 result; > + > + result = I915_READ(emu->reg) | (emu->last & 0xffffffff00000000); > + if (lower_32_bits(result) < lower_32_bits(emu->last)) > + result += 1ull << 32; > + emu->last = result; > + > + return result; > +} > + > +u64 i915_read64emu(struct drm_i915_private *dev_priv, i915_reg_t reg) > +{ > + struct reg64emu *emu; > + > + list_for_each_entry(emu, &dev_priv->uncore.reg64_emu_list, link) > + if (i915_mmio_reg_offset(emu->reg) == i915_mmio_reg_offset(reg)) > + goto read; > + > + emu = kmalloc(sizeof(*emu), GFP_KERNEL); > + if (emu == NULL) > + return I915_READ(reg); > + > + emu->reg = reg; > + emu->last = 0; > + list_add(&emu->link, &dev_priv->uncore.reg64_emu_list); > + > + if (!timer_pending(&dev_priv->uncore.reg64_emu_timer)) > + mod_timer(&dev_priv->uncore.reg64_emu_timer, > + jiffies + dev_priv->uncore.reg64_emu_timeout); > + > +read: > + return __read64emu(dev_priv, emu); > +} > + > +static void intel_uncore_reg64_emu_timer(unsigned long arg) > +{ > + struct drm_i915_private *dev_priv = (struct drm_i915_private *)arg; > + struct reg64emu *emu; > + > + list_for_each_entry(emu, &dev_priv->uncore.reg64_emu_list, link) > + __read64emu(dev_priv, emu); > + > + mod_timer(&dev_priv->uncore.reg64_emu_timer, > + jiffies + dev_priv->uncore.reg64_emu_timeout); > +} > + > +static void intel_uncore_init_regemu64(struct drm_i915_private *dev_priv) > +{ > + dev_priv->uncore.reg64_emu_timeout = MAX_JIFFY_OFFSET; > + > + INIT_LIST_HEAD(&dev_priv->uncore.reg64_emu_list); > + setup_timer(&dev_priv->uncore.reg64_emu_timer, > + intel_uncore_reg64_emu_timer, > + (unsigned long)dev_priv); > +} > + > void intel_uncore_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -1306,16 +1370,32 @@ void intel_uncore_init(struct drm_device *dev) > ASSIGN_READ_MMIO_VFUNCS(vgpu); > } > > + intel_uncore_init_regemu64(dev_priv); > + > i915_check_and_clear_faults(dev); > } > #undef ASSIGN_WRITE_MMIO_VFUNCS > #undef ASSIGN_READ_MMIO_VFUNCS > > +static void intel_uncore_fini_reg64emu(struct drm_device *dev) > +{ > + struct drm_i915_private *i915 = to_i915(dev); > + struct reg64emu *emu, *en; > + > + list_for_each_entry_safe(emu, en, &i915->uncore.reg64_emu_list, link) > + kfree(emu); > + > + INIT_LIST_HEAD(&i915->uncore.reg64_emu_list); > + del_timer_sync(&i915->uncore.reg64_emu_timer); > +} > + > void intel_uncore_fini(struct drm_device *dev) > { > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > intel_uncore_forcewake_reset(dev, false); > + > + intel_uncore_fini_reg64emu(dev); > } > > #define GEN_RANGE(l, h) GENMASK(h, l) > -- > 2.8.0.rc3
On Thu, Apr 07, 2016 at 03:13:51PM +0300, Ville Syrjälä wrote: > On Thu, Apr 07, 2016 at 12:24:13PM +0100, Chris Wilson wrote: > > +static unsigned long calc_overflow_jiffies(struct drm_device *dev) > > { > > - struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_i915_private *dev_priv = to_i915(dev); > > + u32 overflow_ms; > > + > > + /* How many ticks per millisecond? */ > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > + overflow_ms = ~0u / dev_priv->czclk_freq; > > Needs to account for the high range bit. This was the bit I was uncertain about. Does the high range bit imply that is a 24-bit register? Or that the freq is measured differently? Judging by calc_residency() and assuming that the counter wraps at UINT32_MAX, this is still the right calcuations of when it does wrap irrespective of that bit. Hopefully just needs a tweak to freq? I hope we can just ignore the timer aspect of this patch, and if so then handling the 64bit emulation ourselves is also pointless... -Chris
On Thu, Apr 07, 2016 at 01:37:35PM +0100, Chris Wilson wrote: > On Thu, Apr 07, 2016 at 03:13:51PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 07, 2016 at 12:24:13PM +0100, Chris Wilson wrote: > > > +static unsigned long calc_overflow_jiffies(struct drm_device *dev) > > > { > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + u32 overflow_ms; > > > + > > > + /* How many ticks per millisecond? */ > > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > + overflow_ms = ~0u / dev_priv->czclk_freq; > > > > Needs to account for the high range bit. > > This was the bit I was uncertain about. Does the high range bit imply > that is a 24-bit register? Or that the freq is measured differently? The hardware apparently has a 40bit counter internally. In low range mode the register exposes bits [31:0] of the counter, in high range you get to see bits [39:8]. > > Judging by calc_residency() and assuming that the counter wraps at > UINT32_MAX, this is still the right calcuations of when it does wrap > irrespective of that bit. Hopefully just needs a tweak to freq? > > I hope we can just ignore the timer aspect of this patch, and if so then > handling the 64bit emulation ourselves is also pointless... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Thu, Apr 07, 2016 at 03:49:01PM +0300, Ville Syrjälä wrote: > On Thu, Apr 07, 2016 at 01:37:35PM +0100, Chris Wilson wrote: > > On Thu, Apr 07, 2016 at 03:13:51PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 07, 2016 at 12:24:13PM +0100, Chris Wilson wrote: > > > > +static unsigned long calc_overflow_jiffies(struct drm_device *dev) > > > > { > > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > > + u32 overflow_ms; > > > > + > > > > + /* How many ticks per millisecond? */ > > > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > + overflow_ms = ~0u / dev_priv->czclk_freq; > > > > > > Needs to account for the high range bit. > > > > This was the bit I was uncertain about. Does the high range bit imply > > that is a 24-bit register? Or that the freq is measured differently? > > The hardware apparently has a 40bit counter internally. In low range > mode the register exposes bits [31:0] of the counter, in high range > you get to see bits [39:8]. In that case the frequency would be reduced by >>8. Can we set that bit ourselves? That puts the overflow into the 1 hour mark. Thanks, -Chris
On Thu, Apr 07, 2016 at 01:59:44PM +0100, Chris Wilson wrote: > On Thu, Apr 07, 2016 at 03:49:01PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 07, 2016 at 01:37:35PM +0100, Chris Wilson wrote: > > > On Thu, Apr 07, 2016 at 03:13:51PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 07, 2016 at 12:24:13PM +0100, Chris Wilson wrote: > > > > > +static unsigned long calc_overflow_jiffies(struct drm_device *dev) > > > > > { > > > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > > > + u32 overflow_ms; > > > > > + > > > > > + /* How many ticks per millisecond? */ > > > > > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > + overflow_ms = ~0u / dev_priv->czclk_freq; > > > > > > > > Needs to account for the high range bit. > > > > > > This was the bit I was uncertain about. Does the high range bit imply > > > that is a 24-bit register? Or that the freq is measured differently? > > > > The hardware apparently has a 40bit counter internally. In low range > > mode the register exposes bits [31:0] of the counter, in high range > > you get to see bits [39:8]. > > In that case the frequency would be reduced by >>8. > > Can we set that bit ourselves? That puts the overflow into the 1 hour > mark. Thanks, I don't know if it's safe to frob the bit. I worry that something outside our control might depend on it staying put.
On Thu, Apr 07, 2016 at 04:18:16PM +0300, Ville Syrjälä wrote: > On Thu, Apr 07, 2016 at 01:59:44PM +0100, Chris Wilson wrote: > > Can we set that bit ourselves? That puts the overflow into the 1 hour > > mark. Thanks, > > I don't know if it's safe to frob the bit. I worry that something > outside our control might depend on it staying put. A quick frob of the bit says that it is RO. When I try to set it, it doesn't stick. :( -Chris
On Thu, Apr 07, 2016 at 02:58:01PM +0100, Chris Wilson wrote: > On Thu, Apr 07, 2016 at 04:18:16PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 07, 2016 at 01:59:44PM +0100, Chris Wilson wrote: > > > Can we set that bit ourselves? That puts the overflow into the 1 hour > > > mark. Thanks, > > > > I don't know if it's safe to frob the bit. I worry that something > > outside our control might depend on it staying put. > > A quick frob of the bit says that it is RO. When I try to set it, it > doesn't stick. :( Same here on my VLV. I was able to toggle it on my BSW. Perhaps something can lock it down, and my BSW BIOS just doesn't do that.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a1f78f275c55..313d21ec39b7 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -659,6 +659,16 @@ struct intel_uncore { struct intel_uncore_funcs funcs; + /* A few interesting registers track variables over time, + * such as RC6 residencies, in 32bit registers which overflow within + * a matter of seconds (~14s). However, we want to present them as a + * continuous quantity to the user and so we need to keep an overflow + * counter. + */ + struct list_head reg64_emu_list; + struct timer_list reg64_emu_timer; + unsigned long reg64_emu_timeout; + unsigned fifo_count; enum forcewake_domains fw_domains; @@ -3580,6 +3590,8 @@ __raw_write(64, q) #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__)) #define POSTING_READ_FW(reg__) (void)I915_READ_FW(reg__) +u64 i915_read64emu(struct drm_i915_private *dev_priv, i915_reg_t reg); + /* "Broadcast RGB" property */ #define INTEL_BROADCAST_RGB_AUTO 0 #define INTEL_BROADCAST_RGB_FULL 1 diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 2d576b7ff299..6073aa4deaba 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -35,13 +35,27 @@ #define dev_to_drm_minor(d) dev_get_drvdata((d)) #ifdef CONFIG_PM -static u32 calc_residency(struct drm_device *dev, - i915_reg_t reg) +static unsigned long calc_overflow_jiffies(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = to_i915(dev); + u32 overflow_ms; + + /* How many ticks per millisecond? */ + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + overflow_ms = ~0u / dev_priv->czclk_freq; + else if (IS_BROXTON(dev_priv)) + overflow_ms = ~0u / 1200; /* tick every 833ns */ + else + overflow_ms = ~0u / 780; /* tick every 1.28us */ + + return msecs_to_jiffies(overflow_ms); +} + +static unsigned long long calc_residency(struct drm_device *dev, i915_reg_t reg) +{ + struct drm_i915_private *dev_priv = to_i915(dev); u64 raw_time; /* 32b value may overflow during fixed point math */ u64 units = 128ULL, div = 100000ULL; - u32 ret; if (!intel_enable_rc6(dev)) return 0; @@ -49,22 +63,22 @@ static u32 calc_residency(struct drm_device *dev, intel_runtime_pm_get(dev_priv); /* On VLV and CHV, residency time is in CZ units rather than 1.28us */ - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { units = 1; div = dev_priv->czclk_freq; if (I915_READ(VLV_COUNTER_CONTROL) & VLV_COUNT_RANGE_HIGH) units <<= 8; - } else if (IS_BROXTON(dev)) { + } else if (IS_BROXTON(dev_priv)) { units = 1; div = 1200; /* 833.33ns */ } - raw_time = I915_READ(reg) * units; - ret = DIV_ROUND_UP_ULL(raw_time, div); + raw_time = i915_read64emu(dev_priv, reg) * units; intel_runtime_pm_put(dev_priv); - return ret; + + return DIV_ROUND_UP_ULL(raw_time, div); } static ssize_t @@ -78,32 +92,32 @@ static ssize_t show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *dminor = dev_get_drvdata(kdev); - u32 rc6_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6); - return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency); + return snprintf(buf, PAGE_SIZE, "%llu\n", + calc_residency(dminor->dev, GEN6_GT_GFX_RC6)); } static ssize_t show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *dminor = dev_to_drm_minor(kdev); - u32 rc6p_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6p); - return snprintf(buf, PAGE_SIZE, "%u\n", rc6p_residency); + return snprintf(buf, PAGE_SIZE, "%llu\n", + calc_residency(dminor->dev, GEN6_GT_GFX_RC6p)); } static ssize_t show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *dminor = dev_to_drm_minor(kdev); - u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp); - return snprintf(buf, PAGE_SIZE, "%u\n", rc6pp_residency); + return snprintf(buf, PAGE_SIZE, "%llu\n", + calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp)); } static ssize_t show_media_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *dminor = dev_get_drvdata(kdev); - u32 rc6_residency = calc_residency(dminor->dev, VLV_GT_MEDIA_RC6); - return snprintf(buf, PAGE_SIZE, "%u\n", rc6_residency); + return snprintf(buf, PAGE_SIZE, "%llu\n", + calc_residency(dminor->dev, VLV_GT_MEDIA_RC6)); } static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL); @@ -593,22 +607,30 @@ static struct bin_attribute error_state_attr = { void i915_setup_sysfs(struct drm_device *dev) { + struct drm_i915_private *dev_priv = to_i915(dev); int ret; #ifdef CONFIG_PM if (HAS_RC6(dev)) { + dev_priv->uncore.reg64_emu_timeout = + min(dev_priv->uncore.reg64_emu_timeout, + calc_overflow_jiffies(dev) / 2); + calc_residency(dev, GEN6_GT_GFX_RC6); ret = sysfs_merge_group(&dev->primary->kdev->kobj, &rc6_attr_group); if (ret) DRM_ERROR("RC6 residency sysfs setup failed\n"); } if (HAS_RC6p(dev)) { + calc_residency(dev, GEN6_GT_GFX_RC6p); + calc_residency(dev, GEN6_GT_GFX_RC6pp); ret = sysfs_merge_group(&dev->primary->kdev->kobj, &rc6p_attr_group); if (ret) DRM_ERROR("RC6p residency sysfs setup failed\n"); } if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { + calc_residency(dev, VLV_GT_MEDIA_RC6); ret = sysfs_merge_group(&dev->primary->kdev->kobj, &media_rc6_attr_group); if (ret) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index fbc1d215ca67..74b3468c3b1a 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1247,6 +1247,70 @@ static void intel_uncore_fw_domains_init(struct drm_device *dev) WARN_ON(dev_priv->uncore.fw_domains == 0); } +struct reg64emu { + i915_reg_t reg; + u64 last; + struct list_head link; +}; + +static u64 __read64emu(struct drm_i915_private *dev_priv, struct reg64emu *emu) +{ + u64 result; + + result = I915_READ(emu->reg) | (emu->last & 0xffffffff00000000); + if (lower_32_bits(result) < lower_32_bits(emu->last)) + result += 1ull << 32; + emu->last = result; + + return result; +} + +u64 i915_read64emu(struct drm_i915_private *dev_priv, i915_reg_t reg) +{ + struct reg64emu *emu; + + list_for_each_entry(emu, &dev_priv->uncore.reg64_emu_list, link) + if (i915_mmio_reg_offset(emu->reg) == i915_mmio_reg_offset(reg)) + goto read; + + emu = kmalloc(sizeof(*emu), GFP_KERNEL); + if (emu == NULL) + return I915_READ(reg); + + emu->reg = reg; + emu->last = 0; + list_add(&emu->link, &dev_priv->uncore.reg64_emu_list); + + if (!timer_pending(&dev_priv->uncore.reg64_emu_timer)) + mod_timer(&dev_priv->uncore.reg64_emu_timer, + jiffies + dev_priv->uncore.reg64_emu_timeout); + +read: + return __read64emu(dev_priv, emu); +} + +static void intel_uncore_reg64_emu_timer(unsigned long arg) +{ + struct drm_i915_private *dev_priv = (struct drm_i915_private *)arg; + struct reg64emu *emu; + + list_for_each_entry(emu, &dev_priv->uncore.reg64_emu_list, link) + __read64emu(dev_priv, emu); + + mod_timer(&dev_priv->uncore.reg64_emu_timer, + jiffies + dev_priv->uncore.reg64_emu_timeout); +} + +static void intel_uncore_init_regemu64(struct drm_i915_private *dev_priv) +{ + dev_priv->uncore.reg64_emu_timeout = MAX_JIFFY_OFFSET; + + INIT_LIST_HEAD(&dev_priv->uncore.reg64_emu_list); + setup_timer(&dev_priv->uncore.reg64_emu_timer, + intel_uncore_reg64_emu_timer, + (unsigned long)dev_priv); +} + void intel_uncore_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -1306,16 +1370,32 @@ void intel_uncore_init(struct drm_device *dev) ASSIGN_READ_MMIO_VFUNCS(vgpu); } + intel_uncore_init_regemu64(dev_priv); + i915_check_and_clear_faults(dev); } #undef ASSIGN_WRITE_MMIO_VFUNCS #undef ASSIGN_READ_MMIO_VFUNCS +static void intel_uncore_fini_reg64emu(struct drm_device *dev) +{ + struct drm_i915_private *i915 = to_i915(dev); + struct reg64emu *emu, *en; + + list_for_each_entry_safe(emu, en, &i915->uncore.reg64_emu_list, link) + kfree(emu); + + INIT_LIST_HEAD(&i915->uncore.reg64_emu_list); + del_timer_sync(&i915->uncore.reg64_emu_timer); +} + void intel_uncore_fini(struct drm_device *dev) { /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev); intel_uncore_forcewake_reset(dev, false); + + intel_uncore_fini_reg64emu(dev); } #define GEN_RANGE(l, h) GENMASK(h, l)
RC6 residency counters are kept by the hardware in 32bit registers, which then overflow in a couple of hours. Ideally, we want a continuous count of the residency since boot and so we must maintain an overflow counter ourselves. Utilities such as powertop read these registers through sysfs and the unexpected wrapping produces inconsistent results and garbage for the user. In order to detect the overflow, without intervention, we keep a list of the interesting registers and read them every few seconds. RFC: Every 14 seconds!!! Is it worth imposing that timer on everybody for the few users of sysfs? Does userspace only truly care about the relative value and not the absolute value since boot? If so, should we just limit the timer to when the sysfs file is open? That would need userspace to keep said file open for as long as it wants accurate results. Reported-by: Len Brown <lenb@kernel.org> Refernces: https://bugs.freedesktop.org/show_bug.cgi?id=94852 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Len Brown <lenb@kernel.org> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 12 ++++++ drivers/gpu/drm/i915/i915_sysfs.c | 56 ++++++++++++++++++-------- drivers/gpu/drm/i915/intel_uncore.c | 80 +++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 17 deletions(-)