| Message ID | 1346326008-11186-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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....
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.
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
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
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.
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
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 ?
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.
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.
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.
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
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
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
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)
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(+)