[PATCH] drm/i915: add a tracepoint for gpu frequency changes

[PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Daniel Vetter]

We've had and still have too many issues where the gpu turbot doesn't
quite to what it's supposed to do (or what we want it to do).

Adding a tracepoint to track when the desired gpu frequence changes
should help a lot in characterizing and understanding problematic
workloads.

Also, this should be fairly interesting for power tuning (and
especially noticing when the gpu is stuck in high frequencies, as has
happened in the past) and hence for integration into powertop and
similar tools.

Cc: Arjan van de Ven <arjan@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_trace.h |   15 +++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c   |    2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 3c4093d..8134421 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -430,6 +430,21 @@ TRACE_EVENT(i915_reg_rw,
 		(u32)(__entry->val >> 32))
 );
 
+TRACE_EVENT(intel_gpu_freq_change,
+	    TP_PROTO(u32 freq),
+	    TP_ARGS(freq),
+
+	    TP_STRUCT__entry(
+			     __field(u32, freq)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->freq = freq;
+			   ),
+
+	    TP_printk("new_freq=%u", __entry->freq)
+);
+
 #endif /* _I915_TRACE_H_ */
 
 /* This part must be outside protection */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ebe3498..194a72f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2343,6 +2343,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
 
 	dev_priv->rps.cur_delay = val;
+
+	trace_intel_gpu_freq_change(val * 50);
 }
 
 static void gen6_disable_rps(struct drm_device *dev)
-- 
1.7.10.4

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Chris Wilson]

On Thu, 30 Aug 2012 13:26:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We've had and still have too many issues where the gpu turbot doesn't
> quite to what it's supposed to do (or what we want it to do).
> 
> Adding a tracepoint to track when the desired gpu frequence changes
> should help a lot in characterizing and understanding problematic
> workloads.
> 
> Also, this should be fairly interesting for power tuning (and
> especially noticing when the gpu is stuck in high frequencies, as has
> happened in the past) and hence for integration into powertop and
> similar tools.
> 
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Like it, even the naming scheme.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 2282 bytes --]

Am Donnerstag, den 30.08.2012, 13:26 +0200 schrieb Daniel Vetter:
> We've had and still have too many issues where the gpu turbot doesn't

s,turbot,turbo,

> quite to what it's supposed to do (or what we want it to do).

s,to,do,

> Adding a tracepoint to track when the desired gpu frequence changes

frequenc*y*

> should help a lot in characterizing and understanding problematic
> workloads.
> 
> Also, this should be fairly interesting for power tuning (and
> especially noticing when the gpu is stuck in high frequencies, as has
> happened in the past) and hence for integration into powertop and
> similar tools.

If this can be used from user space already, it would be nice to add
some notes to the commit how this can be done.

> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_trace.h |   15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c   |    2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 3c4093d..8134421 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -430,6 +430,21 @@ TRACE_EVENT(i915_reg_rw,
>  		(u32)(__entry->val >> 32))
>  );
>  
> +TRACE_EVENT(intel_gpu_freq_change,
> +	    TP_PROTO(u32 freq),
> +	    TP_ARGS(freq),
> +
> +	    TP_STRUCT__entry(
> +			     __field(u32, freq)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->freq = freq;
> +			   ),
> +
> +	    TP_printk("new_freq=%u", __entry->freq)
> +);
> +
>  #endif /* _I915_TRACE_H_ */
>  
>  /* This part must be outside protection */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ebe3498..194a72f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2343,6 +2343,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
>  
>  	dev_priv->rps.cur_delay = val;
> +
> +	trace_intel_gpu_freq_change(val * 50);
>  }
>  
>  static void gen6_disable_rps(struct drm_device *dev)

Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Daniel Vetter]

On Thu, Aug 30, 2012 at 02:34:11PM +0100, Chris Wilson wrote:
> On Thu, 30 Aug 2012 13:26:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We've had and still have too many issues where the gpu turbot doesn't
> > quite to what it's supposed to do (or what we want it to do).
> > 
> > Adding a tracepoint to track when the desired gpu frequence changes
> > should help a lot in characterizing and understanding problematic
> > workloads.
> > 
> > Also, this should be fairly interesting for power tuning (and
> > especially noticing when the gpu is stuck in high frequencies, as has
> > happened in the past) and hence for integration into powertop and
> > similar tools.
> > 
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Like it, even the naming scheme.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-08-30 04:26, Daniel Vetter wrote:
> We've had and still have too many issues where the gpu turbot doesn't
> quite to what it's supposed to do (or what we want it to do).
>
> Adding a tracepoint to track when the desired gpu frequence changes
> should help a lot in characterizing and understanding problematic
> workloads.
>
> Also, this should be fairly interesting for power tuning (and
> especially noticing when the gpu is stuck in high frequencies, as has
> happened in the past) and hence for integration into powertop and
> similar tools.
>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I can't help but think it's equally interesting to know when the queue 
the work as well.


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Arjan van de Ven]

On 9/1/2012 11:26 AM, Ben Widawsky wrote:
> On 2012-08-30 04:26, Daniel Vetter wrote:
>> We've had and still have too many issues where the gpu turbot doesn't
>> quite to what it's supposed to do (or what we want it to do).
>>
>> Adding a tracepoint to track when the desired gpu frequence changes
>> should help a lot in characterizing and understanding problematic
>> workloads.
>>
>> Also, this should be fairly interesting for power tuning (and
>> especially noticing when the gpu is stuck in high frequencies, as has
>> happened in the past) and hence for integration into powertop and
>> similar tools.
>>
>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I can't help but think it's equally interesting to know when the queue the work as well.
> 
> 

btw... if the userspace interface (e.g. the actual event) is not controversial and very unlikely to change,
I'd like to start coding the powertop support for this already....

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-09-01 11:28, Arjan van de Ven wrote:
> On 9/1/2012 11:26 AM, Ben Widawsky wrote:
>> On 2012-08-30 04:26, Daniel Vetter wrote:
>>> We've had and still have too many issues where the gpu turbot 
>>> doesn't
>>> quite to what it's supposed to do (or what we want it to do).
>>>
>>> Adding a tracepoint to track when the desired gpu frequence changes
>>> should help a lot in characterizing and understanding problematic
>>> workloads.
>>>
>>> Also, this should be fairly interesting for power tuning (and
>>> especially noticing when the gpu is stuck in high frequencies, as 
>>> has
>>> happened in the past) and hence for integration into powertop and
>>> similar tools.
>>>
>>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> I can't help but think it's equally interesting to know when the 
>> queue the work as well.
>>
>>
>
> btw... if the userspace interface (e.g. the actual event) is not
> controversial and very unlikely to change,
> I'd like to start coding the powertop support for this already....

I have no problem with Daniel's patch. It's just a matter of cutting 
through some scheduler BS of "when the GPU wants to change frequency" 
vs. "when we actually change the GPU frequency." I think *both* are 
interesting.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Daniel Vetter]

On Sat, Sep 1, 2012 at 8:35 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 2012-09-01 11:28, Arjan van de Ven wrote:
>>
>> On 9/1/2012 11:26 AM, Ben Widawsky wrote:
>>>
>>> On 2012-08-30 04:26, Daniel Vetter wrote:
>>>>
>>>> We've had and still have too many issues where the gpu turbot doesn't
>>>> quite to what it's supposed to do (or what we want it to do).
>>>>
>>>> Adding a tracepoint to track when the desired gpu frequence changes
>>>> should help a lot in characterizing and understanding problematic
>>>> workloads.
>>>>
>>>> Also, this should be fairly interesting for power tuning (and
>>>> especially noticing when the gpu is stuck in high frequencies, as has
>>>> happened in the past) and hence for integration into powertop and
>>>> similar tools.
>>>>
>>>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>>
>>> I can't help but think it's equally interesting to know when the queue
>>> the work as well.
>>>
>>>
>>
>> btw... if the userspace interface (e.g. the actual event) is not
>> controversial and very unlikely to change,
>> I'd like to start coding the powertop support for this already....
>
>
> I have no problem with Daniel's patch. It's just a matter of cutting through
> some scheduler BS of "when the GPU wants to change frequency" vs. "when we
> actually change the GPU frequency." I think *both* are interesting.

Hm, aren't there some neat tracepoints to measure the latency of work
items around already? I agree that this might be useful, but just
adding a tracepoint for this one workqueue item feels like overkill
...

Arjan, the tracepoint patch is merged already into
drm-intel-next-queued, i.e. powertop integration is good to go.

Thanks, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Chris Wilson]

On Sat, 01 Sep 2012 11:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I have no problem with Daniel's patch. It's just a matter of cutting 
> through some scheduler BS of "when the GPU wants to change frequency" 
> vs. "when we actually change the GPU frequency." I think *both* are 
> interesting.

