On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
> 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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
