From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: i915_chipset_val() Date: Fri, 08 Jul 2011 10:42:02 +0100 Message-ID: References: <20110708092651.GN48734@deviant.kiev.zoral.com.ua> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id DBBF99ECF1 for ; Fri, 8 Jul 2011 02:42:05 -0700 (PDT) In-Reply-To: <20110708092651.GN48734@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 , Intel-gfx List-Id: intel-gfx@lists.freedesktop.org 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. -Chris -- Chris Wilson, Intel Open Source Technology Centre