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

[Lyude Paul <cpaul@redhat.com>]

On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
> On Wed, 07 Sep 2016, Lyude <cpaul@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@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.

The problem is if we don't handle that case though then a couple of the machines
in CI start failing tests since all of the SAGV mailbox commands don't end up
working :(

> 
> 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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx