Re: [PATCH i-g-t v1] tests/kms_async_flips: Add psr async flip subtest

["Reddy Guddati, Santhosh" <santhosh.reddy.guddati@intel.com>]

Hi Chaitanya,

Thanks for the review. Please find comment inline

On 25-03-2025 15:09, Borah, Chaitanya Kumar wrote:
> 
> 
>> -----Original Message-----
>> From: Reddy Guddati, Santhosh <santhosh.reddy.guddati@intel.com>
>> Sent: Monday, March 24, 2025 9:43 PM
>> To: igt-dev@lists.freedesktop.org
>> Cc: B S, Karthik <karthik.b.s@intel.com>; Murthy, Arun R
>> <arun.r.murthy@intel.com>; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@intel.com>; Reddy Guddati, Santhosh
>> <santhosh.reddy.guddati@intel.com>
>> Subject: [PATCH i-g-t v1] tests/kms_async_flips: Add psr async flip subtest
>>
>> Add a new subtest to verify async flips does not cause PSR exit.
>> Enable PSR and execute async flips to verify system remains in PSR mode after
>> async flips.
>>
>> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com>
>> ---
>>   tests/kms_async_flips.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c index
>> da426f753..cf6b63b3e 100644
>> --- a/tests/kms_async_flips.c
>> +++ b/tests/kms_async_flips.c
>> @@ -85,6 +85,9 @@
>>    *
>>    * SUBTEST: async-flip-suspend-resume
>>    * Description: Verify the async flip functionality with suspend and resume
>> cycle
>> + *
>> + * SUBTEST: psr-async-flip
>> + * Description: Verify that async flips do not cause PSR exit
>>    */
>>
>>   #define CURSOR_POS 128
>> @@ -102,6 +105,7 @@ IGT_TEST_DESCRIPTION("Test asynchronous page
>> flips.");
>>
>>   typedef struct {
>>   	int drm_fd;
>> +	int debugfs_fd;
>>   	uint32_t crtc_id;
>>   	uint32_t refresh_rate;
>>   	struct igt_fb bufs[NUM_FBS];
>> @@ -740,6 +744,33 @@ static void run_test_with_modifiers(data_t *data,
>> void (*test)(data_t *))
>>   	}
>>   }
>>
>> +static bool psr_wait_entry_if_enabled(data_t *data) {
>> +	igt_skip_on_f(!is_psr_enable_possible(data->drm_fd, PSR_MODE_1),
>> +		      "enable_psr modparam doesn't allow PSR mode 1\n");
>> +
>> +	return psr_wait_entry(data->debugfs_fd, PSR_MODE_1, data-
>>> output); }
>> +
>> +static void test_psr_async_flip(data_t *data) {
>> +	if (!psr_sink_support(data->drm_fd, data->debugfs_fd, PSR_MODE_1,
>> data->output))
>> +		igt_skip("PSR mode 1 is not supported on this output\n");
>> +
>> +	psr_enable(data->drm_fd, data->debugfs_fd, PSR_MODE_1, NULL);
>> +	igt_display_commit(&data->display);
> 
> Why only PSR_MODE_1?

The intent of this subtest is to add basic PSR concurrent with Async 
flip tests. I will be adding other subtests to validate additional modes 
in follow-up patches. Please let me know if this approach is acceptable

Thanks,
Santhosh>
>> +
>> +	/* Confirm PSR entry before starting async flips */
>> +	igt_assert_f(psr_wait_entry_if_enabled(data),
>> +			 "PSR is not enabled before async flip test\n");
>> +
>> +	test_async_flip(data);
> 
> This will still do the performance check
> 
>          if (!data->alternate_sync_async) {
>                  fps = frame * 1000 / RUN_TIME;
>                  igt_assert_f((fps / 1000) > (data->refresh_rate * MIN_FLIPS_PER_FRAME),
>                               "FPS should be significantly higher than the refresh rate\n");
>          }
> 
> I think there is no harm in it but something to be aware of.
> 
Agreed ! But It would be good to have this to be consistent with other 
tests.

Thanks,
Santhosh> Regards
> 
> Chaitanya
> 
>> +
>> +	/* Confirm PSR is still active after async flips */
>> +	igt_assert_f(psr_wait_entry_if_enabled(data),
>> +			 "PSR is not enabled after async flip test\n"); }
>> +
>>   static data_t data;
>>
>>   igt_main
>> @@ -748,6 +779,7 @@ igt_main
>>
>>   	igt_fixture {
>>   		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>> +		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
>>   		kmstest_set_vt_graphics_mode();
>>   		igt_display_require(&data.display, data.drm_fd);
>>   		igt_display_require_output(&data.display);
>> @@ -896,6 +928,12 @@ igt_main
>>   		run_test(&data, test_async_flip);
>>   	}
>>
>> +	igt_describe("Verify that async flips do not cause PSR exit");
>> +	igt_subtest_with_dynamic("psr-async-flip") {
>> +		data.atomic_path = false;
>> +		run_test(&data, test_psr_async_flip);
>> +	}
>> +
>>   	igt_fixture {
>>   		for (i = 0; i < NUM_FBS; i++)
>>   			igt_remove_fb(data.drm_fd, &data.bufs[i]);
>> --
>> 2.34.1
>