On Wed, Sep 24, 2025 at 08:54:10AM -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]>
Looks correct, thanks for catching it: Reviewed-by: Imre Deak <[email protected]> I wonder about Cc'ing stable. > --- > 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 39b71fffa2cd..d057bbde42c2 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c > @@ -1582,8 +1582,8 @@ static const struct i915_power_well_desc_list > xe2lpd_power_wells[] = { > I915_PW_DESCRIPTORS(i9xx_power_wells_always_on), > 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(xe2lpd_power_wells_pica), > + I915_PW_DESCRIPTORS(xelpdp_power_wells_main), > }; > > /* > @@ -1713,8 +1713,8 @@ static const struct i915_power_well_desc_list > xe3lpd_power_wells[] = { > I915_PW_DESCRIPTORS(i9xx_power_wells_always_on), > 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(xe2lpd_power_wells_pica), > + I915_PW_DESCRIPTORS(xe3lpd_power_wells_main), > }; > > static const struct i915_power_well_desc wcl_power_wells_main[] = { > @@ -1766,8 +1766,8 @@ static const struct i915_power_well_desc_list > wcl_power_wells[] = { > I915_PW_DESCRIPTORS(i9xx_power_wells_always_on), > 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(xe2lpd_power_wells_pica), > + I915_PW_DESCRIPTORS(wcl_power_wells_main), > }; > > static void init_power_well_domains(const struct i915_power_well_instance > *inst, > > --- > base-commit: 308a05859081aae4125b58d186d582b814c6deb2 > change-id: 20250923-pica-power-before-aux-70009ccd1b7b > > Best regards, > -- > Gustavo Sousa <[email protected]> >
