Re: i915_chipset_val()

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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!

-- 
Jesse Barnes, Intel Open Source Technology Center