Re: [PATCH v2 2/2] drm/i915/display: Enable PICA power before AUX

[Imre Deak <imre.deak@intel.com>]

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 <gustavo.sousa@intel.com>

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 <imre.deak@intel.com>

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