We already trace the interrupt, which has been invaluable in
debugging. And there is no other source currently... :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-09-01 12:22, Chris Wilson wrote:
> On Sat, 01 Sep 2012 11:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> I have no problem with Daniel's patch. It's just a matter of cutting
>> through some scheduler BS of "when the GPU wants to change 
>> frequency"
>> vs. "when we actually change the GPU frequency." I think *both* are
>> interesting.
>
> We already trace the interrupt, which has been invaluable in
> debugging. And there is no other source currently... :)
> -Chris

No we do not trace it (/me is looking on cgit, so perhaps a bit error 
prone). Sure we trace GT interrupts, but not pm interrupts (again, 
unless I am missing something via code browsing on the web).

Furthermore, even if we have added a generic interrupt trace event and 
I've missed it: I think it's still nice to have a power/performance 
specific one for users like powerTOP, as opposed to driver developers.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-09-01 12:14, Daniel Vetter wrote:
> On Sat, Sep 1, 2012 at 8:35 PM, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> On 2012-09-01 11:28, Arjan van de Ven wrote:
>>>
>>> On 9/1/2012 11:26 AM, Ben Widawsky wrote:
>>>>
>>>> On 2012-08-30 04:26, Daniel Vetter wrote:
>>>>>
>>>>> We've had and still have too many issues where the gpu turbot 
>>>>> doesn't
>>>>> quite to what it's supposed to do (or what we want it to do).
>>>>>
>>>>> Adding a tracepoint to track when the desired gpu frequence 
>>>>> changes
>>>>> should help a lot in characterizing and understanding problematic
>>>>> workloads.
>>>>>
>>>>> Also, this should be fairly interesting for power tuning (and
>>>>> especially noticing when the gpu is stuck in high frequencies, as 
>>>>> has
>>>>> happened in the past) and hence for integration into powertop and
>>>>> similar tools.
>>>>>
>>>>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>>
>>>> I can't help but think it's equally interesting to know when the 
>>>> queue
>>>> the work as well.
>>>>
>>>>
>>>
>>> btw... if the userspace interface (e.g. the actual event) is not
>>> controversial and very unlikely to change,
>>> I'd like to start coding the powertop support for this already....
>>
>>
>> I have no problem with Daniel's patch. It's just a matter of cutting 
>> through
>> some scheduler BS of "when the GPU wants to change frequency" vs. 
>> "when we
>> actually change the GPU frequency." I think *both* are interesting.
>
> Hm, aren't there some neat tracepoints to measure the latency of work
> items around already? I agree that this might be useful, but just
> adding a tracepoint for this one workqueue item feels like overkill

It depends on what you're trying to measure. I think this patch is 
quite useful but I think I'll make you defend your patch now since 
you're the maintainer and you took your own patch and you're shooting 
down my idea. So please tell me what PowerTOP should do with this patch 
other than notice we're stuck (and furthermore, even if we're stuck, 
what should it tell us to do)?

As you may recall we can get multiple up and down interrupts, and we 
coalesce them and only do one thing. Information is lost there that 
could have been useful; caveat to that - I haven't looked at the code in 
a while, but that's what we used to do. What I mean though is, if we get 
an up then down interrupt, in that order, it will go out as a trace 
event that we're raising the GPU frequency (again, previous caveat 
applies). So I think this event + the current GPU frequency is more 
interesting than just when we change the frequency; however all 3 would 
be even better for finding driver bugs.

More on tracing at the interrupt time: I think getting this info to 
userspace is somewhat less useful than tying it into some kind of CPU 
governor hooks. For example, if we get multiple consecutive RP down 
interrupts, we can probably add it to a list of reasons we might want to 
lower the CPU frequency, and the contrapositive is also true.

> ...
>
> Arjan, the tracepoint patch is merged already into
> drm-intel-next-queued, i.e. powertop integration is good to go.
>
> Thanks, Daniel

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Arjan van de Ven]

On 9/1/2012 6:36 PM, Ben Widawsky wrote:

> It depends on what you're trying to measure. I think this patch is quite useful but I think I'll make you defend your patch now since you're the maintainer and you took
> your own patch and you're shooting down my idea. So please tell me what PowerTOP should do with this patch other than notice we're stuck (and furthermore, even if we're
> stuck, what should it tell us to do)?

what I would like to do, as the powertop guy.... is to show frequency stats similar to how we do that
for CPUs. E.g. as close as to actual as we can get.
few questions:
1) Can I read the momentary frequency somewhere?
(it's great to get change events, but I also need a start frequency, otherwise I have a gap in
knowledge from start of measurement to first change event)
2) Can I read a "hardware average" somewhere, similar to what we can do for cpus ?

