Re: [PATCH v2 2/2] tests/intel/kms_pm_dc: Add DC5 PSR suspend/resume validation test

["Thasleem, Mohammed" <mohammed.thasleem@intel.com>]

Hi Jeevan,

On 11-03-2026 03:47 pm, B, Jeevan wrote:
>> -----Original Message-----
>> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Mohammed
>> Thasleem
>> Sent: Thursday, January 22, 2026 11:52 PM
>> To: igt-dev@lists.freedesktop.org
>> Cc: Thasleem, Mohammed <mohammed.thasleem@intel.com>
>> Subject: [PATCH v2 2/2] tests/intel/kms_pm_dc: Add DC5 PSR suspend/resume
>> validation test
>>
>> This test validates display engine entry to DC5 state while PSR is active, performs
>> suspend/resume cycle, and verifies DC5 entry is restored after resume.
>>
>> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
>> ---
>>   tests/intel/kms_pm_dc.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/tests/intel/kms_pm_dc.c b/tests/intel/kms_pm_dc.c index
>> fde16ebf7..8cdbfad9f 100644
>> --- a/tests/intel/kms_pm_dc.c
>> +++ b/tests/intel/kms_pm_dc.c
>> @@ -80,6 +80,10 @@
>>    *
>>    * SUBTEST: dc5-dpms-suspend-resume
>>    * Description: This test validate DC5 state entry before and after
>> suspend/resume cycle using DPMS
>> + *
>> + * SUBTEST: dc5-psr-suspend-resume
>> + * Description: This test validates display engine entry to DC5 state while PSR is
>> active
>> + *               before and after respend/resume cycle
>>    */
>>
>>   /* DC State Flags */
>> @@ -732,6 +736,13 @@ static void test_dc5_dpms_suspend_resume(data_t
>> *data, int dc_flag)
>>
>>   }
>>
>> +static void test_dc5_psr_suspend_resume(data_t *data, int dc_flag) {
>> +	test_dc_state_psr(data, dc_flag);
>> +	igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>> SUSPEND_TEST_NONE);
>> +	test_dc_state_psr(data, dc_flag);
>> +}
>> +
>>   static void kms_poll_state_restore(int sig)  {
>>   	int sysfs_fd;
>> @@ -857,6 +868,16 @@ int igt_main()
>>   	igt_subtest("dc5-dpms-suspend-resume")
>>   		test_dc5_dpms_suspend_resume(&data, CHECK_DC5);
>>
>> +	igt_describe("This test validates display engine entry to DC5 state "
>> +		     "while PSR is active before and after suspend/resume cycle");
>> +	igt_subtest("dc5-psr-suspend-resume") {
>> +		igt_require(psr_sink_support(data.drm_fd, data.debugfs_fd,
>> +					     PSR_MODE_1, NULL));
>> +		data.op_psr_mode = PSR_MODE_1;
>> +		psr_enable(data.drm_fd, data.debugfs_fd, data.op_psr_mode,
>> NULL);
>> +		test_dc5_psr_suspend_resume(&data, CHECK_DC5);
>> +	}
>> +
> This test currently validates the DC counter by comparing the value before PSR entry and after waiting for PSR to enter.
> If we want to include suspend/resume validation as part of this test, shouldn't the flow be:
>
> 1. Read the DC counter.
> 2. Perform suspend/resume.
> 3. Wait for PSR entry again.
> 4. Read the DC counter after resume.
> 5. Validate the counter using `check_dc_counter()`.
>
> This would ensure that the DC state entry is also validated after the suspend/resume cycle.
> Same goes for patch 1 also.
Thanks for the review. After analyzing the implementation, the suggested 
approach has technical issues:
1. DC5 counter is a hardware register, Counter is read via 
intel_de_read(display, dc5_reg),
     which is a direct hardware register read, not a software counter in 
driver memory.
2. Hardware register resets to 0 after S3 on older platforms, On older 
platforms (display ver < 14),
     dc5_reg lives in the display power domain which is completely cut 
during S3, causing the
     register to reset to 0 on resume.
3. Suggested approach causes FALSE FAILURE on older platforms, 
baseline=11 captured before
     suspend becomes invalid after register resets to 0. After resume 
DC5 fires once
     giving counter=1, assert(1 > 11) FAILS even though DC5 works.
4. Newer platforms (>= ver 14) behave differently,  DC6 "allowed" count 
is a software counter
     in driver RAM (dmc->dc6_allowed.count), preserved across S3. 
Counter behavior differs between
     platforms making a single baseline across suspend boundary unreliable.
5. Current patch correctly handles both platforms, test_dc_state_psr() 
reads its own fresh
     baseline at call time. Calling it twice before and after suspend 
correctly handles register
     reset (older) and preserved counter (newer), while also isolating 
pre-existing failures from S3 regressions.
6. Runtime PM state also breaks with suggested approach: On BXT/GLK/ICL+ 
splitting
     setup_dc_dpms() and cleanup_dc_dpms() across suspend boundary 
leaves runtime PM in unknown state.
     Current patch keeps setup/cleanup always balanced.
> Thanks
> Jeevan B
>>   	igt_fixture() {
>>   		free(data.debugfs_dump);
>>   		close(data.debugfs_fd);
>> --
>> 2.25.1