On Wed, 07 Sep 2016, Lyude <cpaul@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> ---
>> 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.
If by "very early" you mean pre-production, we don't care.
BR,
Jani.
>
> 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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
