Re: [PATCH v2 14/32] drm/i915/wm: Reorder adjust_wm_latency() for Xe3_LPD

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

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ä <ville.syrjala@linux.intel.com>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  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.
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