On Fri, Jul 08, 2011 at 10:42:02AM +0100, Chris Wilson wrote: > On Fri, 8 Jul 2011 12:26:51 +0300, Konstantin Belousov <[email protected]> > 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);
pgpJRtvm35jed.pgp
Description: PGP signature
_______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
