On Mon, Feb 13, 2023 at 06:44:53PM +0200, Stanislav Lisovskiy wrote:
> There was a specific SW workaround requested, which should prevent
> some watermark issues happening, which requires copying highest
> enabled wm level to those disabled wm levels(bit 31 is of course
> still needs to be cleared).
> This is related to different subsystems like PSR and others, which
> may still consult a low power wm values ocassionally, despite those
> are disabled. For that reason we need to keep sane values in
> correspondent registers, even when those are disabled.
>
> HSDES: 22016115093
>
> v2: Remove redundant WA for ICL and extend this WA for all platforms
> starting from SKL, as it seems that we needed this anyway on
> all of those(Ville Syrjälä)
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 26 +++++++++++---------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 962666e74333..227a777e4331 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1407,16 +1407,22 @@ skl_check_nv12_wm_level(struct skl_wm_level *wm, struct skl_wm_level *uv_wm,
> }
> }
>
> -static bool icl_need_wm1_wa(struct drm_i915_private *i915,
> - enum plane_id plane_id)
> +static bool skl_need_wm_copy_wa(struct drm_i915_private *i915, int level,
> + const struct skl_plane_wm *wm)
> {
> /*
> * Wa_1408961008:icl, ehl
> * Wa_14012656716:tgl, adl
> - * Underruns with WM1+ disabled
> + * Wa_14017887344:icl
> + * Wa_14017868169:adl, tgl
Was there any w/a number for the "copy everything" issue?
> + * Due to some power saving optimizations, different subsystems
> + * like PSR, might still use even disabled wm level registers,
> + * for "reference", so lets keep at least the values sane.
> + * Considering amount of WA requiring us to do similar things, was
> + * decided to simply do it for all of the platforms, as those wm
> + * levels are disabled, this isn't going to do harm anyway.
> */
> - return DISPLAY_VER(i915) == 11 ||
> - (IS_DISPLAY_VER(i915, 12, 13) && plane_id == PLANE_CURSOR);
> + return DISPLAY_VER(i915) >= 9 && level > 0 && !wm->wm[level].enable;
DISPLAY_VER check us now actually pointless since this
code never runs on pre-skl.
> }
>
> struct skl_plane_ddb_iter {
> @@ -1585,12 +1591,10 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> else
> skl_check_wm_level(&wm->wm[level], ddb);
>
> - if (icl_need_wm1_wa(i915, plane_id) &&
> - level == 1 && !wm->wm[level].enable &&
> - wm->wm[0].enable) {
Hmm. I was wondering about that enable check for the previous level, but
you're right to remove it. We just want to cascade the last valid
value (whether it's disabled or not) upwards to all disabled levels.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> - wm->wm[level].blocks = wm->wm[0].blocks;
> - wm->wm[level].lines = wm->wm[0].lines;
> - wm->wm[level].ignore_lines = wm->wm[0].ignore_lines;
> + if (skl_need_wm_copy_wa(i915, level, wm)) {
> + wm->wm[level].blocks = wm->wm[level - 1].blocks;
> + wm->wm[level].lines = wm->wm[level - 1].lines;
> + wm->wm[level].ignore_lines = wm->wm[level - 1].ignore_lines;
> }
> }
> }
> --
> 2.37.3
--
Ville Syrjälä
Intel
