Re: [PATCH v4 02/13] drm/i915/display: Switch DC3CO enable from standalone bit to DC level encoding

[Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>]

On 01-06-2026 10:55, Manna, Animesh wrote:
>
>> -----Original Message-----
>> From: Dibin Moolakadan Subrahmanian
>> <dibin.moolakadan.subrahmanian@intel.com>
>> Sent: Wednesday, May 27, 2026 12:48 AM
>> To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Cc: Manna, Animesh <animesh.manna@intel.com>; Shankar, Uma
>> <uma.shankar@intel.com>
>> Subject: [PATCH v4 02/13] drm/i915/display: Switch DC3CO enable from
>> standalone bit to DC level encoding
>>
>> On platforms prior to xe3, DC3CO was controlled via a standalone enable bit.
>> Starting with xe3, DC3CO is encoded as part of the existing
>> DC_STATE_EN_UPTO_DC* field.
>>
>> No functional change, as DC3CO is not enabled on platforms prior to xe3.
>>
>> Changes in v2:
>> - Update commit header (Uma Shankar)
>>
>> Signed-off-by: Dibin Moolakadan Subrahmanian
>> <dibin.moolakadan.subrahmanian@intel.com>
>> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display_power.c      | 6 +++---
>>   drivers/gpu/drm/i915/display/intel_display_power_well.c | 4 ++--
>>   drivers/gpu/drm/i915/display/intel_display_regs.h       | 2 +-
>>   drivers/gpu/drm/i915/display/intel_dmc_wl.c             | 2 +-
>>   4 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
>> b/drivers/gpu/drm/i915/display/intel_display_power.c
>> index 751e6b7d4a29..c70971ffd9f0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> @@ -267,7 +267,7 @@ sanitize_target_dc_state(struct intel_display *display,
>>   	static const u32 states[] = {
>>   		DC_STATE_EN_UPTO_DC6,
>>   		DC_STATE_EN_UPTO_DC5,
>> -		DC_STATE_EN_DC3CO,
>> +		DC_STATE_EN_UPTO_DC3CO,
>>   		DC_STATE_DISABLE,
>>   	};
>>   	int i;
>> @@ -999,10 +999,10 @@ static u32 get_allowed_dc_mask(struct
>> intel_display *display, int enable_dc)
>>
>>   	switch (requested_dc) {
>>   	case 4:
>> -		mask |= DC_STATE_EN_DC3CO | DC_STATE_EN_UPTO_DC6;
>> +		mask |= DC_STATE_EN_UPTO_DC3CO |
>> DC_STATE_EN_UPTO_DC6;
>>   		break;
>>   	case 3:
>> -		mask |= DC_STATE_EN_DC3CO | DC_STATE_EN_UPTO_DC5;
>> +		mask |= DC_STATE_EN_UPTO_DC3CO |
>> DC_STATE_EN_UPTO_DC5;
>>   		break;
>>   	case 2:
>>   		mask |= DC_STATE_EN_UPTO_DC6;
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> index 2f0d0a77c1a2..611f784d8a7a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> @@ -772,7 +772,7 @@ static u32 gen9_dc_mask(struct intel_display
>> *display)
>>   	mask = DC_STATE_EN_UPTO_DC5;
>>
>>   	if (DISPLAY_VER(display) >= 12)
>> -		mask |= DC_STATE_EN_DC3CO | DC_STATE_EN_UPTO_DC6
>> +		mask |= DC_STATE_EN_UPTO_DC3CO |
>> DC_STATE_EN_UPTO_DC6
>>   					  | DC_STATE_EN_DC9;
>>   	else if (DISPLAY_VER(display) == 11)
>>   		mask |= DC_STATE_EN_UPTO_DC6 | DC_STATE_EN_DC9;
>> @@ -1022,7 +1022,7 @@ static void
>> bxt_verify_dpio_phy_power_wells(struct intel_display *display)  static bool
>> gen9_dc_off_power_well_enabled(struct intel_display *display,
>>   					   struct i915_power_well
>> *power_well)  {
>> -	return ((intel_de_read(display, DC_STATE_EN) &
>> DC_STATE_EN_DC3CO) == 0 &&
>> +	return ((intel_de_read(display, DC_STATE_EN) &
>> DC_STATE_EN_UPTO_DC3CO)
>> +== 0 &&
>>   		(intel_de_read(display, DC_STATE_EN) &
>> DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0);  }
> Both DC_STATE_EN_UPTO_DC5_DC6_MASK and DC_STATE_EN_UPTO_DC3CO value is matching which will impact fine granularity to control DC3co.
> Only enabling Dc3co or enabling with DC5/DC6 may be an issue.

Thanks for the review.
Since the DC3CO enable value is 0x3,
DC_STATE_EN_UPTO_DC3CO and DC_STATE_EN_UPTO_DC5_DC6_MASK evaluate to the same value.

That said, DC_STATE_EN_UPTO_DC5_DC6_MASK can be renamed
to DC_STATE_EN_UPTO_DC3CO_DC5_DC6_MASK to better reflect the bits covered by the mask.

I assumed DC3CO-only enablement would be mainly for validation purposes.
Would it be okay to handle that in a follow-up?

>
> Regards,
> Animesh
>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h
>> b/drivers/gpu/drm/i915/display/intel_display_regs.h
>> index 4321f8b529da..680e7dfdcf1b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
>> @@ -3070,13 +3070,13 @@ enum skl_power_gate {
>>   /* GEN9 DC */
>>   #define DC_STATE_EN			_MMIO(0x45504)
>>   #define  DC_STATE_DISABLE		0
>> -#define  DC_STATE_EN_DC3CO		REG_BIT(30)
>>   #define  DC_STATE_DC3CO_STATUS		REG_BIT(29)
>>   #define  HOLD_PHY_CLKREQ_PG1_LATCH	REG_BIT(21)
>>   #define  HOLD_PHY_PG1_LATCH		REG_BIT(20)
>>   #define  DC_STATE_EN_UPTO_DC5		(1 << 0)
>>   #define  DC_STATE_EN_DC9		(1 << 3)
>>   #define  DC_STATE_EN_UPTO_DC6		(2 << 0)
>> +#define  DC_STATE_EN_UPTO_DC3CO		(3 << 0)
>>   #define  DC_STATE_EN_UPTO_DC5_DC6_MASK   0x3
>>
>>   #define  DC_STATE_DEBUG                  _MMIO(0x45520)
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index b007343721e1..ab4e0e9573df 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -267,7 +267,7 @@ static bool intel_dmc_wl_check_range(struct
>> intel_display *display,
>>   	 * the DMC and requires a DC exit for proper access.
>>   	 */
>>   	switch (dc_state) {
>> -	case DC_STATE_EN_DC3CO:
>> +	case DC_STATE_EN_UPTO_DC3CO:
>>   		ranges = xe3lpd_dc3co_dmc_ranges;
>>   		break;
>>   	case DC_STATE_EN_UPTO_DC5:
>> --
>> 2.43.0