| Message ID | 20110708122434.GW48734@deviant.kiev.zoral.com.ua (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 8 Jul 2011 15:24:34 +0300 Konstantin Belousov <kostikbel@gmail.com> wrote: > On Fri, Jul 08, 2011 at 10:42:02AM +0100, Chris Wilson wrote: > > On Fri, 8 Jul 2011 12:26:51 +0300, Konstantin Belousov <kostikbel@gmail.com> wrote: > > > i915_chipset_val saves the jiffies count of the invocation in last_time1. > > > Then, on the next call, the diff between current jiffies value and > > > last_time1 is used as divisor. > > > > > > I have a suspicious that two rapid calls to i915_chipset_val() may result > > > in division by zero. This looks as user-controllable action, since > > > debugfs, if configured, would export i915_chipset_val() as emon status. > > > > > > I did not tested the Linux, but in the (ported) code I am able to get > > > into the described situation. As a workaround, I cached the previous > > > return value from i915_chipset_val() and return it instead of doing > > > the calculation if consequtive calls are close enough. > > > > > > What do you think ? (patch is not directly applicable to Linux). > > > > You're right. But I would go even further and say that if the difference > > is less than say 10ms then we have not accumulated enough samples for the > > calculation to be stable. > > > > > unsigned long i915_mch_val(struct drm_i915_private *dev_priv) > > > diff --git a/sys/dev/drm/i915_drv.h b/sys/dev/drm/i915_drv.h > > > index 5e6340b..36d066a 100644 > > > --- a/sys/dev/drm/i915_drv.h > > > +++ b/sys/dev/drm/i915_drv.h > > > @@ -579,6 +579,7 @@ typedef struct drm_i915_private { > > > > > > u64 last_count1; > > > unsigned long last_time1; > > > + unsigned long last_chipset_val; > > > u64 last_count2; > > > struct timespec last_time2; > > > unsigned long gfx_power; > > > > We need to thank Jesse for such informative variable names. Not even a > > comment to mention that they are part of IPS. > > I agree, I did the following, relative to the previous patch > > diff --git a/sys/dev/drm/i915_dma.c b/sys/dev/drm/i915_dma.c > index 773a171..07100f4 100644 > --- a/sys/dev/drm/i915_dma.c > +++ b/sys/dev/drm/i915_dma.c > @@ -1592,9 +1592,9 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv) > * sysctl(8) reads the value of sysctl twice in rapid > * succession. There is high chance that it happens in the > * same timer tick. Use the cached value to not divide by > - * zero. > + * zero and give the hw a chance to gather more samples. > */ > - if (diff1 == 0) > + if (diff1 <= 10) > return (dev_priv->last_chipset_val); > > count1 = I915_READ(DMIEC); Yeah, sounds good. Thanks!
diff --git a/sys/dev/drm/i915_dma.c b/sys/dev/drm/i915_dma.c index 773a171..07100f4 100644 --- a/sys/dev/drm/i915_dma.c +++ b/sys/dev/drm/i915_dma.c @@ -1592,9 +1592,9 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv) * sysctl(8) reads the value of sysctl twice in rapid * succession. There is high chance that it happens in the * same timer tick. Use the cached value to not divide by - * zero. + * zero and give the hw a chance to gather more samples. */ - if (diff1 == 0) + if (diff1 <= 10) return (dev_priv->last_chipset_val); count1 = I915_READ(DMIEC);