diff mbox

drm/i915: Use wait_for() to wait for Punit to change GPU freq on VLV

Message ID 1372257804-13715-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä June 26, 2013, 2:43 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use wait_for() instead of the open coded loop to avoid spreading the
same old timeout related bugs.

This changes the loop to use msleep(1) instead of udelay(10) when the
Punit had not yet completed the frequency change. In practice that
doesn't seem to hurt performance as the Punit appears to be ready pretty
much always.

Also give the status bit a name, instead of using the magic number 1.

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

Comments

Jesse Barnes June 26, 2013, 4:26 p.m. UTC | #1 
On Wed, 26 Jun 2013 17:43:24 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use wait_for() instead of the open coded loop to avoid spreading the
> same old timeout related bugs.
> 
> This changes the loop to use msleep(1) instead of udelay(10) when the
> Punit had not yet completed the frequency change. In practice that
> doesn't seem to hurt performance as the Punit appears to be ready pretty
> much always.
> 
> Also give the status bit a name, instead of using the magic number 1.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 10ac3d5..d5199a3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -363,6 +363,7 @@
>  #define PUNIT_REG_GPU_LFM			0xd3
>  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
>  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> +#define   GENFREQSTATUS				(1<<0)
>  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
>  
>  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa48fc6..bff5709 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>   */
>  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
>  {
> -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
>  	u32 pval;
>  
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> -	do {
> -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> -		if (time_after(jiffies, timeout)) {
> -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> -			break;
> -		}
> -		udelay(10);
> -	} while (pval & 1);
> +	if (wait_for(((pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) & GENFREQSTATUS) == 0, 10))
> +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
>  
>  	pval >>= 8;
>  

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Daniel Vetter July 2, 2013, 1:41 p.m. UTC | #2 
On Wed, Jun 26, 2013 at 09:26:25AM -0700, Jesse Barnes wrote:
> On Wed, 26 Jun 2013 17:43:24 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use wait_for() instead of the open coded loop to avoid spreading the
> > same old timeout related bugs.
> > 
> > This changes the loop to use msleep(1) instead of udelay(10) when the
> > Punit had not yet completed the frequency change. In practice that
> > doesn't seem to hurt performance as the Punit appears to be ready pretty
> > much always.
> > 
> > Also give the status bit a name, instead of using the magic number 1.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
> >  2 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 10ac3d5..d5199a3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -363,6 +363,7 @@
> >  #define PUNIT_REG_GPU_LFM			0xd3
> >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > +#define   GENFREQSTATUS				(1<<0)
> >  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
> >  
> >  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index aa48fc6..bff5709 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >   */
> >  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> >  {
> > -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> >  	u32 pval;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > -	do {
> > -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > -		if (time_after(jiffies, timeout)) {
> > -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> > -			break;
> > -		}
> > -		udelay(10);
> > -	} while (pval & 1);
> > +	if (wait_for(((pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) & GENFREQSTATUS) == 0, 10))
> > +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> >  
> >  	pval >>= 8;
> >  
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch.
-Daniel
Daniel Vetter July 2, 2013, 2:01 p.m. UTC | #3 
On Wed, Jun 26, 2013 at 09:26:25AM -0700, Jesse Barnes wrote:
> On Wed, 26 Jun 2013 17:43:24 +0300
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use wait_for() instead of the open coded loop to avoid spreading the
> > same old timeout related bugs.
> > 
> > This changes the loop to use msleep(1) instead of udelay(10) when the
> > Punit had not yet completed the frequency change. In practice that
> > doesn't seem to hurt performance as the Punit appears to be ready pretty
> > much always.
> > 
> > Also give the status bit a name, instead of using the magic number 1.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 11 ++---------
> >  2 files changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 10ac3d5..d5199a3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -363,6 +363,7 @@
> >  #define PUNIT_REG_GPU_LFM			0xd3
> >  #define PUNIT_REG_GPU_FREQ_REQ			0xd4
> >  #define PUNIT_REG_GPU_FREQ_STS			0xd8
> > +#define   GENFREQSTATUS				(1<<0)
> >  #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
> >  
> >  #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index aa48fc6..bff5709 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3074,19 +3074,12 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> >   */
> >  static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
> >  {
> > -	unsigned long timeout = jiffies + msecs_to_jiffies(10);
> >  	u32 pval;
> >  
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > -	do {
> > -		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
> > -		if (time_after(jiffies, timeout)) {
> > -			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> > -			break;
> > -		}
> > -		udelay(10);
> > -	} while (pval & 1);
> > +	if (wait_for(((pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) & GENFREQSTATUS) == 0, 10))
> > +		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> >  
> >  	pval >>= 8;
> >  
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Oops, missed that one here. Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 10ac3d5..d5199a3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -363,6 +363,7 @@ 
 #define PUNIT_REG_GPU_LFM			0xd3
 #define PUNIT_REG_GPU_FREQ_REQ			0xd4
 #define PUNIT_REG_GPU_FREQ_STS			0xd8
+#define   GENFREQSTATUS				(1<<0)
 #define PUNIT_REG_MEDIA_TURBO_FREQ_REQ		0xdc
 
 #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa48fc6..bff5709 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3074,19 +3074,12 @@  void gen6_set_rps(struct drm_device *dev, u8 val)
  */
 static void vlv_update_rps_cur_delay(struct drm_i915_private *dev_priv)
 {
-	unsigned long timeout = jiffies + msecs_to_jiffies(10);
 	u32 pval;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
-	do {
-		pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS);
-		if (time_after(jiffies, timeout)) {
-			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
-			break;
-		}
-		udelay(10);
-	} while (pval & 1);
+	if (wait_for(((pval = vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS)) & GENFREQSTATUS) == 0, 10))
+		DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
 
 	pval >>= 8;