[PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing

[PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing

[Imre Deak]

Currently we see sporadic timeouts during CDCLK changing both on BXT and
GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
changing the frequency in a tight loop after blanking the display. The
upper bound for the completion time is 800us based on my tests, so
increase it from the current 500us to 2ms; with that I couldn't trigger
the problem either on BXT or GLK.

Note that timeouts happened during both the change notification and the
voltage level setting PCODE request. (For the latter one BSpec doesn't
require us to wait for completion before further HW programming.)

This issue is similar to
2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
notification")
but there the PCODE request does complete (as shown by the mbox
busy flag), only the reply we get from PCODE indicates a failure.
So there we keep resending the request until a success reply, here we
just have to increase the timeout for the one PCODE request we send.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org # v4.4+
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  6 +++++-
 drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
 drivers/gpu/drm/i915/intel_pm.c    |  6 +++---
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 454d8f937fae..5e293be4e51d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
 					    struct intel_display_error_state *error);
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
+int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
+		      int timeout_us);
+#define sandybridge_pcode_write(dev_priv, mbox, val)	\
+	snb_pcode_request(dev_priv, mbox, val, 500)
+
 int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
 		      u32 reply_mask, u32 reply, int timeout_base_ms);
 
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index c4392ea34a3d..5057336c40ba 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 		break;
 	}
 
-	/* Inform power controller of upcoming frequency change */
+	/*
+	 * Inform power controller of upcoming frequency change. BSpec
+	 * requires us to wait up to 150usec, but that leads to timeouts;
+	 * the 2ms used here is based on experiment.
+	 */
 	mutex_lock(&dev_priv->pcu_lock);
-	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
-				      0x80000000);
+	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
+				0x80000000, 2000);
 	mutex_unlock(&dev_priv->pcu_lock);
 
 	if (ret) {
@@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 	I915_WRITE(CDCLK_CTL, val);
 
 	mutex_lock(&dev_priv->pcu_lock);
-	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
-				      cdclk_state->voltage_level);
+	/*
+	 * The timeout isn't specified, the 2ms used here is based on
+	 * experiment.
+	 * FIXME: Waiting for the request completion could be delayed until
+	 * the next PCODE request based on BSpec.
+	 */
+	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
+				cdclk_state->voltage_level, 2000);
 	mutex_unlock(&dev_priv->pcu_lock);
 
 	if (ret) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0b92ea1dbd40..f6f4dbacb9af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
 	return 0;
 }
 
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
-			    u32 mbox, u32 val)
+int snb_pcode_request(struct drm_i915_private *dev_priv,
+		      u32 mbox, u32 val, int timeout_us)
 {
 	int status;
 
@@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 
 	if (__intel_wait_for_register_fw(dev_priv,
 					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
-					 500, 0, NULL)) {
+					 timeout_us, 0, NULL)) {
 		DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
 			  val, mbox, __builtin_return_address(0));
 		return -ETIMEDOUT;
-- 
2.13.2

Re: [PATCH 1/3] drm/i915/bxt, glk: Increase PCODE timeouts during CDCLK freq changing

[Chris Wilson]

Quoting Imre Deak (2018-01-30 11:47:10)
> Currently we see sporadic timeouts during CDCLK changing both on BXT and
> GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> changing the frequency in a tight loop after blanking the display. The
> upper bound for the completion time is 800us based on my tests, so
> increase it from the current 500us to 2ms; with that I couldn't trigger
> the problem either on BXT or GLK.
> 
> Note that timeouts happened during both the change notification and the
> voltage level setting PCODE request. (For the latter one BSpec doesn't
> require us to wait for completion before further HW programming.)
> 
> This issue is similar to
> 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> notification")
> but there the PCODE request does complete (as shown by the mbox
> busy flag), only the reply we get from PCODE indicates a failure.
> So there we keep resending the request until a success reply, here we
> just have to increase the timeout for the one PCODE request we send.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing

