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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jan 30, 2018 at 04:29:38PM +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.
> 
> v2:
> - s/snb_pcode_request/sandybridge_pcode_write_timeout/ (Ville)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.4+
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103326
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  6 +++++-
>  drivers/gpu/drm/i915/intel_cdclk.c | 22 +++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_pm.c    |  6 +++---
>  3 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 454d8f937fae..7e83d0d3e632 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 sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv, u32 mbox,
> +				    u32 val, int timeout_us);
> +#define sandybridge_pcode_write(dev_priv, mbox, val)	\
> +	sandybridge_pcode_write_timeout(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..a423b674fcec 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1370,10 +1370,15 @@ 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 = sandybridge_pcode_write_timeout(dev_priv,
> +					      HSW_PCODE_DE_WRITE_FREQ_REQ,
> +					      0x80000000, 2000);
>  	mutex_unlock(&dev_priv->pcu_lock);
>  
>  	if (ret) {
> @@ -1404,8 +1409,15 @@ 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 = sandybridge_pcode_write_timeout(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..a6098932be5a 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 sandybridge_pcode_write_timeout(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