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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 17, 2024 at 05:57:18PM +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.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_bw.h |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 7baa1c13eccd..36a6304207ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -852,6 +852,27 @@ static unsigned int icl_max_bw_qgv_point(struct drm_i915_private *i915,
>  	return max_bw_point;
>  }
>  
> +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 qgv_points;
> +	unsigned int psf_points;
> +
> +	qgv_points = BIT(max_bw_qgv_point);
> +
> +	/*
> +	 * We don't restrict PSF GV points, when disabling SAGV
> +	 */
> +	psf_points = 0;

Using 0 looks very wrong here. Since we have no idea how much
bandwidth the display is consuming at this time we should
restrict this to the max psf gv point as well.

> +
> +	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: leaving QGV point %d\n",
> +				max_bw_qgv_point);

You didn't actually poke the hardware to disable anything.

> +}
> +
>  static int mtl_find_qgv_points(struct drm_i915_private *i915,
>  			       unsigned int data_rate,
>  			       unsigned int num_active_planes,
> @@ -1351,5 +1372,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) < 14)

Should be some kind of range check to avoid putting garbage in there on
old platforms that don't support QGV.

> +		icl_force_disable_sagv(dev_priv, state);
> +
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 59cb4fc5db76..243192fd4cae 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -74,5 +74,7 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
>  			    bool *need_cdclk_calc);
>  int intel_bw_min_cdclk(struct drm_i915_private *i915,
>  		       const struct intel_bw_state *bw_state);
> +void icl_force_disable_sagv(struct drm_i915_private *dev_priv,
> +			    struct intel_bw_state *bw_state);

Why?

>  
>  #endif /* __INTEL_BW_H__ */
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel