Re: [PATCH v3 3/3] drm/i915/gen9: Fix PCODE polling during SAGV disabling

[Lyude Paul <lyude@redhat.com>]

One tiny nitpick below

On Mon, 2016-11-28 at 13:12 +0200, Imre Deak wrote:
> According to the previous patch, it's possible atm that we call
> intel_do_sagv_disable() only once during the 1ms period and time out
> if
> that call fails. As opposed to this the spec says that we need to
> keep
> retrying this request for a 1ms duration, so let's do this similarly
> to
> the CDCLK change notification request.
> 
> Cc: Lyude <cpaul@redhat.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 -
>  drivers/gpu/drm/i915/intel_pm.c | 33 +++++++----------------------
> ----
>  2 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index f542cbc..c7458b7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7434,7 +7434,6 @@ enum {
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
>  #define   GEN9_PCODE_SAGV_CONTROL		0x21
>  #define     GEN9_SAGV_DISABLE			0x0
> -#define     GEN9_SAGV_IS_DISABLED		0x1
>  #define     GEN9_SAGV_ENABLE			0x3
>  #define   GEN9_PCODE_REQUEST_DONE		0x1
>  #define GEN6_PCODE_DATA				_MMIO(0x13812
> 8)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index edd68f3..43b0b40 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2954,24 +2954,10 @@ intel_enable_sagv(struct drm_i915_private
> *dev_priv)
>  	return 0;
>  }
>  
> -static int
> -intel_do_sagv_disable(struct drm_i915_private *dev_priv)
> -{
> -	int ret;
> -	uint32_t temp = GEN9_SAGV_DISABLE;
> -
> -	ret = sandybridge_pcode_read(dev_priv,
> GEN9_PCODE_SAGV_CONTROL,
> -				     &temp);
> -	if (ret)
> -		return ret;
> -	else
> -		return temp & GEN9_SAGV_IS_DISABLED;
> -}
> -
>  int
>  intel_disable_sagv(struct drm_i915_private *dev_priv)
>  {
> -	int ret, result;
> +	int ret;
>  
>  	if (!intel_has_sagv(dev_priv))
>  		return 0;
> @@ -2981,27 +2967,22 @@ intel_disable_sagv(struct drm_i915_private
> *dev_priv)
>  
>  	DRM_DEBUG_KMS("Disabling the SAGV\n");
>  	mutex_lock(&dev_priv->rps.hw_lock);
> -
I'd leave the whitespace here.

Other then that:

	Reviewed-by: Lyude <lyude@redhat.com>
>  	/* bspec says to keep retrying for at least 1 ms */
> -	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
> +	ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				GEN9_SAGV_DISABLE);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> -	if (ret == -ETIMEDOUT) {
> -		DRM_ERROR("Request to disable SAGV timed out\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	/*
>  	 * Some skl systems, pre-release machines in particular,
>  	 * don't actually have an SAGV.
>  	 */
> -	if (IS_SKYLAKE(dev_priv) && result == -ENXIO) {
> +	if (IS_SKYLAKE(dev_priv) && ret == -ENXIO) {
>  		DRM_DEBUG_DRIVER("No SAGV found on system,
> ignoring\n");
>  		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
>  		return 0;
> -	} else if (result < 0) {
> -		DRM_ERROR("Failed to disable the SAGV\n");
> -		return result;
> +	} else if (ret < 0) {
> +		DRM_ERROR("Failed to disable the SAGV (%d)\n", ret);
> +		return ret;
>  	}
>  
>  	dev_priv->sagv_status = I915_SAGV_DISABLED;
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx