[Intel-gfx] [PATCH v18 7/8] drm/i915: Restrict qgv points which don't have enough bandwidth.
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Mon Mar 2 13:15:45 UTC 2020
On Thu, 2020-02-27 at 18:20 +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 05:00:43PM +0200, Stanislav Lisovskiy wrote:
> > According to BSpec 53998, we should try to
> > restrict qgv points, which can't provide
> > enough bandwidth for desired display configuration.
> >
> > Currently we are just comparing against all of
> > those and take minimum(worst case).
> >
> > v2: Fixed wrong PCode reply mask, removed hardcoded
> > values.
> >
> > v3: Forbid simultaneous legacy SAGV PCode requests and
> > restricting qgv points. Put the actual restriction
> > to commit function, added serialization(thanks to Ville)
> > to prevent commit being applied out of order in case of
> > nonblocking and/or nomodeset commits.
> >
> > v4:
> > - Minor code refactoring, fixed few typos(thanks to James
> > Ausmus)
> > - Change the naming of qgv point
> > masking/unmasking functions(James Ausmus).
> > - Simplify the masking/unmasking operation itself,
> > as we don't need to mask only single point per request(James
> > Ausmus)
> > - Reject and stick to highest bandwidth point if SAGV
> > can't be enabled(BSpec)
> >
> > v5:
> > - Add new mailbox reply codes, which seems to happen during
> > boot
> > time for TGL and indicate that QGV setting is not yet
> > available.
> >
> > v6:
> > - Increase number of supported QGV points to be in sync with
> > BSpec.
> >
> > v7: - Rebased and resolved conflict to fix build failure.
> > - Fix NUM_QGV_POINTS to 8 and moved that to header file(James
> > Ausmus)
> >
> > v8: - Don't report an error if we can't restrict qgv points, as
> > SAGV
> > can be disabled by BIOS, which is completely legal. So don't
> > make CI panic. Instead if we detect that there is only 1 QGV
> > point accessible just analyze if we can fit the required
> > bandwidth
> > requirements, but no need in restricting.
> >
> > v9: - Fix wrong QGV transition if we have 0 planes and no SAGV
> > simultaneously.
> >
> > v10: - Fix CDCLK corruption, because of global state getting
> > serialized
> > without modeset, which caused copying of non-calculated
> > cdclk
> > to be copied to dev_priv(thanks to Ville for the hint).
> >
> > v11: - Remove unneeded headers and spaces(Matthew Roper)
> > - Remove unneeded intel_qgv_info qi struct from bw check and
> > zero
> > out the needed one(Matthew Roper)
> > - Changed QGV error message to have more clear meaning(Matthew
> > Roper)
> > - Use state->modeset_set instead of any_ms(Matthew Roper)
> > - Moved NUM_SAGV_POINTS from i915_reg.h to i915_drv.h where
> > it's used
> > - Keep using crtc_state->hw.active instead of .enable(Matthew
> > Roper)
> > - Moved unrelated changes to other patch(using latency as
> > parameter
> > for plane wm calculation, moved to SAGV refactoring patch)
> >
> > v12: - Fix rebase conflict with own temporary SAGV/QGV fix.
> > - Remove unnecessary mask being zero check when unmasking
> > qgv points as this is completely legal(Matt Roper)
> > - Check if we are setting the same mask as already being set
> > in hardware to prevent error from PCode.
> > - Fix error message when restricting/unrestricting qgv points
> > to "mask/unmask" which sounds more accurate(Matt Roper)
> > - Move sagv status setting to icl_get_bw_info from atomic
> > check
> > as this should be calculated only once.(Matt Roper)
> > - Edited comments for the case when we can't enable SAGV and
> > use only 1 QGV point with highest bandwidth to be more
> > understandable.(Matt Roper)
> >
> > v13: - Moved max_data_rate in bw check to closer scope(Ville
> > Syrjälä)
> > - Changed comment for zero new_mask in qgv points masking
> > function
> > to better reflect reality(Ville Syrjälä)
> > - Simplified bit mask operation in qgv points masking function
> > (Ville Syrjälä)
> > - Moved intel_qgv_points_mask closer to gen11 SAGV disabling,
> > however this still can't be under modeset condition(Ville
> > Syrjälä)
> > - Packed qgv_points_mask as u8 and moved closer to
> > pipe_sagv_mask
> > (Ville Syrjälä)
> > - Extracted PCode changes to separate patch.(Ville Syrjälä)
> > - Now treat num_planes 0 same as 1 to avoid confusion and
> > returning max_bw as 0, which would prevent choosing QGV
> > point having max bandwidth in case if SAGV is not allowed,
> > as per BSpec(Ville Syrjälä)
> > - Do the actual qgv_points_mask swap in the same place as
> > all other global state parts like cdclk are swapped.
> > In the next patch, this all will be moved to bw state as
> > global state, once new global state patch series from Ville
> > lands
> >
> > v14: - Now using global state to serialize access to qgv points
> > - Added global state locking back, otherwise we seem to read
> > bw state in a wrong way.
> >
> > v15: - Added TODO comment for near atomic global state locking in
> > bw code.
> >
> > v16: - Fixed intel_atomic_bw_* functions to be intel_bw_* as
> > discussed
> > with Jani Nikula.
> > - Take bw_state_changed flag into use.
> >
> > 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.c | 184 +++++++++++++
> > ------
> > drivers/gpu/drm/i915/display/intel_bw.h | 9 +
> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++++++++++++-
> > drivers/gpu/drm/i915/i915_drv.h | 3 +
> > 4 files changed, 255 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c
> > b/drivers/gpu/drm/i915/display/intel_bw.c
> > index d5be603b8b03..4986a5464700 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -8,6 +8,9 @@
> > #include "intel_bw.h"
> > #include "intel_display_types.h"
> > #include "intel_sideband.h"
> > +#include "intel_atomic.h"
> > +#include "intel_pm.h"
> > +
> >
> > /* Parameters for Qclk Geyserville (QGV) */
> > struct intel_qgv_point {
> > @@ -113,6 +116,26 @@ static int
> > icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
> > return 0;
> > }
> >
> > +int icl_pcode_restrict_qgv_points(struct drm_i915_private
> > *dev_priv,
> > + u32 points_mask)
> > +{
> > + int ret;
> > +
> > + /* bspec says to keep retrying for at least 1 ms */
> > + ret = skl_pcode_request(dev_priv,
> > ICL_PCODE_SAGV_DE_MEM_SS_CONFIG,
> > + points_mask,
> > + GEN11_PCODE_POINTS_RESTRICTED_MASK,
> > + GEN11_PCODE_POINTS_RESTRICTED,
> > + 1);
> > +
> > + if (ret < 0) {
> > + DRM_ERROR("Failed to disable qgv points (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> > struct intel_qgv_info *qi)
> > {
> > @@ -240,6 +263,16 @@ static int icl_get_bw_info(struct
> > drm_i915_private *dev_priv, const struct intel
> > break;
> > }
> >
> > + /*
> > + * In case if SAGV is disabled in BIOS, we always get 1
> > + * SAGV point, but we can't send PCode commands to restrict it
> > + * as it will fail and pointless anyway.
> > + */
> > + if (qi.num_points == 1)
> > + dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
> > + else
> > + dev_priv->sagv_status = I915_SAGV_ENABLED;
> > +
> > return 0;
> > }
> >
> > @@ -259,7 +292,7 @@ static unsigned int icl_max_bw(struct
> > drm_i915_private *dev_priv,
> > if (qgv_point >= bi->num_qgv_points)
> > return UINT_MAX;
> >
> > - if (num_planes >= bi->num_planes)
> > + if (num_planes >= bi->num_planes || !num_planes)
> > return bi->deratedbw[qgv_point];
> > }
> >
> > @@ -277,34 +310,6 @@ void intel_bw_init_hw(struct drm_i915_private
> > *dev_priv)
> > icl_get_bw_info(dev_priv, &icl_sa_info);
> > }
> >
> > -static unsigned int intel_max_data_rate(struct drm_i915_private
> > *dev_priv,
> > - int num_planes)
> > -{
> > - if (INTEL_GEN(dev_priv) >= 11) {
> > - /*
> > - * Any bw group has same amount of QGV points
> > - */
> > - const struct intel_bw_info *bi =
> > - &dev_priv->max_bw[0];
> > - unsigned int min_bw = UINT_MAX;
> > - int i;
> > -
> > - /*
> > - * FIXME with SAGV disabled maybe we can assume
> > - * point 1 will always be used? Seems to match
> > - * the behaviour observed in the wild.
> > - */
> > - for (i = 0; i < bi->num_qgv_points; i++) {
> > - unsigned int bw = icl_max_bw(dev_priv,
> > num_planes, i);
> > -
> > - min_bw = min(bw, min_bw);
> > - }
> > - return min_bw;
> > - } else {
> > - return UINT_MAX;
> > - }
> > -}
> > -
> > static unsigned int intel_bw_crtc_num_active_planes(const struct
> > intel_crtc_state *crtc_state)
> > {
> > /*
> > @@ -418,11 +423,16 @@ int intel_bw_atomic_check(struct
> > intel_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > - struct intel_bw_state *bw_state = NULL;
> > - unsigned int data_rate, max_data_rate;
> > + struct intel_bw_state *new_bw_state = NULL;
> > + struct intel_bw_state *old_bw_state = NULL;
> > + unsigned int data_rate;
> > unsigned int num_active_planes;
> > struct intel_crtc *crtc;
> > int i, ret;
> > + u32 allowed_points = 0;
> > + unsigned int max_bw_point = 0, max_bw = 0;
> > + unsigned int num_qgv_points = dev_priv-
> > >max_bw[0].num_qgv_points;
> > + u32 mask = (1 << num_qgv_points) - 1;
> >
> > /*
> > * For earlier Gens let's consider bandwidth changed if ddb
> > requirements,
> > @@ -430,11 +440,11 @@ int intel_bw_atomic_check(struct
> > intel_atomic_state *state)
> > */
> > if (INTEL_GEN(dev_priv) < 11) {
> > if (state->ddb_state_changed) {
> > - bw_state = intel_bw_get_state(state);
> > - if (IS_ERR(bw_state))
> > - return PTR_ERR(bw_state);
> > + new_bw_state = intel_bw_get_state(state);
> > + if (IS_ERR(new_bw_state))
> > + return PTR_ERR(new_bw_state);
>
> Lot's of rename noise in the patch. Can't really see what's going on
> unless those are split out.
>
> >
> > - ret = intel_atomic_lock_global_state(&bw_state-
> > >base);
> > + ret =
> > intel_atomic_lock_global_state(&new_bw_state->base);
> > if (ret)
> > return ret;
> >
> > @@ -465,45 +475,107 @@ int intel_bw_atomic_check(struct
> > intel_atomic_state *state)
> > old_active_planes == new_active_planes)
> > continue;
> >
> > - bw_state = intel_bw_get_state(state);
> > - if (IS_ERR(bw_state))
> > - return PTR_ERR(bw_state);
> > + new_bw_state = intel_bw_get_state(state);
> > + if (IS_ERR(new_bw_state))
> > + return PTR_ERR(new_bw_state);
> >
> > - bw_state->data_rate[crtc->pipe] = new_data_rate;
> > - bw_state->num_active_planes[crtc->pipe] =
> > new_active_planes;
> > + new_bw_state->data_rate[crtc->pipe] = new_data_rate;
> > + new_bw_state->num_active_planes[crtc->pipe] =
> > new_active_planes;
> >
> > drm_dbg_kms(&dev_priv->drm,
> > "pipe %c data rate %u num active planes
> > %u\n",
> > pipe_name(crtc->pipe),
> > - bw_state->data_rate[crtc->pipe],
> > - bw_state->num_active_planes[crtc->pipe]);
> > + new_bw_state->data_rate[crtc->pipe],
> > + new_bw_state->num_active_planes[crtc-
> > >pipe]);
> > }
> >
> > - if (!bw_state)
> > + if (!new_bw_state)
> > return 0;
> >
> > - ret = intel_atomic_lock_global_state(&bw_state->base);
> > - if (ret)
> > + ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > + if (ret) {
> > + DRM_DEBUG_KMS("Could not lock global state\n");
> > return ret;
> > + }
> >
> > - DRM_DEBUG_KMS("Marking bw state changed for atomic state %p\n",
> > state);
> > -
> > - state->bw_state_changed = true;
> > + data_rate = intel_bw_data_rate(dev_priv, new_bw_state);
> > + data_rate = DIV_ROUND_UP(data_rate, 1000);
> >
> > - data_rate = intel_bw_data_rate(dev_priv, bw_state);
> > - num_active_planes = intel_bw_num_active_planes(dev_priv,
> > bw_state);
> > + num_active_planes = intel_bw_num_active_planes(dev_priv,
> > new_bw_state);
> >
> > - max_data_rate = intel_max_data_rate(dev_priv,
> > num_active_planes);
> > + for (i = 0; i < num_qgv_points; i++) {
> > + unsigned int max_data_rate;
> >
> > - data_rate = DIV_ROUND_UP(data_rate, 1000);
> > + max_data_rate = icl_max_bw(dev_priv, num_active_planes,
> > i);
> > + /*
> > + * We need to know which qgv point gives us
> > + * maximum bandwidth in order to disable SAGV
> > + * if we find that we exceed SAGV block time
> > + * with watermarks. By that moment we already
> > + * have those, as it is calculated earlier in
> > + * intel_atomic_check,
> > + */
> > + if (max_data_rate > max_bw) {
> > + max_bw_point = i;
> > + max_bw = max_data_rate;
> > + }
> > + if (max_data_rate >= data_rate)
> > + allowed_points |= BIT(i);
> > + DRM_DEBUG_KMS("QGV point %d: max bw %d required %d\n",
> > + i, max_data_rate, data_rate);
> > + }
> >
> > - if (data_rate > max_data_rate) {
> > - drm_dbg_kms(&dev_priv->drm,
> > - "Bandwidth %u MB/s exceeds max available %d
> > MB/s (%d active planes)\n",
> > - data_rate, max_data_rate,
> > num_active_planes);
> > + /*
> > + * BSpec states that we always should have at least one allowed
> > point
> > + * left, so if we couldn't - simply reject the configuration
> > for obvious
> > + * reasons.
> > + */
> > + if (allowed_points == 0) {
> > + DRM_DEBUG_KMS("No QGV points provide sufficient memory"
> > + " bandwidth for display
> > configuration.\n");
> > return -EINVAL;
> > }
> >
> > + /*
> > + * Leave only single point with highest bandwidth, if
> > + * we can't enable SAGV due to the increased memory latency it
> > may
> > + * cause.
> > + */
> > + if (!intel_can_enable_sagv_for_state(state)) {
> > + allowed_points = 1 << max_bw_point;
> > + DRM_DEBUG_KMS("No SAGV, using single QGV point %d\n",
> > + max_bw_point);
> > + }
> > + /*
> > + * We store the ones which need to be masked as that is what
> > PCode
> > + * actually accepts as a parameter.
> > + */
> > + new_bw_state->qgv_points_mask = (~allowed_points) & mask;
> > +
> > + DRM_DEBUG_KMS("New state %p qgv mask %x\n",
> > + state, new_bw_state->qgv_points_mask);
> > +
> > + old_bw_state = intel_bw_get_old_state(state);
> > + if (IS_ERR(old_bw_state)) {
> > + DRM_DEBUG_KMS("Could not get old bw state!\n");
> > + return PTR_ERR(old_bw_state);
> > + }
> > +
> > + /*
> > + * If the actual mask had changed we need to make sure that
> > + * the commits are serialized(in case this is a nomodeset,
> > nonblocking)
> > + */
> > + if (new_bw_state->qgv_points_mask != old_bw_state-
> > >qgv_points_mask) {
> > + ret =
> > intel_atomic_serialize_global_state(&new_bw_state->base);
> > + if (ret) {
> > + DRM_DEBUG_KMS("Could not serialize global
> > state\n");
> > + return ret;
> > + }
> > +
> > + DRM_DEBUG_KMS("Marking bw state changed for atomic
> > state %p\n", state);
> > + state->bw_state_changed = true;
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h
> > b/drivers/gpu/drm/i915/display/intel_bw.h
> > index c32b5285c12f..b3522389a181 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -36,6 +36,13 @@ struct intel_bw_state {
> > */
> > bool can_sagv;
> >
> > + /*
> > + * Current QGV points mask, which restricts
> > + * some particular SAGV states, not to confuse
> > + * with pipe_sagv_mask.
> > + */
> > + u8 qgv_points_mask;
> > +
> > unsigned int data_rate[I915_MAX_PIPES];
> > u8 num_active_planes[I915_MAX_PIPES];
> > };
> > @@ -56,5 +63,7 @@ int intel_bw_init(struct drm_i915_private
> > *dev_priv);
> > int intel_bw_atomic_check(struct intel_atomic_state *state);
> > void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> > const struct intel_crtc_state *crtc_state);
> > +int icl_pcode_restrict_qgv_points(struct drm_i915_private
> > *dev_priv,
> > + u32 points_mask);
> >
> > #endif /* __INTEL_BW_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6df836cbe0cd..cb1d10af88ce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15504,6 +15504,105 @@ static void
> > intel_atomic_cleanup_work(struct work_struct *work)
> > intel_atomic_helper_free_state(i915);
> > }
> >
> > +static void intel_qgv_points_mask(struct intel_atomic_state
> > *state)
>
> I think we should move these next to the other sagv code and just
> call them somehting like intel_sagv_{pre,post}_plane_update() to
> a) avoid the caller having to know anything about them, b) match
> the pattern used for other similar things.
>
> And I think the internals should just be something like:
> intel_sagv_pre/post_plane_update()
> {
> new_bw_state = get_new_bw_state();
> old_bw_state = get_old_bw_state();
>
> if (!new_bw_state ||
> new_bw_state->sagv_mask == old_bw_state->sagv_mask)
> return;
>
> do the pcode dance
> }
>
> I don't see a need for any more checks than than because that
> would just mean that bw_check() is broken and needs to be fixed.
Those are actually qgv points mask, also you _have to_ do some
other checks - for example when we are masking qgv points - it means
we can only mask those however for example transition 0x001 (qgv point
0 masked) to 0x000(new mask), means that there is nothing to mask here
and moreover it will be wrong to call PCode at this point.
Stan
>
>
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + int ret;
> > + struct intel_bw_state *new_bw_state = NULL;
> > + struct intel_bw_state *old_bw_state = NULL;
> > + u32 new_mask = 0;
> > + unsigned int num_qgv_points = dev_priv-
> > >max_bw[0].num_qgv_points;
> > + unsigned int mask = (1 << num_qgv_points) - 1;
> > +
> > + new_bw_state = intel_bw_get_state(state);
> > + if (IS_ERR(new_bw_state)) {
> > + WARN(1, "Could not get new bw_state!\n");
> > + return;
> > + }
> > +
> > + old_bw_state = intel_bw_get_old_state(state);
> > + if (IS_ERR(old_bw_state)) {
> > + WARN(1, "Could not get old bw_state!\n");
> > + return;
> > + }
> > +
> > + new_mask = old_bw_state->qgv_points_mask | new_bw_state-
> > >qgv_points_mask;
> > +
> > + /*
> > + * If new mask is zero - means there is nothing to mask,
> > + * we can only unmask, which should be done in unmask.
> > + */
> > + if (!new_mask)
> > + return;
> > +
> > + WARN_ON(new_mask == mask);
> > +
> > + /*
> > + * Just return if we can't control SAGV or don't have it.
> > + */
> > + if (!intel_has_sagv(dev_priv))
> > + return;
> > +
> > + /*
> > + * Restrict required qgv points before updating the
> > configuration.
> > + * According to BSpec we can't mask and unmask qgv points at
> > the same
> > + * time. Also masking should be done before updating the
> > configuration
> > + * and unmasking afterwards.
> > + */
> > + ret = icl_pcode_restrict_qgv_points(dev_priv, new_mask);
> > + if (ret < 0)
> > + DRM_DEBUG_KMS("Could not mask required qgv
> > points(%d)\n",
> > + ret);
> > +}
> > +
> > +static void intel_qgv_points_unmask(struct intel_atomic_state
> > *state)
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > + int ret;
> > + struct intel_bw_state *new_bw_state = NULL;
> > + struct intel_bw_state *old_bw_state = NULL;
> > + u32 new_mask = 0;
> > +
> > + new_bw_state = intel_bw_get_state(state);
> > + if (IS_ERR(new_bw_state)) {
> > + WARN(1, "Could not get new bw_state!\n");
> > + return;
> > + }
> > +
> > + old_bw_state = intel_bw_get_old_state(state);
> > + if (IS_ERR(old_bw_state)) {
> > + WARN(1, "Could not get new bw_state!\n");
> > + return;
> > + }
> > +
> > + new_mask = new_bw_state->qgv_points_mask;
> > +
> > + /*
> > + * Just return if we can't control SAGV or don't have it.
> > + */
> > + if (!intel_has_sagv(dev_priv))
> > + return;
> > +
> > + /*
> > + * Nothing to unmask
> > + */
> > + if (new_mask == old_bw_state->qgv_points_mask)
> > + return;
> > +
> > + /*
> > + * Allow required qgv points after updating the configuration.
> > + * According to BSpec we can't mask and unmask qgv points at
> > the same
> > + * time. Also masking should be done before updating the
> > configuration
> > + * and unmasking afterwards.
> > + */
> > + ret = icl_pcode_restrict_qgv_points(dev_priv, new_mask);
> > + if (ret < 0)
> > + DRM_DEBUG_KMS("Could not unmask required qgv
> > points(%d)\n",
> > + ret);
> > +}
> > +
> > static void intel_atomic_commit_tail(struct intel_atomic_state
> > *state)
> > {
> > struct drm_device *dev = state->base.dev;
> > @@ -15537,6 +15636,15 @@ static void
> > intel_atomic_commit_tail(struct intel_atomic_state *state)
> > for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i)
> > crtc->config = new_crtc_state;
> >
> > + /*
> > + * Now we need to check if SAGV needs to be disabled(i.e QGV
> > points
> > + * modified even, when no modeset is done(for example plane
> > updates
> > + * can now trigger that).
> > + */
> > + if ((INTEL_GEN(dev_priv) >= 11))
> > + if (state->bw_state_changed)
> > + intel_qgv_points_mask(state);
> > +
> > if (state->modeset) {
> > drm_atomic_helper_update_legacy_modeset_state(dev,
> > &state->base);
> >
> > @@ -15546,12 +15654,9 @@ static void
> > intel_atomic_commit_tail(struct intel_atomic_state *state)
> > * SKL workaround: bspec recommends we disable the SAGV
> > when we
> > * have more then one pipe enabled
> > */
> > - if (INTEL_GEN(dev_priv) < 11) {
> > - if (state->bw_state_changed) {
> > - if (!intel_can_enable_sagv(dev_priv))
> > - intel_disable_sagv(dev_priv);
> > - }
> > - }
> > + if ((INTEL_GEN(dev_priv) < 11))
> > + if (!intel_can_enable_sagv(dev_priv))
> > + intel_disable_sagv(dev_priv);
> >
> > intel_modeset_verify_disabled(dev_priv, state);
> > }
> > @@ -15652,10 +15757,10 @@ static void
> > intel_atomic_commit_tail(struct intel_atomic_state *state)
> > intel_verify_planes(state);
> >
> > if (INTEL_GEN(dev_priv) < 11) {
> > - if (state->bw_state_changed) {
> > - if (intel_can_enable_sagv(dev_priv))
> > - intel_enable_sagv(dev_priv);
> > - }
> > + if (intel_can_enable_sagv(dev_priv))
> > + intel_enable_sagv(dev_priv);
> > + } else if (state->bw_state_changed) {
> > + intel_qgv_points_unmask(state);
> > }
> >
> > drm_atomic_helper_commit_hw_done(&state->base);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 4305ccc4c683..0a589700a071 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -841,6 +841,9 @@ enum intel_pipe_crc_source {
> > INTEL_PIPE_CRC_SOURCE_MAX,
> > };
> >
> > +/* BSpec precisely defines this */
> > +#define NUM_SAGV_POINTS 8
> > +
> > #define INTEL_PIPE_CRC_ENTRIES_NR 128
> > struct intel_pipe_crc {
> > spinlock_t lock;
> > --
> > 2.24.1.485.gad05a3d8e5
>
>
More information about the Intel-gfx
mailing list