From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: i915_chipset_val() Date: Fri, 8 Jul 2011 07:21:46 -0700 Message-ID: <20110708072146.22609f4a@jbarnes-desktop> References: <20110708092651.GN48734@deviant.kiev.zoral.com.ua> <20110708122434.GW48734@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy8-pub.bluehost.com (oproxy8-pub.bluehost.com [69.89.22.20]) by gabe.freedesktop.org (Postfix) with SMTP id E042EA024E for ; Fri, 8 Jul 2011 07:21:49 -0700 (PDT) In-Reply-To: <20110708122434.GW48734@deviant.kiev.zoral.com.ua> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Konstantin Belousov Cc: Intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Fri, 8 Jul 2011 15:24:34 +0300 Konstantin Belousov 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 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