[RFC][PATCH 0/2] drm/i915: VLV rps timer changes

[RFC][PATCH 0/2] drm/i915: VLV rps timer changes

[ville.syrjala]

Just a few more rps patches for VLV.

I noticed a silly GPU frequency ping-pong effect on an idle GPU due to the
VLV rps timer. So I figured I'd do something about it, and the second patch
tries to make sure that the first patch doesn't cause poor performance when
the GPU becomes busy again.

[RFC][PATCH 1/2] drm/i915: Don't increase the GPU frequency from the delayed VLV rps timer

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's little point in increasing the GPU frequency from the delayed
rps work on VLV. Now when the GPU is idle, the GPU frequency actually
keeps dropping gradually until it hits the minimum, whereas previously
it just ping-ponged constantly between RPe and RPe-1.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96cfb3e..eaf0fa2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3464,7 +3464,8 @@ static void vlv_rps_timer_work(struct work_struct *work)
 	 * min freq available.
 	 */
 	mutex_lock(&dev_priv->rps.hw_lock);
-	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
+	if (dev_priv->rps.cur_delay > dev_priv->rps.rpe_delay)
+		valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
-- 
1.8.1.5

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

Re: [RFC][PATCH 1/2] drm/i915: Don't increase the GPU frequency from the delayed VLV rps timer

[Jesse Barnes]

On Tue, 25 Jun 2013 21:38:10 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There's little point in increasing the GPU frequency from the delayed
> rps work on VLV. Now when the GPU is idle, the GPU frequency actually
> keeps dropping gradually until it hits the minimum, whereas previously
> it just ping-ponged constantly between RPe and RPe-1.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 96cfb3e..eaf0fa2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3464,7 +3464,8 @@ static void vlv_rps_timer_work(struct work_struct *work)
>  	 * min freq available.
>  	 */
>  	mutex_lock(&dev_priv->rps.hw_lock);
> -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> +	if (dev_priv->rps.cur_delay > dev_priv->rps.rpe_delay)
> +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  

Hm that's not what I saw here; I stopped getting the timer interrupts
when the GPU went idle.  But that could be explained by punit fw
differences.

So this change looks ok to me.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
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: [RFC][PATCH 1/2] drm/i915: Don't increase the GPU frequency from the delayed VLV rps timer

[Daniel Vetter]

On Tue, Jun 25, 2013 at 11:53:28AM -0700, Jesse Barnes wrote:
> On Tue, 25 Jun 2013 21:38:10 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > There's little point in increasing the GPU frequency from the delayed
> > rps work on VLV. Now when the GPU is idle, the GPU frequency actually
> > keeps dropping gradually until it hits the minimum, whereas previously
> > it just ping-ponged constantly between RPe and RPe-1.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 96cfb3e..eaf0fa2 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3464,7 +3464,8 @@ static void vlv_rps_timer_work(struct work_struct *work)
> >  	 * min freq available.
> >  	 */
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> > -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > +	if (dev_priv->rps.cur_delay > dev_priv->rps.rpe_delay)
> > +		valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  }
> >  
> 
> Hm that's not what I saw here; I stopped getting the timer interrupts
> when the GPU went idle.  But that could be explained by punit fw
> differences.
> 
> So this change looks ok to me.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch. In case you noodle around in here
some more, I'd also vote for a s/timer_work/delayed_work/ color change:
The execution enviroment of a time is _much_ different than from a work,
so sticking to consistent naming helps in reading code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC][PATCH 2/2] drm/i915: Jump to at least RPe on VLV when increasing the GPU frequency

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the current GPU frquency is below RPe, and we're asked to increase
it, just go directly to RPe. This should provide better performance
faster than letting the frequency trickle up in response to the up
threshold interrupts.

For now just do it for VLV, since that matches quite closely how VLV
used to operate when the rps delayed timer kept things at RPe always.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 62f8b2d..d6bd0d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -699,9 +699,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 
-	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
+	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
 		new_delay = dev_priv->rps.cur_delay + 1;
-	else
+
+		/*
+		 * For better performance, jump directly
+		 * to RPe if we're below it.
+		 */
+		if (IS_VALLEYVIEW(dev_priv->dev) &&
+		    dev_priv->rps.cur_delay < dev_priv->rps.rpe_delay)
+			new_delay = dev_priv->rps.rpe_delay;
+	} else
 		new_delay = dev_priv->rps.cur_delay - 1;
 
 	/* sysfs frequency interfaces may have snuck in while servicing the
-- 
1.8.1.5

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

Re: [RFC][PATCH 2/2] drm/i915: Jump to at least RPe on VLV when increasing the GPU frequency

[Jesse Barnes]

On Tue, 25 Jun 2013 21:38:11 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the current GPU frquency is below RPe, and we're asked to increase
> it, just go directly to RPe. This should provide better performance
> faster than letting the frequency trickle up in response to the up
> threshold interrupts.
> 
> For now just do it for VLV, since that matches quite closely how VLV
> used to operate when the rps delayed timer kept things at RPe always.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 62f8b2d..d6bd0d7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -699,9 +699,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
> -	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
> +	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>  		new_delay = dev_priv->rps.cur_delay + 1;
> -	else
> +
> +		/*
> +		 * For better performance, jump directly
> +		 * to RPe if we're below it.
> +		 */
> +		if (IS_VALLEYVIEW(dev_priv->dev) &&
> +		    dev_priv->rps.cur_delay < dev_priv->rps.rpe_delay)
> +			new_delay = dev_priv->rps.rpe_delay;
> +	} else
>  		new_delay = dev_priv->rps.cur_delay - 1;
>  
>  	/* sysfs frequency interfaces may have snuck in while servicing the

Yeah, seems reasonable.  Going to RP1 on other platforms might be a
good approximation of this.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
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: [RFC][PATCH 2/2] drm/i915: Jump to at least RPe on VLV when increasing the GPU frequency

[Daniel Vetter]

On Tue, Jun 25, 2013 at 11:54:39AM -0700, Jesse Barnes wrote:
> On Tue, 25 Jun 2013 21:38:11 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If the current GPU frquency is below RPe, and we're asked to increase
> > it, just go directly to RPe. This should provide better performance
> > faster than letting the frequency trickle up in response to the up
> > threshold interrupts.
> > 
> > For now just do it for VLV, since that matches quite closely how VLV
> > used to operate when the rps delayed timer kept things at RPe always.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 62f8b2d..d6bd0d7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -699,9 +699,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
> >  
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  
> > -	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
> > +	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> >  		new_delay = dev_priv->rps.cur_delay + 1;
> > -	else
> > +
> > +		/*
> > +		 * For better performance, jump directly
> > +		 * to RPe if we're below it.
> > +		 */
> > +		if (IS_VALLEYVIEW(dev_priv->dev) &&
> > +		    dev_priv->rps.cur_delay < dev_priv->rps.rpe_delay)
> > +			new_delay = dev_priv->rps.rpe_delay;
> > +	} else
> >  		new_delay = dev_priv->rps.cur_delay - 1;
> >  
> >  	/* sysfs frequency interfaces may have snuck in while servicing the
> 
> Yeah, seems reasonable.  Going to RP1 on other platforms might be a
> good approximation of this.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 0/2] drm/i915: VLV rps timer changes

[Daniel Vetter]

On Tue, Jun 25, 2013 at 8:38 PM,  <ville.syrjala@linux.intel.com> wrote:
> Just a few more rps patches for VLV.
>
> I noticed a silly GPU frequency ping-pong effect on an idle GPU due to the
> VLV rps timer. So I figured I'd do something about it, and the second patch
> tries to make sure that the first patch doesn't cause poor performance when
> the GPU becomes busy again.

While you're touching the rps idle work, I think it'd make sense to
have this as part of the idleness tracking in the retire work handler
...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch