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

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Ville Syrjälä (2025-10-16 18:03:45-03:00)
>On Wed, Oct 15, 2025 at 12:15:15AM -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.
>> 
>> Bspec: 68986, 69126
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/skl_watermark.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
>> index 41f64e347436..88342d07727f 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
>> @@ -3249,6 +3249,13 @@ adjust_wm_latency(struct intel_display *display)
>>  
>>          make_wm_latency_monotonic(display);
>>  
>> +        /*
>> +         * Xe3p asks to ignore wm[0] read from the register and always
>> +         * use the adjustment done with read_latency.
>> +         */
>> +        if (DISPLAY_VER(display) >= 35)
>> +                wm[0] = 0;
>
>make_wm_latency_monotonic() already used wm[0]. I think this
>needs to be the very first thing you do in adjust_wm_latency().

Right.  Or as an alternative, maybe we could have
make_wm_latency_monotonic() be the last thing to be done?

I was thinking about having this as the end result:

    |diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
    |index 237af46c1974..b3f8cbadeb99 100644
    |--- a/drivers/gpu/drm/i915/display/skl_watermark.c
    |+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
    |@@ -3213,39 +3213,44 @@ 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);
    |-
    | 	/*
    | 	 * Xe3p asks to ignore wm[0] read from the register and always
    | 	 * use the adjustment done with read_latency.
    | 	 */
    |-	if (DISPLAY_VER(display) >= 35)
    |+	if (DISPLAY_VER(display) >= 35) {
    | 		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.
    |-	 */
    |-	if (wm[0] == 0)
    |-		increase_wm_latency(display, wm_read_latency(display));
    |+		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);
    |+	}
    | 
    | 	/*
    | 	 * WA Level-0 adjustment for 16Gb+ DIMMs: SKL+
    | 	 * If we could not get dimm info enable this WA to prevent from
    | 	 * any underrun. If not able to get DIMM info assume 16Gb+ DIMM
    | 	 * to avoid any underrun.
    | 	 */
    | 	if (need_16gb_dimm_wa(display))
    |-		increase_wm_latency(display, 1);
    |+		inc += 1;
    |+
    |+	if (inc)
    |+		increase_wm_latency(display, inc);
    |+
    |+	make_wm_latency_monotonic(display);
    | }
    | 
    | static void mtl_read_wm_latency(struct intel_display *display)


With that, we:

    * make sure to differentiate between WaWmMemoryReadLatency
      and what now is a "normal" Bspec instruction starting with
      Xe3p_LD.

    * have a single call to increase_wm_latency().

It could be split into 2 patches, if you prefer: first to use a single
call to increase_wm_latency() and then another for Xe3p_LPD (which would
include moving make_wm_latency_monotonic()).

What do you think?

--
Gustavo Sousa

>
>> +
>>          /*
>>           * WaWmMemoryReadLatency
>>           *
>> 
>> -- 
>> 2.51.0
>
>-- 
>Ville Syrjälä
>Intel