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@xxxxxxxxx>
> ---
> 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);