Re: [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-09-01 19:44, Arjan van de Ven wrote:
> On 9/1/2012 6:36 PM, Ben Widawsky wrote:
>
>> It depends on what you're trying to measure. I think this patch is 
>> quite useful but I think I'll make you defend your patch now since 
>> you're the maintainer and you took
>> your own patch and you're shooting down my idea. So please tell me 
>> what PowerTOP should do with this patch other than notice we're stuck 
>> (and furthermore, even if we're
>> stuck, what should it tell us to do)?
>
> what I would like to do, as the powertop guy.... is to show frequency
> stats similar to how we do that
> for CPUs. E.g. as close as to actual as we can get.
> few questions:
> 1) Can I read the momentary frequency somewhere?
> (it's great to get change events, but I also need a start frequency,
> otherwise I have a gap in
> knowledge from start of measurement to first change event)

I forget offhand if we ever added this. Daniel did ask me to do it a 
long time ago and I never got around to it. OTOH his trace event does 
tell you what frequency it's changed to, and the up/down steps should be 
incremental. In other words, from a single trace event you should know 
the current frequency and the previous frequency. Besides, eventually 
we'll just add this to systemd and load it before i915, right :p?

> 2) Can I read a "hardware average" somewhere, similar to what we can
> do for cpus ?

I'm not aware of an easy way to do this. The way the programming of 
these up and down events sort of does that though. There are various 
heuristics we have to configure via registers to get the HW to generate 
the interrupts at all. In other words if you decode the way the 
interrupts are generated and record a few events, you may get something 
that almost resembles an average. Honestly, it gets quite complicated as 
you might imagine and as such many of these values are opaque to us 
(magic numbers handed down through the ages). We could try to engage 
with mother Intel to find out from the designers how we could go about 
doing this in a more suitable way.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Arjan van de Ven]

On 9/1/2012 8:05 PM, Ben Widawsky wrote:
> , from a single trace event you should know the current frequency and the previous frequency.
but if you're doing a heavy game or whatever... you may stay at the highest all the time,
and I get no information...
same for getting stuck or being always low.

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Ben Widawsky]

On 2012-09-01 20:06, Arjan van de Ven wrote:
> On 9/1/2012 8:05 PM, Ben Widawsky wrote:
>> , from a single trace event you should know the current frequency 
>> and the previous frequency.
> but if you're doing a heavy game or whatever... you may stay at the
> highest all the time,
> and I get no information...
> same for getting stuck or being always low.

True. Well, as I said, Daniel did ask me to do it. I will send out a 
patch this weekend, it's pretty trivial.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Chris Wilson]

On Sat, 01 Sep 2012 18:16:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 2012-09-01 12:22, Chris Wilson wrote:
> > On Sat, 01 Sep 2012 11:35:13 -0700, Ben Widawsky <ben@bwidawsk.net> 
> > wrote:
> >> I have no problem with Daniel's patch. It's just a matter of cutting
> >> through some scheduler BS of "when the GPU wants to change 
> >> frequency"
> >> vs. "when we actually change the GPU frequency." I think *both* are
> >> interesting.
> >
> > We already trace the interrupt, which has been invaluable in
> > debugging. And there is no other source currently... :)
> > -Chris
> 
> No we do not trace it (/me is looking on cgit, so perhaps a bit error 
> prone). Sure we trace GT interrupts, but not pm interrupts (again, 
> unless I am missing something via code browsing on the web).

Oh we definitely do otherwise it would have been a bit difficult to have
noticed that we woke up every 10ms to service a PM event that we then
ignore...

> Furthermore, even if we have added a generic interrupt trace event and 
> I've missed it: I think it's still nice to have a power/performance 
> specific one for users like powerTOP, as opposed to driver developers.

As complete BS as GPU op/s? If you can find a tracepoint that is
actionable from within powertop, go for it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Daniel Vetter]

