[PATCH] drm/i915: add turbo boost trace point

[PATCH] drm/i915: add turbo boost trace point

[Jesse Barnes]

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(-)

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);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[shuang.he]

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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Jesse Barnes]

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,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add turbo boost trace point

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx