On Thu, Oct 30, 2025 at 10:45:56AM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjälä (2025-10-29 19:36:14-03:00)
> >On Thu, Oct 30, 2025 at 12:22:01AM +0200, Ville Syrjälä wrote:
> >> On Tue, Oct 21, 2025 at 09:28:39PM -0300, Gustavo Sousa wrote:
> >> > In an upcoming change related to Xe3p_LPD, we will need to (i) update
> >> > wm[0] in adjust_wm_latency() and (ii) do the same increase_wm_latency()
> >> > that is currently done when (wm[0] == 0).
> >> >
> >> > Because make_wm_latency_monotonic() depends on wm[0], part (i) needs to
> >> > be done before it gets called. In order to keep (i) and (ii) as a
> >> > contiguous logical sequence, let's reorder adjust_wm_latency(), making
> >> > make_wm_latency_monotonic() the last thing to be done.
> >> >
> >> > Also take this opportunity to simplify the code by doing the call to
> >> > increase_wm_latency() only once.
> >> >
> >> > Cc: Ville Syrjälä <[email protected]>
> >> > Signed-off-by: Gustavo Sousa <[email protected]>
> >> > ---
> >> > drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++----
> >> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> >> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> >> > index c141d575009f..57260a2a765a 100644
> >> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> >> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> >> > @@ -3213,14 +3213,13 @@ static void
> >> > adjust_wm_latency(struct intel_display *display)
> >> > {
> >> > u16 *wm = display->wm.skl_latency;
> >> > + int inc = 0;
> >> >
> >> > if (display->platform.dg2)
> >> > multiply_wm_latency(display, 2);
> >> >
> >> > sanitize_wm_latency(display);
> >> >
> >> > - make_wm_latency_monotonic(display);
> >> > -
> >>
> >> I was thinking that by doing this early we avoid potentially papering
> >> over our own bugs in the later adjustments. But Windows does appear to
> >> do this after the read latency adjustment.
> >
> >Hmm, I suppose it doesn't really matter whether it's before or after
> >since the read latency adjustment applies to all wm levels. So I think
> >I still prefer to keep it early to avoid papering over our own bugs.
>
> Okay. In this case, I guess I can drop this patch then and go back to
> the original approach + moving the assignment of "wm[0] = 0" to be done
> earlier.
>
> >
> >>
> >> And it looks like Windows actually stopped doing this for xe3 and now
> >> it rejects non-monotonic values. And it also does that after the read
> >> latency adjustment.
> >>
> >> So I guess what we want to do is move this later, only call it for
> >> pre-xe3, and add another step after it to validate that the latencies
> >> are indeed monotonic.
>
> I guess in our case, we would reject them in sanitize_wm_latency(),
> making everything after the invalid level (i.e. wm[level] < wm[level -
> 1]) be forced to zero, right?
>
> In summary, with keeping make_wm_latency_monotonic() early, something
> like this?
>
> |diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> |index c141d575009f..6cf1565dcefd 100644
> |--- a/drivers/gpu/drm/i915/display/skl_watermark.c
> |+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> |@@ -3183,11 +3183,16 @@ static void sanitize_wm_latency(struct
> intel_display *display)
> | int level, num_levels = display->wm.num_levels;
> |
> | /*
> |- * If a level n (n > 1) has a 0us latency, all levels m (m >= n)
> |- * need to be disabled. We make sure to sanitize the values out
> |- * of the punit to satisfy this requirement.
> |+ * Two types of sanitization are done here:
> |+ * 1) If a level n (n > 1) has a 0us latency, all levels m (m
> >= n)
> |+ * need to be disabled. We make sure to sanitize the values
> out of
> |+ * the punit to satisfy this requirement.
> |+ * 2) For Xe3 onward, only accept monotonic ranges.
> | */
> | for (level = 1; level < num_levels; level++) {
> |+ if (DISPLAY_VER(display) >= 30 && wm[level] < wm[level
> - 1])
> |+ wm[level] = 0;
I don't think we want to sweep these under the rug.
IMO we should simply not call make_wm_latency_monotonic() on xe3+,
and do some kind of drm_WARN(!is_wm_latency_monotonic()) once
we're done with all the adjustments.
> |+
> | if (wm[level] == 0)
> | break;
> | }
> |@@ -3201,6 +3206,9 @@ static void make_wm_latency_monotonic(struct
> intel_display *display)
> | u16 *wm = display->wm.skl_latency;
> | int level, num_levels = display->wm.num_levels;
> |
> |+ if (DISPLAY_VER(display) < 30)
> |+ return;
> |+
> | for (level = 1; level < num_levels; level++) {
> | if (wm[level] == 0)
> | break;
>
> If so, I could include this patch as part of this series to avoid
> conflicts or keep it as a separate patch...
>
> --
> Gustavo Sousa
>
> >>
> >> > /*
> >> > * WaWmMemoryReadLatency
> >> > *
> >> > @@ -3229,7 +3228,7 @@ adjust_wm_latency(struct intel_display *display)
> >> > * from the punit when level 0 response data is 0us.
> >> > */
> >> > if (wm[0] == 0)
> >> > - increase_wm_latency(display, wm_read_latency(display));
> >> > + inc += wm_read_latency(display);
> >> >
> >> > /*
> >> > * WA Level-0 adjustment for 16Gb+ DIMMs: SKL+
> >> > @@ -3238,7 +3237,12 @@ adjust_wm_latency(struct intel_display *display)
> >> > * to avoid any underrun.
> >> > */
> >> > if (need_16gb_dimm_wa(display))
> >> > - increase_wm_latency(display, 1);
> >> > + inc += 1;
> >> > +
> >> > + if (inc)
> >> > + increase_wm_latency(display, inc);
> >>
> >> I don't see that variable being helpful in any real way.
> >> Just makes the function more complicated for no good reason.
> >> It also has nothing to do with the rest of this patch.
> >>
> >> > +
> >> > + make_wm_latency_monotonic(display);
> >> > }
> >> >
> >> > static void mtl_read_wm_latency(struct intel_display *display)
> >> >
> >> > --
> >> > 2.51.0
> >>
> >> --
> >> Ville Syrjälä
> >> Intel
> >
> >--
> >Ville Syrjälä
> >Intel
--
Ville Syrjälä
Intel
