On Wed, Aug 03, 2016 at 09:50:23AM -0400, Lyude wrote:
...
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> + int ret, result;
> +
> + if (IS_BROXTON(dev_priv))
> + return 0;
> + if (!dev_priv->skl_sagv_enabled)
> + return 0;
> +
> + mutex_lock(&dev_priv->rps.hw_lock);
> + DRM_DEBUG_KMS("Disabling the SAGV\n");
> +
> + /* bspec says to keep retrying for at least 1 ms */
> + ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> + mutex_unlock(&dev_priv->rps.hw_lock);
> +
> + if (ret == -ETIMEDOUT)
> + DRM_ERROR("Request to disable SAGV timed out\n");
> + else {
I think you might have missed my note here on the last patch, but we
need to fix the minor coding style issue here (if either branch of an
if/else needs braces, you need to put them on both branches).
> + if (result == 1)
As I mentioned before, the 1=off looks confusing if someone isn't
looking at the bspec carefully. Using a #define for the various 0x1's
in this patch might help clarify the code slightly.
With those minor changes made,
Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
Matt
> + dev_priv->skl_sagv_enabled = false;
> +
> + ret = result;
> + }
> +
> + return ret;
> +}
> +
> +static void
> skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> const struct intel_crtc_state *cstate,
> struct skl_ddb_entry *alloc, /* out */
> @@ -4236,6 +4346,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> /* Easy/common case; just sanitize DDB now if everything off */
> memset(ddb, 0, sizeof(*ddb));
> }
> +
> + skl_sagv_get_hw_state(dev_priv);
> }
>
> static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> --
> 2.7.4
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
