| Message ID | 1416345149-16985-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate
BYT: pass/total=290/290->290/290
PNV: pass/total=362/362->362/362
ILK: pass/total=381/381->379/381
IVB: pass/total=522/559->522/559
SNB: pass/total=444/444->444/444
HSW: pass/total=595/595->595/595
BDW: pass/total=436/436->436/436
-------------------------------------Detailed-------------------------------------
test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)...
ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-wf_vblank-interruptible, DMESG_WARN(2, M26)PASS(2, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
ILK: Intel_gpu_tools, igt_kms_flip_single-buffer-flip-vs-dpms-off-vs-modeset, DMESG_WARN(1, M26)PASS(3, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26)
On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > Might be helpful for debugging places where userspace ends up boosting > or waiting where it doesn't intend to. Might be feels a bit weak justification for a new tracepoint imo. Please drum up more support. > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > drivers/gpu/drm/i915/i915_trace.h | 15 +++++++++++++++ > drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 86cf428..b03cb07 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4209,8 +4209,10 @@ i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj) > struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); > > BUG_ON(!vma); > - BUG_ON(vma->pin_count == 0); > - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); > + if (WARN(vma->pin_count == 0, "bad pin count\n")) > + return; > + if (WARN(!i915_gem_obj_ggtt_bound(obj), "obj not bound\n")) > + return; Separate patch. Can you please split it out with the usual "BUG_ON considered harmful" commit message? Thanks, Daniel > > if (--vma->pin_count == 0) > obj->pin_mappable = false; > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index 751d4ad..d710fe1 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -691,6 +691,21 @@ TRACE_EVENT(switch_mm, > __entry->dev, __entry->ring, __entry->to, __entry->vm) > ); > > +TRACE_EVENT(turbo_boost, > + TP_PROTO(u32 freq), > + TP_ARGS(freq), > + > + TP_STRUCT__entry( > + __field(u32, freq) > + ), > + > + TP_fast_assign( > + __entry->freq = freq; > + ), > + > + TP_printk("turbo boost to %d MHz", __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 7558ba2..2944593 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4483,10 +4483,15 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) > > mutex_lock(&dev_priv->rps.hw_lock); > if (dev_priv->rps.enabled) { > - if (IS_VALLEYVIEW(dev)) > + if (IS_VALLEYVIEW(dev)) { > valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); > - else > + trace_turbo_boost(vlv_gpu_freq(dev_priv, > + dev_priv->rps.max_freq_softlimit)); > + } else { > gen6_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); > + trace_turbo_boost(dev_priv->rps.max_freq_softlimit * 50); > + } > + > dev_priv->rps.last_adj = 0; > } > mutex_unlock(&dev_priv->rps.hw_lock); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 19, 2014 at 03:00:04PM +0100, Daniel Vetter wrote: > On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > > Might be helpful for debugging places where userspace ends up boosting > > or waiting where it doesn't intend to. > > Might be feels a bit weak justification for a new tracepoint imo. Please > drum up more support. > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > > drivers/gpu/drm/i915/i915_trace.h | 15 +++++++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > 3 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 86cf428..b03cb07 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4209,8 +4209,10 @@ i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj) > > struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); > > > > BUG_ON(!vma); > > - BUG_ON(vma->pin_count == 0); > > - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); > > + if (WARN(vma->pin_count == 0, "bad pin count\n")) > > + return; > > + if (WARN(!i915_gem_obj_ggtt_bound(obj), "obj not bound\n")) > > + return; > > Separate patch. Can you please split it out with the usual "BUG_ON > considered harmful" commit message? I have argued that these represent extreme fail on the part of the programmer - you have left the hardware accessing random physical pages in the system, game over. I need a stronger argument than that because presumably Jesse did end up fixing whatever he did actually blow up. -Chris
On 18 Nov 2014 19:15:18 -0800 shuang.he@intel.com wrote: > Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) > -------------------------------------Summary------------------------------------- > Platform: baseline_drm_intel_nightly_pass_rate->patch_applied_pass_rate > BYT: pass/total=290/290->290/290 > PNV: pass/total=362/362->362/362 > ILK: pass/total=381/381->379/381 > IVB: pass/total=522/559->522/559 > SNB: pass/total=444/444->444/444 > HSW: pass/total=595/595->595/595 > BDW: pass/total=436/436->436/436 > -------------------------------------Detailed------------------------------------- > test_platform: test_suite, test_case, result_with_drm_intel_nightly(count, machine_id...)...->result_with_patch_applied(count, machine_id)... > ILK: Intel_gpu_tools, igt_kms_flip_flip-vs-wf_vblank-interruptible, DMESG_WARN(2, M26)PASS(2, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26) > ILK: Intel_gpu_tools, igt_kms_flip_single-buffer-flip-vs-dpms-off-vs-modeset, DMESG_WARN(1, M26)PASS(3, M37M26) -> DMESG_WARN(1, M26)PASS(3, M26) Looks like these two tests are flakey; this patch shouldn't affect those cases. Do we have bugs open for these subtests already? Thanks,
On Wed, 19 Nov 2014 15:00:04 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > > Might be helpful for debugging places where userspace ends up boosting > > or waiting where it doesn't intend to. > > Might be feels a bit weak justification for a new tracepoint imo. Please > drum up more support. I put it together for some media playback debugging we're doing on Chrome, where I suspect we're boosting when we don't want to (possibly due to userspace waits). > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- > > drivers/gpu/drm/i915/i915_trace.h | 15 +++++++++++++++ > > drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > 3 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 86cf428..b03cb07 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4209,8 +4209,10 @@ i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj) > > struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); > > > > BUG_ON(!vma); > > - BUG_ON(vma->pin_count == 0); > > - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); > > + if (WARN(vma->pin_count == 0, "bad pin count\n")) > > + return; > > + if (WARN(!i915_gem_obj_ggtt_bound(obj), "obj not bound\n")) > > + return; > > Separate patch. Can you please split it out with the usual "BUG_ON > considered harmful" commit message? Oops forgot about that hunk. Yeah I was debugging a mode setting failure and ran into this BUG, which made things a little tougher when I only had one machine... But this hunk can be ignored.
On Wed, Nov 19, 2014 at 07:39:17AM -0800, Jesse Barnes wrote: > On Wed, 19 Nov 2014 15:00:04 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > > > Might be helpful for debugging places where userspace ends up boosting > > > or waiting where it doesn't intend to. > > > > Might be feels a bit weak justification for a new tracepoint imo. Please > > drum up more support. > > I put it together for some media playback debugging we're doing on > Chrome, where I suspect we're boosting when we don't want to (possibly > due to userspace waits). Hm, I've discussed this exact topic many moons ago with vpg folks and they've said that the boosting done for media workloads on the idle->busy transition annoys them. Iirc the plan we've hashed out was to add an execbuf flag to prevent the execbuf boosting. We've also discussed the wait boosting but that's imo just a bug in libva ;-) And for debugging pointless waits we already have good tracepoints imo. Unfortunately nothing ever came out of that discussion :( -Daniel
On Wed, 19 Nov 2014 17:19:23 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Nov 19, 2014 at 07:39:17AM -0800, Jesse Barnes wrote: > > On Wed, 19 Nov 2014 15:00:04 +0100 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > > > > Might be helpful for debugging places where userspace ends up boosting > > > > or waiting where it doesn't intend to. > > > > > > Might be feels a bit weak justification for a new tracepoint imo. Please > > > drum up more support. > > > > I put it together for some media playback debugging we're doing on > > Chrome, where I suspect we're boosting when we don't want to (possibly > > due to userspace waits). > > Hm, I've discussed this exact topic many moons ago with vpg folks and > they've said that the boosting done for media workloads on the idle->busy > transition annoys them. Iirc the plan we've hashed out was to add an > execbuf flag to prevent the execbuf boosting. > > We've also discussed the wait boosting but that's imo just a bug in libva > ;-) And for debugging pointless waits we already have good tracepoints > imo. We have tracing for waits, but my expectation was that we may end up growing or adding new boost points elsewhere, so having an explicit trace would make sense anyway. But we've already typed many more lines about this than the patch itself contains, so feel free to drop/ignore. It's easy enough to add on an ad-hoc basis.
On Wed, Nov 19, 2014 at 08:29:50AM -0800, Jesse Barnes wrote: > On Wed, 19 Nov 2014 17:19:23 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Wed, Nov 19, 2014 at 07:39:17AM -0800, Jesse Barnes wrote: > > > On Wed, 19 Nov 2014 15:00:04 +0100 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > On Tue, Nov 18, 2014 at 01:12:29PM -0800, Jesse Barnes wrote: > > > > > Might be helpful for debugging places where userspace ends up boosting > > > > > or waiting where it doesn't intend to. > > > > > > > > Might be feels a bit weak justification for a new tracepoint imo. Please > > > > drum up more support. > > > > > > I put it together for some media playback debugging we're doing on > > > Chrome, where I suspect we're boosting when we don't want to (possibly > > > due to userspace waits). > > > > Hm, I've discussed this exact topic many moons ago with vpg folks and > > they've said that the boosting done for media workloads on the idle->busy > > transition annoys them. Iirc the plan we've hashed out was to add an > > execbuf flag to prevent the execbuf boosting. > > > > We've also discussed the wait boosting but that's imo just a bug in libva > > ;-) And for debugging pointless waits we already have good tracepoints > > imo. > > We have tracing for waits, but my expectation was that we may end up > growing or adding new boost points elsewhere, so having an explicit > trace would make sense anyway. > > But we've already typed many more lines about this than the patch > itself contains, so feel free to drop/ignore. It's easy enough to add > on an ad-hoc basis. Or just use the function tracer. And combine with the already existing rps event in case you want the freq too.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 86cf428..b03cb07 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4209,8 +4209,10 @@ i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj) struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); BUG_ON(!vma); - BUG_ON(vma->pin_count == 0); - BUG_ON(!i915_gem_obj_ggtt_bound(obj)); + if (WARN(vma->pin_count == 0, "bad pin count\n")) + return; + if (WARN(!i915_gem_obj_ggtt_bound(obj), "obj not bound\n")) + return; if (--vma->pin_count == 0) obj->pin_mappable = false; diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 751d4ad..d710fe1 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -691,6 +691,21 @@ TRACE_EVENT(switch_mm, __entry->dev, __entry->ring, __entry->to, __entry->vm) ); +TRACE_EVENT(turbo_boost, + TP_PROTO(u32 freq), + TP_ARGS(freq), + + TP_STRUCT__entry( + __field(u32, freq) + ), + + TP_fast_assign( + __entry->freq = freq; + ), + + TP_printk("turbo boost to %d MHz", __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 7558ba2..2944593 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4483,10 +4483,15 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv) mutex_lock(&dev_priv->rps.hw_lock); if (dev_priv->rps.enabled) { - if (IS_VALLEYVIEW(dev)) + if (IS_VALLEYVIEW(dev)) { valleyview_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); - else + trace_turbo_boost(vlv_gpu_freq(dev_priv, + dev_priv->rps.max_freq_softlimit)); + } else { gen6_set_rps(dev_priv->dev, dev_priv->rps.max_freq_softlimit); + trace_turbo_boost(dev_priv->rps.max_freq_softlimit * 50); + } + dev_priv->rps.last_adj = 0; } mutex_unlock(&dev_priv->rps.hw_lock);
Might be helpful for debugging places where userspace ends up boosting or waiting where it doesn't intend to. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_gem.c | 6 ++++-- drivers/gpu/drm/i915/i915_trace.h | 15 +++++++++++++++ drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- 3 files changed, 26 insertions(+), 4 deletions(-)