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

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

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ä <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.

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.

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

-- 
Ville Syrjälä
Intel