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]> > --- > 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 >
