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;
|+
| 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
