[Intel-gfx] [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Jani Nikula]

On Wed, 07 Sep 2016, Lyude <cpaul at redhat.com> 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 at 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.

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