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.zan...@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
