Quoting Ville Syrjälä (2025-10-29 19:22:01-03:00) >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. > >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. > >> /* >> * 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.
I liked the fact that we would be calling increase_wm_latency() only once... Not a big deal though. -- Gustavo Sousa >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
