Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Lyude <cpaul@redhat.com>]

On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> And use it to move knowledge about the SAGV-supporting platforms from
> the callers to the SAGV code.
> 
> We'll add more platforms to intel_has_sagv(), so IMHO it makes more
> sense to move all this to a single function instead of patching all
> the callers every time we add SAGV support to a new platform.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 ++---
>  drivers/gpu/drm/i915/intel_pm.c      | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4dd4961..2442ab2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14379,7 +14379,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		 * SKL workaround: bspec recommends we disable the
> SAGV when we
>  		 * have more then one pipe enabled
>  		 */
> -		if (IS_SKYLAKE(dev_priv) &&
> !intel_can_enable_sagv(state))
> +		if (!intel_can_enable_sagv(state))
>  			intel_disable_sagv(dev_priv);
>  
>  		intel_modeset_verify_disabled(dev);
> @@ -14437,8 +14437,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state,
> crtc->state);
>  	}
>  
> -	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> -	    intel_can_enable_sagv(state))
> +	if (intel_state->modeset && intel_can_enable_sagv(state))
>  		intel_enable_sagv(dev_priv);
>  
>  	drm_atomic_helper_commit_hw_done(state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 32588e3..af75011 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2884,6 +2884,12 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>  	}
>  }
>  
> +static bool
> +intel_has_sagv(struct drm_i915_private *dev_priv)
> +{
> +	return IS_SKYLAKE(dev_priv);
> +}
> +

Not sure I agree on this one. Even if a system is skylake or kabylake,
there's a couple of very early skylake machines that don't actually
have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value we set
if we get mailbox errors.

So if we're going to split SAGV detection into a different function, I
would also move the dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED
check into there:

if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
	return false;
if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
	return false;

return true;

>  /*
>   * SAGV dynamically adjusts the system agent voltage and clock
> frequencies
>   * depending on power and performance requirements. The display
> engine access
> @@ -2900,6 +2906,9 @@ intel_enable_sagv(struct drm_i915_private
> *dev_priv)
>  {
>  	int ret;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return 0;
> +
>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>  	    dev_priv->sagv_status == I915_SAGV_ENABLED)
>  		return 0;
> @@ -2949,6 +2958,9 @@ intel_disable_sagv(struct drm_i915_private
> *dev_priv)
>  {
>  	int ret, result;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return 0;
> +
>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>  	    dev_priv->sagv_status == I915_SAGV_DISABLED)
>  		return 0;
> @@ -2991,6 +3003,9 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  	enum pipe pipe;
>  	int level, plane;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return false;
> +
>  	/*
>  	 * SKL workaround: bspec recommends we disable the SAGV when
> we have
>  	 * more then one pipe enabled
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx