Re: [PATCH v2 13/32] drm/i915/xe3p_lpd: Adapt to updates on MBUS_CTL/DBUF_CTL registers

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

Quoting Matt Roper (2025-10-29 18:22:15-03:00)
>On Tue, Oct 21, 2025 at 09:28:38PM -0300, Gustavo Sousa wrote:
>> From: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
>> 
>> Some of the register fields of MBUS_CTL and DBUF_CTL register are
>> changed for Xe3p_LPD platforms. Update the changed fields in the driver.
>> Below are the changes:
>> 
>> MBUS_CTL:
>>         Translation Throttle Min
>>                 It changed from BIT[15:13] to BIT[16:13]
>> 
>> DBUF_CTL:
>>         Min Tracker State Service
>>                 It changed from BIT[18:16] to BIT[20:16]
>>         Max Tracker State Service
>>                 It changed to from BIT[23:19] to BIT[14:10]
>>                 but using default value, so no need to define
>>                 in code.
>
>In a lot of cases when a register field picks up extra high bit(s), the
>extra bits were previously reserved, so it's fine to just adjust the
>existing definition (since reserved bits are required to always read out
>of hardware as zeroes).  However in these cases, the new bits these
>fields are extending into were actively used by the hardware for other
>purposes on previous platforms, which is why it's necessary to keep the
>existing pre-Xe3p definitions unchanged and create separate Xe3p ones
>that can be used only on the newer Xe3p platforms.  You should make some
>mention of that in the commit message so it's clear why we're handling
>these a bit differently than a lot of other registers.

Agreed.  Just updated the local v3 to make that clear.

