[Intel-gfx] [PATCH v25 2/8] drm/i915: Use bw state for per crtc SAGV evaluation
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Wed Apr 22 07:50:22 UTC 2020
On Tue, Apr 21, 2020 at 07:00:38PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 20, 2020 at 02:14:10PM +0300, Stanislav Lisovskiy wrote:
> > Future platforms require per-crtc SAGV evaluation
> > and serializing global state when those are changed
> > from different commits.
> >
> > v2: - Add has_sagv check to intel_crtc_can_enable_sagv
> > so that it sets bit in reject mask.
> > - Use bw_state in intel_pre/post_plane_enable_sagv
> > instead of atomic state
> >
> > v3: - Fixed rebase conflict, now using
> > intel_atomic_crtc_state_for_each_plane_state in
> > order to call it from atomic check
> > v4: - Use fb modifier from plane state
> >
> > v5: - Make intel_has_sagv static again(Ville)
> > - Removed unnecessary NULL assignments(Ville)
> > - Removed unnecessary SAGV debug(Ville)
> > - Call intel_compute_sagv_mask only for modesets(Ville)
> > - Serialize global state only if sagv results change, but
> > not mask itself(Ville)
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at intel.com>
> > Cc: James Ausmus <james.ausmus at intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_bw.h | 6 ++
> > drivers/gpu/drm/i915/intel_pm.c | 113 ++++++++++++++++++------
> > drivers/gpu/drm/i915/intel_pm.h | 3 +-
> > 3 files changed, 93 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > index ac004d6f4276..d6df91058223 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -18,6 +18,12 @@ struct intel_crtc_state;
> > struct intel_bw_state {
> > struct intel_global_state base;
> >
> > + /*
> > + * Contains a bit mask, used to determine, whether correspondent
> > + * pipe allows SAGV or not.
> > + */
> > + u8 pipe_sagv_reject;
> > +
> > unsigned int data_rate[I915_MAX_PIPES];
> > u8 num_active_planes[I915_MAX_PIPES];
> > };
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index b0810d76ad47..d06a1a3713ed 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -43,6 +43,7 @@
> > #include "i915_fixed.h"
> > #include "i915_irq.h"
> > #include "i915_trace.h"
> > +#include "display/intel_bw.h"
> > #include "intel_pm.h"
> > #include "intel_sideband.h"
> > #include "../../../platform/x86/intel_ips.h"
> > @@ -3760,34 +3761,75 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + const struct intel_bw_state *new_bw_state;
> >
> > - if (!intel_can_enable_sagv(state))
> > + /*
> > + * Just return if we can't control SAGV or don't have it.
> > + * This is different from situation when we have SAGV but just can't
> > + * afford it due to DBuf limitation - in case if SAGV is completely
> > + * disabled in a BIOS, we are not even allowed to send a PCode request,
> > + * as it will throw an error. So have to check it here.
> > + */
> > + if (!intel_has_sagv(dev_priv))
> > + return;
> > +
> > + new_bw_state = intel_atomic_get_new_bw_state(state);
> > + if (!new_bw_state)
> > + return;
> > +
> > + if (!intel_can_enable_sagv(new_bw_state))
> > intel_disable_sagv(dev_priv);
> > }
> >
> > void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + const struct intel_bw_state *new_bw_state;
> >
> > - if (intel_can_enable_sagv(state))
> > + /*
> > + * Just return if we can't control SAGV or don't have it.
> > + * This is different from situation when we have SAGV but just can't
> > + * afford it due to DBuf limitation - in case if SAGV is completely
> > + * disabled in a BIOS, we are not even allowed to send a PCode request,
> > + * as it will throw an error. So have to check it here.
> > + */
> > + if (!intel_has_sagv(dev_priv))
> > + return;
> > +
> > + new_bw_state = intel_atomic_get_new_bw_state(state);
> > + if (!new_bw_state)
> > + return;
> > +
> > + if (intel_can_enable_sagv(new_bw_state))
> > intel_enable_sagv(dev_priv);
> > }
> >
> > static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > {
> > - struct drm_device *dev = crtc_state->uapi.crtc->dev;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > struct intel_plane *plane;
> > + const struct intel_plane_state *plane_state;
> > int level, latency;
> >
> > + if (!intel_has_sagv(dev_priv))
> > + return false;
> > +
> > if (!crtc_state->hw.active)
> > return true;
> >
> > + /*
> > + * SKL+ workaround: bspec recommends we disable SAGV when we have
> > + * more then one pipe enabled
> > + */
> > + if (hweight8(state->active_pipes) > 1)
> > + return false;
> > +
> > if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > return false;
> >
> > - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > const struct skl_plane_wm *wm =
> > &crtc_state->wm.skl.optimal.planes[plane->id];
> >
> > @@ -3803,7 +3845,7 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > latency = dev_priv->wm.skl_latency[level];
> >
> > if (skl_needs_memory_bw_wa(dev_priv) &&
> > - plane->base.state->fb->modifier ==
> > + plane_state->uapi.fb->modifier ==
> > I915_FORMAT_MOD_X_TILED)
> > latency += 15;
> >
> > @@ -3819,35 +3861,44 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state
> > return true;
> > }
> >
> > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + return bw_state->pipe_sagv_reject == 0;
> > +}
> > +
> > +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > +{
> > + int ret;
> > struct intel_crtc *crtc;
> > - const struct intel_crtc_state *crtc_state;
> > - enum pipe pipe;
> > + struct intel_crtc_state *new_crtc_state;
> > + struct intel_bw_state *new_bw_state = NULL;
> > + const struct intel_bw_state *old_bw_state = NULL;
> > + int i;
> >
> > - if (!intel_has_sagv(dev_priv))
> > - return false;
> > + for_each_new_intel_crtc_in_state(state, crtc,
> > + new_crtc_state, i) {
> > + new_bw_state = intel_atomic_get_bw_state(state);
> > + if (IS_ERR(new_bw_state))
> > + return PTR_ERR(new_bw_state);
> >
> > - /*
> > - * If there are no active CRTCs, no additional checks need be performed
> > - */
> > - if (hweight8(state->active_pipes) == 0)
> > - return true;
> > + old_bw_state = intel_atomic_get_old_bw_state(state);
> >
> > - /*
> > - * SKL+ workaround: bspec recommends we disable SAGV when we have
> > - * more then one pipe enabled
> > - */
> > - if (hweight8(state->active_pipes) > 1)
> > - return false;
> > + if (intel_crtc_can_enable_sagv(new_crtc_state))
> > + new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > + else
> > + new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > + }
> >
> > - /* Since we're now guaranteed to only have one active CRTC... */
> > - pipe = ffs(state->active_pipes) - 1;
> > - crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > - crtc_state = to_intel_crtc_state(crtc->base.state);
> > + if (!new_bw_state)
> > + return 0;
> >
> > - return intel_crtc_can_enable_sagv(crtc_state);
> > + if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) {
>
> Missing the lock_global_state() for the old_maks != new_mask case.
> Otherwise seems fine.
Yep, however most likely if SAGV state had changed, allowed qgv points will change
and then you'll have to serialize commits anyway.
>
> Now I believe we need to separate state->modeset from sag, as I
> mentioned in the previous review. Please take care of that stuff
> before we continue with the rest of this series.
Ah it's this active_pipes discussion again.. So the problem is that
we set active_pipes only for modeset commits as I understand.
One thing which worries me here, is that duplication of active_pipes
in semantically different states seems redundant imo, i.e the more
states like bw_state, cdclk_state, dbuf_state and etc you are going
to introduce - the more this duplication will happen.
I think what is missing here(I mentioned this also in dbuf review)
is that we need a separate state containing active pipes, which can
be then queried, each time we need to get active_pipes.
In order to separate SAGV from modeset to use active_pipes, we basically
need to populate active pipes in a non-modeset commits, as we can't just
read it from dev_priv right away, we simply need one more global state
object containing this, which we can query just the same way we do for
cdclk_state and bw_state. I.e you just have one crtc lock grabbed and
copy this old global state to new state.
This would eliminate the need in duplicating that field in other states,
also reducing probability of some issues with that duplication.
Not sure, may be there are some arguments, why we have to duplicate it
- at least would like to know those then :D
IMO idea to split global state into a pool of separately maintainable objects
was great - this brings much more flexibility to driver code, so lets
utilize this.
Stan
>
> > + ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > }
> >
> > /*
> > @@ -5860,6 +5911,12 @@ skl_compute_wm(struct intel_atomic_state *state)
> > if (ret)
> > return ret;
> >
> > + if (state->modeset) {
> > + ret = intel_compute_sagv_mask(state);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > /*
> > * skl_compute_ddb() will have adjusted the final watermarks
> > * based on how much ddb is available. Now we can actually
> > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > index 9a6036ab0f90..fd1dc422e6c5 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> >
> > #include "i915_reg.h"
> > +#include "display/intel_bw.h"
> >
> > struct drm_device;
> > struct drm_i915_private;
> > @@ -41,7 +42,7 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> > struct skl_pipe_wm *out);
> > void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> > void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> > -bool intel_can_enable_sagv(struct intel_atomic_state *state);
> > +bool intel_can_enable_sagv(const struct intel_bw_state *bw_state);
> > int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > --
> > 2.24.1.485.gad05a3d8e5
>
> --
> Ville Syrjälä
> Intel
More information about the Intel-gfx
mailing list