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@xxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> /* 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
