Re: [PATCH v2 15/32] drm/i915/xe3p_lpd: Always apply level-0 watermark adjustment

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

On Wed, Oct 29, 2025 at 03:08:00PM -0700, Matt Roper wrote:
> On Tue, Oct 21, 2025 at 09:28:40PM -0300, Gustavo Sousa wrote:
> > When reading memory latencies for watermark calculations, previous
> > display releases instructed to apply an adjustment of adding a certain
> > value (e.g. 6us) to all levels when the level 0's memory latency read
> > from hardware was zero.
> > 
> > For Xe3p_LPD, the instruction is to always use 6us for level 0 and to
> > add that value to the other levels.  Update adjust_wm_latency()
> > accordingly.
> > 
> > While previously the adjustment was considered a workaround by the
> > driver, for Xe3p_LPD that is part of the formal specification.  So,
> > let's make sure that we differentiate those two in the driver code, even
> > if there is a bit of redundancy with "inc += wm_read_latency(display)"
> > appearing twice in the code.
> > 
> > v2:
> >   - Rebased after addition of prep patch "drm/i915/wm: Reorder
> >     adjust_wm_latency() for Xe3_LPD".
> > 
> > Bspec: 68986, 69126
> > Cc: Matt Atwood <matthew.s.atwood@intel.com>
> > 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 | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 57260a2a765a..5bb6cdc4ad2c 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3220,15 +3220,24 @@ adjust_wm_latency(struct intel_display *display)
> >  
> >  	sanitize_wm_latency(display);
> >  
> > -	/*
> > -	 * WaWmMemoryReadLatency
> > -	 *
> > -	 * punit doesn't take into account the read latency so we need
> > -	 * to add proper adjustment to each valid level we retrieve
> > -	 * from the punit when level 0 response data is 0us.
> > -	 */
> > -	if (wm[0] == 0)
> > +	if (DISPLAY_VER(display) >= 35) {
> > +		/*
> > +		 * Xe3p asks to ignore wm[0] read from the register and always
> > +		 * use the adjustment that adds the read latency to all valid
> > +		 * latency values.
> > +		 */
> > +		wm[0] = 0;
> >  		inc += wm_read_latency(display);
> > +	} else if (wm[0] == 0) {
> > +		/*
> > +		 * WaWmMemoryReadLatency
> > +		 *
> > +		 * punit doesn't take into account the read latency so we need
> > +		 * to add proper adjustment to each valid level we retrieve
> > +		 * from the punit when level 0 response data is 0us.
> > +		 */
> > +		inc += wm_read_latency(display);
> > +	}
> 
> Wouldn't it be simpler to just have a separate
> 
>         /*
>          * Xe3p and beyond should ignore level 0's reported latency and
>          * always apply WaWmMemoryReadLatency logic.
>          */
>         if (DISPLAY_VER(display) >= 35)
>                 wm[0] = 0;
> 
> and leave the rest of the code unchanged?

That, and I think just stuff it into sanitize_wm_latency() so that
the bogus value gets nuked before we use it for anything.

> 
> Either way, matches the spec so
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> Matt
> 
> >  
> >  	/*
> >  	 * WA Level-0 adjustment for 16Gb+ DIMMs: SKL+
> > 
> > -- 
> > 2.51.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Ville Syrjälä
Intel