On Sat, Sep 01, 2012 at 06:36:32PM -0700, Ben Widawsky wrote:
> On 2012-09-01 12:14, Daniel Vetter wrote:
> >On Sat, Sep 1, 2012 at 8:35 PM, Ben Widawsky <ben@bwidawsk.net>
> >wrote:
> >>On 2012-09-01 11:28, Arjan van de Ven wrote:
> >>>
> >>>On 9/1/2012 11:26 AM, Ben Widawsky wrote:
> >>>>
> >>>>On 2012-08-30 04:26, Daniel Vetter wrote:
> >>>>>
> >>>>>We've had and still have too many issues where the gpu
> >>>>>turbot doesn't
> >>>>>quite to what it's supposed to do (or what we want it to do).
> >>>>>
> >>>>>Adding a tracepoint to track when the desired gpu
> >>>>>frequence changes
> >>>>>should help a lot in characterizing and understanding problematic
> >>>>>workloads.
> >>>>>
> >>>>>Also, this should be fairly interesting for power tuning (and
> >>>>>especially noticing when the gpu is stuck in high
> >>>>>frequencies, as has
> >>>>>happened in the past) and hence for integration into powertop and
> >>>>>similar tools.
> >>>>>
> >>>>>Cc: Arjan van de Ven <arjan@linux.intel.com>
> >>>>>Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>
> >>>>
> >>>>I can't help but think it's equally interesting to know when
> >>>>the queue
> >>>>the work as well.
> >>>>
> >>>>
> >>>
> >>>btw... if the userspace interface (e.g. the actual event) is not
> >>>controversial and very unlikely to change,
> >>>I'd like to start coding the powertop support for this already....
> >>
> >>
> >>I have no problem with Daniel's patch. It's just a matter of
> >>cutting through
> >>some scheduler BS of "when the GPU wants to change frequency"
> >>vs. "when we
> >>actually change the GPU frequency." I think *both* are interesting.
> >
> >Hm, aren't there some neat tracepoints to measure the latency of work
> >items around already? I agree that this might be useful, but just
> >adding a tracepoint for this one workqueue item feels like overkill
> 
> It depends on what you're trying to measure. I think this patch is
> quite useful but I think I'll make you defend your patch now since
> you're the maintainer and you took your own patch and you're
> shooting down my idea. So please tell me what PowerTOP should do
> with this patch other than notice we're stuck (and furthermore, even
> if we're stuck, what should it tell us to do)?

Actually it shouldn't only notice that we're stuck but e.g. also notice
that a blinking cursor keeps us at high gpu clock (which together with the
low rc6 residency should explain the power usage in these scenarios). Or
maybe integrate it into a graphical overview (but to make that useful we
first need to figure out how to add precise tracepoints for
batch_begin/end) so that interactions between gpu/cpu stand out more.

> As you may recall we can get multiple up and down interrupts, and we
> coalesce them and only do one thing. Information is lost there that
> could have been useful; caveat to that - I haven't looked at the
> code in a while, but that's what we used to do. What I mean though
> is, if we get an up then down interrupt, in that order, it will go
> out as a trace event that we're raising the GPU frequency (again,
> previous caveat applies). So I think this event + the current GPU
> frequency is more interesting than just when we change the
> frequency; however all 3 would be even better for finding driver
> bugs.

The added tracepoints gives us an event when we actually change the hw
state - which is imo the important thing to measure for performance tuning
and diagnosing issues. Figuring out _why_ things are amiss is then the
usual gfx driver debug madness, and I think adding a bunch of tracepoints
specifically just for that feels like overwill.

> More on tracing at the interrupt time: I think getting this info to
> userspace is somewhat less useful than tying it into some kind of
> CPU governor hooks. For example, if we get multiple consecutive RP
> down interrupts, we can probably add it to a list of reasons we
> might want to lower the CPU frequency, and the contrapositive is
> also true.

I didn't add the tracepoint at irq time, but only where we change the gpu
clock. And the tracepoint dumps the new gpu clock freq we've just set, so
no way to get out of sync with down/up irqs, either.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH] drm/i915: add a tracepoint for gpu frequency changes

[Daniel Vetter]

On Sat, Sep 01, 2012 at 07:44:17PM -0700, Arjan van de Ven wrote:
> On 9/1/2012 6:36 PM, Ben Widawsky wrote:
> 
> > It depends on what you're trying to measure. I think this patch is quite useful but I think I'll make you defend your patch now since you're the maintainer and you took
> > your own patch and you're shooting down my idea. So please tell me what PowerTOP should do with this patch other than notice we're stuck (and furthermore, even if we're
> > stuck, what should it tell us to do)?
> 
> what I would like to do, as the powertop guy.... is to show frequency stats similar to how we do that
> for CPUs. E.g. as close as to actual as we can get.
> few questions:
> 1) Can I read the momentary frequency somewhere?
> (it's great to get change events, but I also need a start frequency, otherwise I have a gap in
> knowledge from start of measurement to first change event)

Oops, forgotten about this, but it looks like Ben has already volunteered
himself for those ;-)

> 2) Can I read a "hardware average" somewhere, similar to what we can do for cpus ?

Afaik, there's nothing. But since the cpu controls the gpu freq
completely, you can just compute those yourselves from the tracepoints.
Together with the rc6 residency timers we already expose this should give
a neat overview of how busy the gpu is and how much power it blows through
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48