Re: [PATCH v7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Matt Roper]

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@xxxxxxxxx>


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