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

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

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

Looks correct, thanks for catching it:

Reviewed-by: Imre Deak <imre.deak@intel.com>

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