>
>> 
>> v2:
>>   - Keep definitions in the same line (i.e. without line continuation
>>     breaks) for better readability. (Jani)
>> 
>> Bspec: 68868, 68872
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/skl_watermark.c      | 16 +++++--
>>  drivers/gpu/drm/i915/display/skl_watermark_regs.h | 52 ++++++++++++-----------
>>  2 files changed, 40 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
>> index 256162da9afc..c141d575009f 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
>> @@ -3477,7 +3477,10 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct intel_display *display,
>>          if (!HAS_MBUS_JOINING(display))
>>                  return;
>>  
>> -        if (DISPLAY_VER(display) >= 20)
>> +        if (DISPLAY_VER(display) >= 35)
>> +                intel_de_rmw(display, MBUS_CTL, XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK,
>> +                             XE3P_MBUS_TRANSLATION_THROTTLE_MIN(ratio - 1));
>> +        else if (DISPLAY_VER(display) >= 20)
>>                  intel_de_rmw(display, MBUS_CTL, MBUS_TRANSLATION_THROTTLE_MIN_MASK,
>>                               MBUS_TRANSLATION_THROTTLE_MIN(ratio - 1));
>>  
>> @@ -3488,9 +3491,14 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct intel_display *display,
>>                      ratio, str_yes_no(joined_mbus));
>>  
>>          for_each_dbuf_slice(display, slice)
>> -                intel_de_rmw(display, DBUF_CTL_S(slice),
>> -                             DBUF_MIN_TRACKER_STATE_SERVICE_MASK,
>> -                             DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
>> +                if (DISPLAY_VER(display) >= 35)
>> +                        intel_de_rmw(display, DBUF_CTL_S(slice),
>> +                                     XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK,
>> +                                     XE3P_DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
>> +                else
>> +                        intel_de_rmw(display, DBUF_CTL_S(slice),
>> +                                     DBUF_MIN_TRACKER_STATE_SERVICE_MASK,
>> +                                     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
>>  }
>>  
>>  static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark_regs.h b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> index c5572fc0e847..94915afc6b0b 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> @@ -32,16 +32,18 @@
>>  #define MBUS_BBOX_CTL_S1                _MMIO(0x45040)
>>  #define MBUS_BBOX_CTL_S2                _MMIO(0x45044)
>>  
>> -#define MBUS_CTL                                _MMIO(0x4438C)
>> -#define   MBUS_JOIN                                REG_BIT(31)
>> -#define   MBUS_HASHING_MODE_MASK                REG_BIT(30)
>> -#define   MBUS_HASHING_MODE_2x2                        REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0)
>> -#define   MBUS_HASHING_MODE_1x4                        REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1)
>> -#define   MBUS_JOIN_PIPE_SELECT_MASK                REG_GENMASK(28, 26)
>> -#define   MBUS_JOIN_PIPE_SELECT(pipe)                REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe)
>> -#define   MBUS_JOIN_PIPE_SELECT_NONE                MBUS_JOIN_PIPE_SELECT(7)
>> -#define   MBUS_TRANSLATION_THROTTLE_MIN_MASK        REG_GENMASK(15, 13)
>> -#define   MBUS_TRANSLATION_THROTTLE_MIN(val)        REG_FIELD_PREP(MBUS_TRANSLATION_THROTTLE_MIN_MASK, val)
>> +#define MBUS_CTL                                        _MMIO(0x4438C)
>> +#define   MBUS_JOIN                                        REG_BIT(31)
>> +#define   MBUS_HASHING_MODE_MASK                        REG_BIT(30)
>> +#define   MBUS_HASHING_MODE_2x2                                REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0)
>> +#define   MBUS_HASHING_MODE_1x4                                REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1)
>> +#define   MBUS_JOIN_PIPE_SELECT_MASK                        REG_GENMASK(28, 26)
>> +#define   MBUS_JOIN_PIPE_SELECT(pipe)                        REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe)
>> +#define   MBUS_JOIN_PIPE_SELECT_NONE                        MBUS_JOIN_PIPE_SELECT(7)
>> +#define   MBUS_TRANSLATION_THROTTLE_MIN_MASK                REG_GENMASK(15, 13)
>> +#define   MBUS_TRANSLATION_THROTTLE_MIN(val)                REG_FIELD_PREP(MBUS_TRANSLATION_THROTTLE_MIN_MASK, val)
>> +#define   XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK        REG_GENMASK(16, 13)
>> +#define   XE3P_MBUS_TRANSLATION_THROTTLE_MIN(val)        REG_FIELD_PREP(XE3P_MBUS_TRANSLATION_THROTTLE_MIN_MASK, val)
>
>Nitpick:  I'm not sure if we're 100% consistent, but I feel like we
>usually sort bitfields based on their upper bound rather than their
>lower bound.  So even though xe3p and pre-xe3p start at the same bit 13,
>the xe3p should probably be sorted first since it ends at a higher bit
>(16 vs 15).

Ack.

Thanks!

--
Gustavo Sousa

>
>>  
>>  /*
>>   * The below are numbered starting from "S1" on gen11/gen12, but starting
>> @@ -51,21 +53,23 @@
>>   * way things will be named by the hardware team going forward, plus it's more
>>   * consistent with how most of the rest of our registers are named.
>>   */
>> -#define _DBUF_CTL_S0                                0x45008
>> -#define _DBUF_CTL_S1                                0x44FE8
>> -#define _DBUF_CTL_S2                                0x44300
>> -#define _DBUF_CTL_S3                                0x44304
>> -#define DBUF_CTL_S(slice)                        _MMIO(_PICK(slice, \
>> -                                                            _DBUF_CTL_S0, \
>> -                                                            _DBUF_CTL_S1, \
>> -                                                            _DBUF_CTL_S2, \
>> -                                                            _DBUF_CTL_S3))
>> -#define  DBUF_POWER_REQUEST                        REG_BIT(31)
>> -#define  DBUF_POWER_STATE                        REG_BIT(30)
>> -#define  DBUF_TRACKER_STATE_SERVICE_MASK        REG_GENMASK(23, 19)
>> -#define  DBUF_TRACKER_STATE_SERVICE(x)                REG_FIELD_PREP(DBUF_TRACKER_STATE_SERVICE_MASK, x)
>> -#define  DBUF_MIN_TRACKER_STATE_SERVICE_MASK        REG_GENMASK(18, 16) /* ADL-P+ */
>> +#define _DBUF_CTL_S0                                        0x45008
>> +#define _DBUF_CTL_S1                                        0x44FE8
>> +#define _DBUF_CTL_S2                                        0x44300
>> +#define _DBUF_CTL_S3                                        0x44304
>> +#define DBUF_CTL_S(slice)                                _MMIO(_PICK(slice, \
>> +                                                                    _DBUF_CTL_S0, \
>> +                                                                    _DBUF_CTL_S1, \
>> +                                                                    _DBUF_CTL_S2, \
>> +                                                                    _DBUF_CTL_S3))
>> +#define  DBUF_POWER_REQUEST                                REG_BIT(31)
>> +#define  DBUF_POWER_STATE                                REG_BIT(30)
>> +#define  DBUF_TRACKER_STATE_SERVICE_MASK                REG_GENMASK(23, 19)
>> +#define  DBUF_TRACKER_STATE_SERVICE(x)                        REG_FIELD_PREP(DBUF_TRACKER_STATE_SERVICE_MASK, x)
>> +#define  DBUF_MIN_TRACKER_STATE_SERVICE_MASK                REG_GENMASK(18, 16) /* ADL-P+ */
>>  #define  DBUF_MIN_TRACKER_STATE_SERVICE(x)                REG_FIELD_PREP(DBUF_MIN_TRACKER_STATE_SERVICE_MASK, x) /* ADL-P+ */
>> +#define  XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK        REG_GENMASK(20, 16)
>> +#define  XE3P_DBUF_MIN_TRACKER_STATE_SERVICE(x)                REG_FIELD_PREP(XE3P_DBUF_MIN_TRACKER_STATE_SERVICE_MASK, x)
>
>Same here.
>
>
>Matt
>
>>  
>>  #define MTL_LATENCY_LP0_LP1                _MMIO(0x45780)
>>  #define MTL_LATENCY_LP2_LP3                _MMIO(0x45784)
>> 
>> -- 
>> 2.51.0
>> 
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation