Quoting Imre Deak (2025-10-02 06:05:06-03:00)
>On Wed, Oct 01, 2025 at 01:04:49PM -0300, Gustavo Sousa wrote:
>> According to Bspec, before enabling AUX power, we need to have the
>> "power well containing Aux logic powered up". Starting with Xe2_LPD,
>> such power well is the "PICA" power well, which is managed by the driver
>> on demand.
>>
>> While we did add the mapping of AUX power domains to the PICA power
>> well, we ended up placing its power well descriptor after the
>> descriptor for AUX power. As a result, when enabling power wells for one
>> of the aux power domains, the driver will enable AUX power before PICA
>> power, going against the order specified in Bspec.
>>
>> It appears that issue did not become apparent to us mainly because,
>> luckily, AUX power is brought up after we assert PICA power, even if
>> done in the wrong order; and in enough time for the first AUX
>> transaction to succeed.
>>
>> Furthermore, I have also realized that, in some cases, like driver
>> initialization, PICA power is already up when we need to acquire AUX
>> power.
>>
>> One case where we can observe the incorrect ordering is when the driver
>> is resuming from runtime PM suspend. Here is an excerpt of a dmesg with
>> some extra debug logs extracted from a LNL machine to illustrate the
>> issue:
>>
>> [ +0.000156] xe 0000:00:02.0: [drm:intel_power_well_enable [xe]]
>> enabling AUX_TC1
>> [ +0.001312] xe 0000:00:02.0: [drm:xelpdp_aux_power_well_enable [xe]]
>> DBG: AUX_CH_USBC1 power status: 0
>> [ +0.000127] xe 0000:00:02.0: [drm:intel_power_well_enable [xe]]
>> enabling PICA_TC
>> [ +0.001072] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC1 power status: 1
>> [ +0.000102] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC2 power status: 0
>> [ +0.000090] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC3 power status: 0
>> [ +0.000092] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC4 power status: 0
>>
>> The first "DBG: ..." line shows that AUX power for TC1 is off after we
>> assert and wait. The remaining lines show that AUX power for TC1 was on
>> after we enabled PICA power and waited for AUX power.
>>
>> It is important that we stay compliant with the spec, so let's fix this
>> by listing the power wells in an order that matches the requirements
>> from Bspec. (As a side note, it would be nice if we could define those
>> dependencies explicitly.)
>>
>> After this change, we have:
>>
>> [ +0.000146] xe 0000:00:02.0: [drm:intel_power_well_enable [xe]]
>> enabling PICA_TC
>> [ +0.001417] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC1 power status: 0
>> [ +0.000116] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC2 power status: 0
>> [ +0.000096] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC3 power status: 0
>> [ +0.000094] xe 0000:00:02.0: [drm:xe2lpd_pica_power_well_enable [xe]]
>> DBG: AUX_CH_USBC4 power status: 0
>> [ +0.000095] xe 0000:00:02.0: [drm:intel_power_well_enable [xe]]
>> enabling AUX_TC1
>> [ +0.000915] xe 0000:00:02.0: [drm:xelpdp_aux_power_well_enable [xe]]
>> DBG: AUX_CH_USBC1 power status: 1
>>
>> Bspec: 68967, 68886, 72519
>> Signed-off-by: Gustavo Sousa <[email protected]>
>
>Upgrading my R-b from v1, where I didn't consider that the order of
>enabling/disabling the PICA PW would change wrt. PW_2/pipe PWs. That
>shouldn't be an issue in theory - spec doesn't forbid it -, but Windows
>is also using the PW_2 -> pipe PW -> PICA_PW order, so better to keep
>that order.
>
>Thanks again for debugging and fixing this issue:
>Reviewed-by: Imre Deak <[email protected]>
Pushed to drm-intel-next. Thanks the review!
--
Gustavo Sousa
>
>> ---
>> drivers/gpu/drm/i915/display/intel_display_power_map.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> b/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> index e89f18b7037f..9b49952994ce 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
>> @@ -1588,8 +1588,8 @@ static const struct i915_power_well_desc_list
>> xe2lpd_power_wells[] = {
>> I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
>> I915_PW_DESCRIPTORS(xe2lpd_power_wells_dcoff),
>> I915_PW_DESCRIPTORS(xelpdp_power_wells_main),
>> - I915_PW_DESCRIPTORS(xelpdp_power_wells_aux),
>> I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
>> + I915_PW_DESCRIPTORS(xelpdp_power_wells_aux),
>> };
>>
>> /*
>> @@ -1710,8 +1710,8 @@ static const struct i915_power_well_desc_list
>> xe3lpd_power_wells[] = {
>> I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
>> I915_PW_DESCRIPTORS(xe3lpd_power_wells_dcoff),
>> I915_PW_DESCRIPTORS(xe3lpd_power_wells_main),
>> - I915_PW_DESCRIPTORS(xelpdp_power_wells_aux),
>> I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
>> + I915_PW_DESCRIPTORS(xelpdp_power_wells_aux),
>> };
>>
>> static const struct i915_power_well_desc wcl_power_wells_main[] = {
>> @@ -1768,8 +1768,8 @@ static const struct i915_power_well_desc_list
>> wcl_power_wells[] = {
>> I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
>> I915_PW_DESCRIPTORS(xe3lpd_power_wells_dcoff),
>> I915_PW_DESCRIPTORS(wcl_power_wells_main),
>> - I915_PW_DESCRIPTORS(wcl_power_wells_aux),
>> I915_PW_DESCRIPTORS(xe2lpd_power_wells_pica),
>> + I915_PW_DESCRIPTORS(wcl_power_wells_aux),
>> };
>>
>> static void init_power_well_domains(const struct i915_power_well_instance
>> *inst,
>>
>> --
>> 2.51.0
>>
