[PATCH 3/3] drm/i915: Disable SAGV on bw init, to force QGV point recalculation

Govindapillai, Vinod vinod.govindapillai at intel.com
Wed Mar 20 22:33:26 UTC 2024


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 at 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);



More information about the Intel-gfx mailing list