| Message ID | 20240220093136.32181-4-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGV/SAGV related fixes | expand |
Hi Stan On Tue, 2024-02-20 at 11:31 +0200, Stanislav Lisovskiy wrote: > Problem is that on some platforms, we do get QGV point mask in wrong > state on boot. However driver assumes it is set to 0 > (i.e all points allowed), however in reality we might get them all restricted, > causing issues. > Lets disable SAGV initially to force proper QGV point state. > If more QGV points are available, driver will recalculate and update > those then after next commit. > > v2: - Added trace to see which QGV/PSF GV point is used when SAGV is > disabled. > v3: - Move force disable function to intel_bw_init in order to initialize > bw state as well, so that hw/sw are immediately in sync after init. > v4: - Don't try sending PCode request, seems like it is not possible at > intel_bw_init, however assigning bw->state to be restricted as if > SAGV is off, still forces driveer to send PCode request anyway on > next modeset, so the solution still works. > However we still need to address the case, when no display is connected, > which anyway requires much more changes. > > v5: - Put PCode request back and apply temporary hack to make the > request succeed(in case if there 2 PSF GV points with same BW, PCode > accepts only if both points are restricted/unrestricted same time) > - Fix argument sequence for adl_qgv_bw(Ville Syrjälä) > > v6: Fix wrong platform checks, not to break everything else. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bw.c | 73 ++++++++++++++++++-- > drivers/gpu/drm/i915/display/skl_watermark.c | 2 +- > drivers/gpu/drm/i915/display/skl_watermark.h | 1 + > 3 files changed, 71 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > index 7baa1c13eccd..f9c301114f02 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -162,7 +162,7 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > 1); > > if (ret < 0) { > - drm_err(&dev_priv->drm, "Failed to disable qgv points (%d) points: 0x%x\n", ret, > points_mask); > + drm_err(&dev_priv->drm, "Failed to disable qgv points (%x) points: 0x%x\n", ret, > points_mask); > return ret; > } > > @@ -662,7 +662,7 @@ static unsigned int adl_psf_bw(struct drm_i915_private *i915, > } > > static unsigned int adl_qgv_bw(struct drm_i915_private *i915, > - int qgv_point, int num_active_planes) > + int num_active_planes, int qgv_point) As mentioned in the previous patch, this change should be handled in the previous patch. > { > unsigned int idx; > > @@ -833,7 +833,7 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915, > for (i = 0; i < num_qgv_points; i++) { > unsigned int max_data_rate; > > - max_data_rate = adl_qgv_bw(i915, i, num_active_planes); > + max_data_rate = adl_qgv_bw(i915, num_active_planes, i); > > /* > * We need to know which qgv point gives us > @@ -852,6 +852,68 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915, > return max_bw_point; > } > > +/* > + * Due to some strange reason, we have to use a mask of PSF GV > + * points, instead of finding the one which provides the highest bandwidth, > + * this is because PCode rejects the request, if 2 PSF GV points, which have > + * same bandwidth are not set/cleared same time. > + */ > +static unsigned int icl_max_bw_psf_gv_point_mask(struct drm_i915_private *i915) > +{ > + unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; > + unsigned int max_bw = 0; > + unsigned int max_bw_point_mask = 0; > + int i; > + > + for (i = 0; i < num_psf_gv_points; i++) { > + unsigned int max_data_rate = adl_psf_bw(i915, i); > + > + if (max_data_rate > max_bw) { > + max_bw_point_mask = BIT(i); > + max_bw = max_data_rate; > + } else if (max_data_rate == max_bw) > + max_bw_point_mask |= BIT(i); > + } > + > + return max_bw_point_mask; > +} > + > +static void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state > *bw_state) > +{ > + unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0); There could be QGV points with similar values. Shouldn't we handle the QGV point case also as mask.. . similar to psf_gv points? > + unsigned int max_bw_psf_gv_point_mask = icl_max_bw_psf_gv_point_mask(i915); > + unsigned int qgv_points; > + unsigned int psf_points; > + int ret; > + > + /* > + * 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(i915)) > + return; > + > + qgv_points = BIT(max_bw_qgv_point); > + psf_points = max_bw_psf_gv_point_mask; > + > + bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points)| > + ADLS_PCODE_REQ_PSF_PT(psf_points)) & > + icl_qgv_points_mask(i915); > + > + drm_dbg_kms(&i915->drm, "Forcing SAGV disable: mask %x\n", bw_state->qgv_points_mask); > + > + ret = icl_pcode_restrict_qgv_points(i915, bw_state->qgv_points_mask); > + > + if (ret) > + drm_dbg_kms(&i915->drm, "Restricting GV points failed: %x\n", ret); > + else > + drm_dbg_kms(&i915->drm, "Restricting GV points succeeded\n"); > + > +} > + > static int mtl_find_qgv_points(struct drm_i915_private *i915, > unsigned int data_rate, > unsigned int num_active_planes, > @@ -943,7 +1005,7 @@ static int icl_find_qgv_points(struct drm_i915_private *i915, > for (i = 0; i < num_qgv_points; i++) { > unsigned int max_data_rate; > > - max_data_rate = adl_qgv_bw(i915, i, num_active_planes); > + max_data_rate = adl_qgv_bw(i915, num_active_planes, i); > > if (max_data_rate >= data_rate) > qgv_points |= BIT(i); > @@ -1351,5 +1413,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv) > intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj, > &state->base, &intel_bw_funcs); > > + if (DISPLAY_VER(dev_priv) >= 11) > + icl_force_disable_sagv(dev_priv, state); > + As MTL handles this differently, we shouldn't call this for display >= 14. BR vinod > return 0; > } > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c > b/drivers/gpu/drm/i915/display/skl_watermark.c > index 56588d6e24ae..706f5afbbab4 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > @@ -63,7 +63,7 @@ static bool skl_needs_memory_bw_wa(struct drm_i915_private *i915) > return DISPLAY_VER(i915) == 9; > } > > -static bool > +bool > intel_has_sagv(struct drm_i915_private *i915) > { > return HAS_SAGV(i915) && > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h > b/drivers/gpu/drm/i915/display/skl_watermark.h > index fb0da36fd3ec..4cca93cd83ad 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark.h > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h > @@ -25,6 +25,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > void intel_sagv_post_plane_update(struct intel_atomic_state *state); > bool intel_can_enable_sagv(struct drm_i915_private *i915, > const struct intel_bw_state *bw_state); > +bool intel_has_sagv(struct drm_i915_private *i915); > > u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *i915, > const struct skl_ddb_entry *entry);
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c index 7baa1c13eccd..f9c301114f02 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.c +++ b/drivers/gpu/drm/i915/display/intel_bw.c @@ -162,7 +162,7 @@ int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, 1); if (ret < 0) { - drm_err(&dev_priv->drm, "Failed to disable qgv points (%d) points: 0x%x\n", ret, points_mask); + drm_err(&dev_priv->drm, "Failed to disable qgv points (%x) points: 0x%x\n", ret, points_mask); return ret; } @@ -662,7 +662,7 @@ static unsigned int adl_psf_bw(struct drm_i915_private *i915, } static unsigned int adl_qgv_bw(struct drm_i915_private *i915, - int qgv_point, int num_active_planes) + int num_active_planes, int qgv_point) { unsigned int idx; @@ -833,7 +833,7 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915, for (i = 0; i < num_qgv_points; i++) { unsigned int max_data_rate; - max_data_rate = adl_qgv_bw(i915, i, num_active_planes); + max_data_rate = adl_qgv_bw(i915, num_active_planes, i); /* * We need to know which qgv point gives us @@ -852,6 +852,68 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915, return max_bw_point; } +/* + * Due to some strange reason, we have to use a mask of PSF GV + * points, instead of finding the one which provides the highest bandwidth, + * this is because PCode rejects the request, if 2 PSF GV points, which have + * same bandwidth are not set/cleared same time. + */ +static unsigned int icl_max_bw_psf_gv_point_mask(struct drm_i915_private *i915) +{ + unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; + unsigned int max_bw = 0; + unsigned int max_bw_point_mask = 0; + int i; + + for (i = 0; i < num_psf_gv_points; i++) { + unsigned int max_data_rate = adl_psf_bw(i915, i); + + if (max_data_rate > max_bw) { + max_bw_point_mask = BIT(i); + max_bw = max_data_rate; + } else if (max_data_rate == max_bw) + max_bw_point_mask |= BIT(i); + } + + return max_bw_point_mask; +} + +static void icl_force_disable_sagv(struct drm_i915_private *i915, struct intel_bw_state *bw_state) +{ + unsigned int max_bw_qgv_point = icl_max_bw_qgv_point(i915, 0); + unsigned int max_bw_psf_gv_point_mask = icl_max_bw_psf_gv_point_mask(i915); + unsigned int qgv_points; + unsigned int psf_points; + int ret; + + /* + * 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(i915)) + return; + + qgv_points = BIT(max_bw_qgv_point); + psf_points = max_bw_psf_gv_point_mask; + + bw_state->qgv_points_mask = ~(ICL_PCODE_REQ_QGV_PT(qgv_points)| + ADLS_PCODE_REQ_PSF_PT(psf_points)) & + icl_qgv_points_mask(i915); + + drm_dbg_kms(&i915->drm, "Forcing SAGV disable: mask %x\n", bw_state->qgv_points_mask); + + ret = icl_pcode_restrict_qgv_points(i915, bw_state->qgv_points_mask); + + if (ret) + drm_dbg_kms(&i915->drm, "Restricting GV points failed: %x\n", ret); + else + drm_dbg_kms(&i915->drm, "Restricting GV points succeeded\n"); + +} + static int mtl_find_qgv_points(struct drm_i915_private *i915, unsigned int data_rate, unsigned int num_active_planes, @@ -943,7 +1005,7 @@ static int icl_find_qgv_points(struct drm_i915_private *i915, for (i = 0; i < num_qgv_points; i++) { unsigned int max_data_rate; - max_data_rate = adl_qgv_bw(i915, i, num_active_planes); + max_data_rate = adl_qgv_bw(i915, num_active_planes, i); if (max_data_rate >= data_rate) qgv_points |= BIT(i); @@ -1351,5 +1413,8 @@ int intel_bw_init(struct drm_i915_private *dev_priv) intel_atomic_global_obj_init(dev_priv, &dev_priv->display.bw.obj, &state->base, &intel_bw_funcs); + if (DISPLAY_VER(dev_priv) >= 11) + icl_force_disable_sagv(dev_priv, state); + return 0; } diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index 56588d6e24ae..706f5afbbab4 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -63,7 +63,7 @@ static bool skl_needs_memory_bw_wa(struct drm_i915_private *i915) return DISPLAY_VER(i915) == 9; } -static bool +bool intel_has_sagv(struct drm_i915_private *i915) { return HAS_SAGV(i915) && diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h index fb0da36fd3ec..4cca93cd83ad 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.h +++ b/drivers/gpu/drm/i915/display/skl_watermark.h @@ -25,6 +25,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state); void intel_sagv_post_plane_update(struct intel_atomic_state *state); bool intel_can_enable_sagv(struct drm_i915_private *i915, const struct intel_bw_state *bw_state); +bool intel_has_sagv(struct drm_i915_private *i915); u32 skl_ddb_dbuf_slice_mask(struct drm_i915_private *i915, const struct skl_ddb_entry *entry);
Problem is that on some platforms, we do get QGV point mask in wrong state on boot. However driver assumes it is set to 0 (i.e all points allowed), however in reality we might get them all restricted, causing issues. Lets disable SAGV initially to force proper QGV point state. If more QGV points are available, driver will recalculate and update those then after next commit. v2: - Added trace to see which QGV/PSF GV point is used when SAGV is disabled. v3: - Move force disable function to intel_bw_init in order to initialize bw state as well, so that hw/sw are immediately in sync after init. v4: - Don't try sending PCode request, seems like it is not possible at intel_bw_init, however assigning bw->state to be restricted as if SAGV is off, still forces driveer to send PCode request anyway on next modeset, so the solution still works. However we still need to address the case, when no display is connected, which anyway requires much more changes. v5: - Put PCode request back and apply temporary hack to make the request succeed(in case if there 2 PSF GV points with same BW, PCode accepts only if both points are restricted/unrestricted same time) - Fix argument sequence for adl_qgv_bw(Ville Syrjälä) v6: Fix wrong platform checks, not to break everything else. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_bw.c | 73 ++++++++++++++++++-- drivers/gpu/drm/i915/display/skl_watermark.c | 2 +- drivers/gpu/drm/i915/display/skl_watermark.h | 1 + 3 files changed, 71 insertions(+), 5 deletions(-)