Quoting Ville Syrjälä (2025-10-29 19:39:47-03:00) >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 <[email protected]> >> > Cc: Ville Syrjälä <[email protected]> >> > Signed-off-by: Gustavo Sousa <[email protected]> >> > --- >> > 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.
Alright. Let me do that then. FWIW, I wanted to keep the code clearly differentiating a WA path from what is now part of the regular spec for Xe3p (or is it still a WA? :-P), hence trying avoiding the split (doing wm[0] = 0 elsewhere). -- Gustavo Sousa > >> >> Either way, matches the spec so >> >> Reviewed-by: Matt Roper <[email protected]> >> >> >> 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
