Re: i915_chipset_val()

[Chris Wilson <chris@chris-wilson.co.uk>]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre