| Message ID | 1473209539-6689-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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
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. 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
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 >
On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote: > 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: >> > > +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 :( Regardless of whose CI you refer to, no pre-production machines should be used for CI. Which machines are these? Can we be sure all production machines have SAGV? BR, Jani.
Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu: > On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote: > > > > 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: > > > > > > > > > > +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. Ok, so I'd like some clarification regarding this from the maintainers. I always thought we didn't really care, but do this: $ git grep _REVID_ If we don't care, why do we have this? Newer platforms also have this. And many of these REVID checks are only pre-prod. > > > > 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 :( > > Regardless of whose CI you refer to, no pre-production machines > should > be used for CI. Which machines are these? I suppose he's talking about our CI. > > Can we be sure all production machines have SAGV? Our specs don't mention anything regarding this. I'll have to ask for clarification, but I don't think it will be a good idea to remove the code if CI starts complaining. > > BR, > Jani. > >
On Fri, Sep 09, 2016 at 07:51:15PM +0000, Zanoni, Paulo R wrote: > Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu: > > On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote: > > > > > > 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: > > > > > > > > > > > > +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. > > Ok, so I'd like some clarification regarding this from the maintainers. > I always thought we didn't really care, but do this: > > $ git grep _REVID_ > > If we don't care, why do we have this? Newer platforms also have this. > And many of these REVID checks are only pre-prod. For some reason no one has stepped up to remove them.
On Sat, 10 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Sep 09, 2016 at 07:51:15PM +0000, Zanoni, Paulo R wrote: >> Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu: >> > On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote: >> > > >> > > 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: >> > > > > > >> > > > > > +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. >> >> Ok, so I'd like some clarification regarding this from the maintainers. >> I always thought we didn't really care, but do this: >> >> $ git grep _REVID_ >> >> If we don't care, why do we have this? Newer platforms also have this. >> And many of these REVID checks are only pre-prod. > > For some reason no one has stepped up to remove them. All the pre-production revid checks for each platform should be thrown out when production hardware is readily available and in CI. We've had this discussion before [1], but nobody has still stepped up. Perhaps because this would involve checking and double checking what the first shipped revision really was... We're just hurting ourselves catering for a tiny portion of people who still have pre-production hardware laying around. We should just warn and taint the kernel for them. BR, Jani. [1] http://mid.mail-archive.com/87inykiz6f.fsf@intel.com
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); +} + /* * 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
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(-)