[Ville Syrjälä]

On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> Currently we see sporadic timeouts during CDCLK changing both on BXT and
> GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> changing the frequency in a tight loop after blanking the display. The
> upper bound for the completion time is 800us based on my tests, so
> increase it from the current 500us to 2ms; with that I couldn't trigger
> the problem either on BXT or GLK.
> 
> Note that timeouts happened during both the change notification and the
> voltage level setting PCODE request. (For the latter one BSpec doesn't
> require us to wait for completion before further HW programming.)
> 
> This issue is similar to
> 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> notification")
> but there the PCODE request does complete (as shown by the mbox
> busy flag), only the reply we get from PCODE indicates a failure.
> So there we keep resending the request until a success reply, here we
> just have to increase the timeout for the one PCODE request we send.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org # v4.4+
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  6 +++++-
>  drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
>  drivers/gpu/drm/i915/intel_pm.c    |  6 +++---
>  3 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 454d8f937fae..5e293be4e51d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
>  					    struct intel_display_error_state *error);
>  
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> +		      int timeout_us);
> +#define sandybridge_pcode_write(dev_priv, mbox, val)	\
> +	snb_pcode_request(dev_priv, mbox, val, 500)

The naming feels a bit inconsistent. snb_pcode_request() is nothing
like skl_pcode_request(), rather it's just an improved
sandybridge_pcode_write().

> +
>  int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
>  		      u32 reply_mask, u32 reply, int timeout_base_ms);
>  
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index c4392ea34a3d..5057336c40ba 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	/* Inform power controller of upcoming frequency change */
> +	/*
> +	 * Inform power controller of upcoming frequency change. BSpec
> +	 * requires us to wait up to 150usec, but that leads to timeouts;
> +	 * the 2ms used here is based on experiment.
> +	 */
>  	mutex_lock(&dev_priv->pcu_lock);
> -	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> -				      0x80000000);
> +	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> +				0x80000000, 2000);
>  	mutex_unlock(&dev_priv->pcu_lock);
>  
>  	if (ret) {
> @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  	I915_WRITE(CDCLK_CTL, val);
>  
>  	mutex_lock(&dev_priv->pcu_lock);
> -	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> -				      cdclk_state->voltage_level);
> +	/*
> +	 * The timeout isn't specified, the 2ms used here is based on
> +	 * experiment.
> +	 * FIXME: Waiting for the request completion could be delayed until
> +	 * the next PCODE request based on BSpec.
> +	 */
> +	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> +				cdclk_state->voltage_level, 2000);
>  	mutex_unlock(&dev_priv->pcu_lock);
>  
>  	if (ret) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0b92ea1dbd40..f6f4dbacb9af 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>  	return 0;
>  }
>  
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> -			    u32 mbox, u32 val)
> +int snb_pcode_request(struct drm_i915_private *dev_priv,
> +		      u32 mbox, u32 val, int timeout_us)
>  {
>  	int status;
>  
> @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  
>  	if (__intel_wait_for_register_fw(dev_priv,
>  					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> -					 500, 0, NULL)) {
> +					 timeout_us, 0, NULL)) {
>  		DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
>  			  val, mbox, __builtin_return_address(0));
>  		return -ETIMEDOUT;
> -- 
> 2.13.2

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH 1/3] drm/i915/bxt,glk: Increase PCODE timeouts during CDCLK freq changing

[Imre Deak]

On Tue, Jan 30, 2018 at 03:42:45PM +0200, Ville Syrj�l� wrote:
> On Tue, Jan 30, 2018 at 01:47:10PM +0200, Imre Deak wrote:
> > Currently we see sporadic timeouts during CDCLK changing both on BXT and
> > GLK as reported by the Bugzilla: ticket. It's easy to reproduce this by
> > changing the frequency in a tight loop after blanking the display. The
> > upper bound for the completion time is 800us based on my tests, so
> > increase it from the current 500us to 2ms; with that I couldn't trigger
> > the problem either on BXT or GLK.
> > 
> > Note that timeouts happened during both the change notification and the
> > voltage level setting PCODE request. (For the latter one BSpec doesn't
> > require us to wait for completion before further HW programming.)
> > 
> > This issue is similar to
> > 2c7d0602c815 ("drm/i915/gen9: Fix PCODE polling during CDCLK change
> > notification")
> > but there the PCODE request does complete (as shown by the mbox
> > busy flag), only the reply we get from PCODE indicates a failure.
> > So there we keep resending the request until a success reply, here we
> > just have to increase the timeout for the one PCODE request we send.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > Cc: stable@vger.kernel.org # v4.4+
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    |  6 +++++-
> >  drivers/gpu/drm/i915/intel_cdclk.c | 20 +++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_pm.c    |  6 +++---
> >  3 files changed, 23 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 454d8f937fae..5e293be4e51d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3723,7 +3723,11 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
> >  					    struct intel_display_error_state *error);
> >  
> >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
> > +int snb_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 val,
> > +		      int timeout_us);
> > +#define sandybridge_pcode_write(dev_priv, mbox, val)	\
> > +	snb_pcode_request(dev_priv, mbox, val, 500)
> 
> The naming feels a bit inconsistent. snb_pcode_request() is nothing
> like skl_pcode_request(), rather it's just an improved
> sandybridge_pcode_write().

The idea was to keep in then end (in drm-tip) only two pcode helpers
snb_pcode_request() and skl_pcode_request(). But yes, they are different
so probably the name should reflect this. I'll use
sandybridge_pcode_write_timeout().

> 
> > +
> >  int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
> >  		      u32 reply_mask, u32 reply, int timeout_base_ms);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index c4392ea34a3d..5057336c40ba 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1370,10 +1370,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >  		break;
> >  	}
> >  
> > -	/* Inform power controller of upcoming frequency change */
> > +	/*
> > +	 * Inform power controller of upcoming frequency change. BSpec
> > +	 * requires us to wait up to 150usec, but that leads to timeouts;
> > +	 * the 2ms used here is based on experiment.
> > +	 */
> >  	mutex_lock(&dev_priv->pcu_lock);
> > -	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > -				      0x80000000);
> > +	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > +				0x80000000, 2000);
> >  	mutex_unlock(&dev_priv->pcu_lock);
> >  
> >  	if (ret) {
> > @@ -1404,8 +1408,14 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >  	I915_WRITE(CDCLK_CTL, val);
> >  
> >  	mutex_lock(&dev_priv->pcu_lock);
> > -	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > -				      cdclk_state->voltage_level);
> > +	/*
> > +	 * The timeout isn't specified, the 2ms used here is based on
> > +	 * experiment.
> > +	 * FIXME: Waiting for the request completion could be delayed until
> > +	 * the next PCODE request based on BSpec.
> > +	 */
> > +	ret = snb_pcode_request(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > +				cdclk_state->voltage_level, 2000);
> >  	mutex_unlock(&dev_priv->pcu_lock);
> >  
> >  	if (ret) {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0b92ea1dbd40..f6f4dbacb9af 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -9169,8 +9169,8 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> >  	return 0;
> >  }
> >  
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> > -			    u32 mbox, u32 val)
> > +int snb_pcode_request(struct drm_i915_private *dev_priv,
> > +		      u32 mbox, u32 val, int timeout_us)
> >  {
> >  	int status;
> >  
> > @@ -9193,7 +9193,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >  
> >  	if (__intel_wait_for_register_fw(dev_priv,
> >  					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > -					 500, 0, NULL)) {
> > +					 timeout_us, 0, NULL)) {
> >  		DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> >  			  val, mbox, __builtin_return_address(0));
> >  		return -ETIMEDOUT;
> > -- 
> > 2.13.2
> 
> -- 
> Ville Syrj�l�
> Intel OTC