* i915_chipset_val()
@ 2011-07-08 9:26 Konstantin Belousov
2011-07-08 9:42 ` i915_chipset_val() Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Konstantin Belousov @ 2011-07-08 9:26 UTC (permalink / raw)
To: Intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2033 bytes --]
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).
diff --git a/sys/dev/drm/i915_dma.c b/sys/dev/drm/i915_dma.c
index a22643f..773a171 100644
--- a/sys/dev/drm/i915_dma.c
+++ b/sys/dev/drm/i915_dma.c
@@ -1588,9 +1588,14 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
int i;
diff1 = now - dev_priv->last_time1;
+ /*
+ * 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.
+ */
+ if (diff1 == 0)
+ return (dev_priv->last_chipset_val);
count1 = I915_READ(DMIEC);
count2 = I915_READ(DDREC);
@@ -1622,7 +1627,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
dev_priv->last_count1 = total_count;
dev_priv->last_time1 = now;
- return ret;
+ dev_priv->last_chipset_val = ret;
+ return (ret);
}
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;
[-- Attachment #1.2: Type: application/pgp-signature, Size: 196 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: i915_chipset_val()
2011-07-08 9:26 i915_chipset_val() Konstantin Belousov
@ 2011-07-08 9:42 ` Chris Wilson
2011-07-08 12:24 ` i915_chipset_val() Konstantin Belousov
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2011-07-08 9:42 UTC (permalink / raw)
To: Konstantin Belousov, Intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: i915_chipset_val()
2011-07-08 9:42 ` i915_chipset_val() Chris Wilson
@ 2011-07-08 12:24 ` Konstantin Belousov
2011-07-08 14:21 ` i915_chipset_val() Jesse Barnes
0 siblings, 1 reply; 4+ messages in thread
From: Konstantin Belousov @ 2011-07-08 12:24 UTC (permalink / raw)
To: Chris Wilson; +Cc: Intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2424 bytes --]
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);
[-- Attachment #1.2: Type: application/pgp-signature, Size: 196 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: i915_chipset_val()
2011-07-08 12:24 ` i915_chipset_val() Konstantin Belousov
@ 2011-07-08 14:21 ` Jesse Barnes
0 siblings, 0 replies; 4+ messages in thread
From: Jesse Barnes @ 2011-07-08 14:21 UTC (permalink / raw)
To: Konstantin Belousov; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-08 14:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-08 9:26 i915_chipset_val() Konstantin Belousov
2011-07-08 9:42 ` i915_chipset_val() Chris Wilson
2011-07-08 12:24 ` i915_chipset_val() Konstantin Belousov
2011-07-08 14:21 ` i915_chipset_val() Jesse Barnes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).