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

Thasleem, Mohammed
2026-03-17T08:13:45Z

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