Re: [PATCH i-g-t v10 2/2] tests/intel/xe_pm_residency: Add subtest for ASPM Link state residency

["Poosa, Karthik" <karthik.poosa@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 13965 bytes --]


On 23-01-2026 19:36, Kamil Konieczny wrote:
> Hi Poosa,,
> On 2026-01-23 at 18:05:01 +0530, Poosa, Karthik wrote:
>> On 23-01-2026 17:37, Kamil Konieczny wrote:
>>> Hi,
>>> On 2026-01-23 at 16:04:11 +0530, Poosa, Karthik wrote:
>>>> On 22-01-2026 20:30, Kamil Konieczny wrote:
>>>>> Hi Karthik,
>>>>> On 2026-01-21 at 20:27:47 +0530, Karthik Poosa wrote:
>>>>>> Add subtest aspm_link_residency to verify PCIe ASPM.
>>>>>> Active State Power Management (ASPM) is a power management mechanism
>>>>>> for PCI Express (PCIe) devices that aims to save power while the devices
>>>>>> are in a fully active state.
>>>>>> This test uses link state counters from the debugfs
>>>>>> dgfx_pcie_link_residencies to verify this.
>>>>>>
>>>>>> v2:
>>>>>>     - Add dedicated function to get pcie endpoint upstream port. (Badal)
>>>>>>     - Read residency counter as unsigned long long int instead of
>>>>>>       unsigned long int.
>>>>>>     - Print residency counter before sleep also.
>>>>>>     - Don't assert if sysfs not corresponding to aspm_link_state
>>>>>>       is not present. (Badal)
>>>>>>     - Run workload before validation of aspm link residency. (Anshuman)
>>>>>>
>>>>>> v3:
>>>>>>     - Move igt_device_get_pci_usp to separate patch. (Kamil)
>>>>>>     - Move reading of residency to separate function. (Badal)
>>>>>>
>>>>>> v4:
>>>>>>     - Add description about PCIe ASPM in commit message and code. (Kamil)
>>>>>>     - Add a NULL check for the return value of igt_device_get_pci_usp().
>>>>>>     - Resolve compilation warnings about using variable as format string
>>>>>>       to sscanf.
>>>>>>
>>>>>> v5:
>>>>>>     - Use igt_device_get_pci_upstream_port() which is the renamed version
>>>>>>       of igt_device_get_pci_usp().
>>>>>>
>>>>>> v6:
>>>>>>     - Refactor and enhance readability. (Badal)
>>>>>>     - Move save and restore of link states to separate functions. (Badal)
>>>>>>
>>>>>> v7:
>>>>>>     - Skip aspm_link_residency on integrated platforms as it not supported.
>>>>>>
>>>>>> v8:
>>>>>>     - Address below review comments from Riana.
>>>>>>     - Use igt_sysfs_has_attr() instead of faccess().
>>>>>>     - Remove unnecessary spaces, debug logs, if checks.
>>>>>>     - Wrap line length to 100 chars.
>>>>>>     - Use spinner instead of mmap for workload.
>>>>>>
>>>>>> v9:
>>>>>>     - Address review comments from Kamil.
>>>>>>     - Simplify couple of igt_asserts.
>>>>>>     - Remove extra spaces.
>>>>>>
>>>>>> v10:
>>>>>>     - Add exit handler for aspm_link_residency test. (Riana)
>>>>>>     - Remove unused and unnecessary variables.
>>>>>>
>>>>>> Signed-off-by: Karthik Poosa<karthik.poosa@intel.com>
>>>>>> Reviewed-by: Badal Nilawar<badal.nilawar@intel.com>
>>>>>> ---
>>>>>>     tests/intel/xe_pm_residency.c | 196 ++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 196 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/intel/xe_pm_residency.c b/tests/intel/xe_pm_residency.c
>>>>>> index d33a87b13..2d965ef7d 100644
>>>>>> --- a/tests/intel/xe_pm_residency.c
>>>>>> +++ b/tests/intel/xe_pm_residency.c
>>>>>> @@ -31,12 +31,36 @@
>>>>>>     const double tolerance = 0.1;
>>>>>>     int fw_handle = -1;
>>>>>> +int fd_pci_usp;
>>>>> int fd_pci_usp = -1;
>>>>>
>>>>>> +bool rpm_disabled;
>>>>>> +bool link_states_saved;
>>>>>>     enum test_type {
>>>>>>     	TEST_S2IDLE,
>>>>>>     	TEST_IDLE,
>>>>>>     };
>>>>>> +enum link_state_index {
>>>>>> +	LINK_STATE_ASPM,
>>>>>> +	LINK_STATE_ASPM_L1_1,
>>>>>> +	LINK_STATE_ASPM_L1_2,
>>>>>> +	LINK_STATE_PCIPM_L1_1,
>>>>>> +	LINK_STATE_PCIPM_L1_2,
>>>>>> +	MAX_LINK_STATES,
>>>>>> +};
>>>>>> +
>>>>>> +struct link_state_info {
>>>>>> +	const char *filename;
>>>>>> +	char state;
>>>>>> +	const char *parse_str;
>>>>>> +} link_state_sysfs[] = {
>>>>>> +	{ "l1_aspm", 0, "PCIE LINK L1 RESIDENCY : "},
>>>>>> +	{ "l1_1_aspm", 0, "NULL"},
>>>>>> +	{ "l1_2_aspm", 0, "PCIE LINK L1.2 RESIDENCY : "},
>>>>>> +	{ "l1_1_pcipm", 0, NULL},
>>>>>> +	{ "l1_2_pcipm", 0, NULL}, +}; + /** * SUBTEST: gt-c6-on-idle * Description: 
>>>>>> Validate GT C6 state on idle @@ -64,6 +88,10 @@ enum test_type { 
>>>>>> * SUBTEST: cpg-gt-toggle * Description: Toggle GT coarse power 
>>>>>> gating states by acquiring/releasing * forcewake. + * + * 
>>>>>> SUBTEST: aspm_link_residency + * Description: Check for PCIe ASPM 
>>>>>> (Active State Power Management) link states + * entry while 
>>>>>> device is in D0. */ IGT_TEST_DESCRIPTION("Tests for gtidle
>>>>>> properties");
>>>>>> @@ -255,6 +283,21 @@ static void idle_residency_on_exec(int fd, struct drm_xe_engine_class_instance *
>>>>>>     	munmap(done, 4096);
>>>>>>     }
>>>>>> +static void do_spin(int fd, struct drm_xe_engine_class_instance *eci)
>>>>>> +{
>>>>>> +	igt_spin_t *spin;
>>>>>> +	uint64_t vm, ahnd;
>>>>>> +
>>>>>> +	igt_info("Running spinner on %s:%d\n",
>>>>>> +		 xe_engine_class_string(eci->engine_class), eci->engine_instance);
>>>>>> +	vm = xe_vm_create(fd, 0, 0);
>>>>>> +	intel_allocator_init();
>>>>>> +	ahnd = intel_allocator_open(fd, 0, INTEL_ALLOCATOR_RELOC);
>>>>>> +	spin = igt_spin_new(fd, .ahnd = ahnd, .vm = vm, .hwe = eci);
>>>>>> +	igt_measured_usleep(USEC_PER_SEC);
>>>>>> +	igt_spin_free(fd, spin);
>>>>>> +}
>>>>>> +
>>>>>>     static void measure_power(struct igt_power *gpu, double *power)
>>>>>>     {
>>>>>>     	struct power_sample power_sample[2];
>>>>>> @@ -370,6 +413,143 @@ static void cpg_gt_toggle(int fd)
>>>>>>     		powergate_status(fd, gt, "down");
>>>>>>     }
>>>>>> +static uint64_t get_link_state_residency(int fd_xe, const char *parse_str)
>>>>>> +{
>>>>>> +	int fd_debugfs_dir = 0;
>>>>>> +	int ret = 0;
>>>>>> +	char *ptr = NULL;
>>>>>> +	char buf[1024] = {0};
>>>>>> +	uint64_t residency = 0;
>>>>>> +
>>>>>> +	fd_debugfs_dir = igt_debugfs_dir(fd_xe);
>>>>>> +	igt_assert(fd_debugfs_dir >= 0);
>>>>>> +	ret = igt_debugfs_simple_read(fd_debugfs_dir, "dgfx_pcie_link_residencies", buf,
>>>>>> +				      sizeof(buf));
>>>>>> +	igt_assert_f(ret >= 0, "Cannot read residency file dgfx_pcie_link_residencies, ret %d\n",
>>>>>> +		     ret);
>>>>>> +
>>>>>> +	ptr = strstr(buf, parse_str);
>>>>>> +	igt_assert_f(ptr, "Cannot find residency string %s\n", parse_str);
>>>>>> +	ret = sscanf(ptr + strlen(parse_str), "%lu", &residency);
>>>>>> +	igt_assert_f(ret > 0, "Couldn't read residency value, ret %d", ret);
>>>>>> +	igt_info("Link residency %"PRIu64"\n", residency);
>>>>>> +	close(fd_debugfs_dir);
>>>>>> +
>>>>>> +	return residency;
>>>>>> +}
>>>>>> +
>>>>>> +static void save_and_disable_link_states(void)
>>>>>> +{
>>>>>> +	int i = 0;
>>>>>> +	int ret = 0;
>>>>>> +	char path[256] = {0};
>>>>>> +
>>>>>> +	for (i = 0 ; i < MAX_LINK_STATES ; i++) {
>>>>>> +		sprintf(path, "%s", link_state_sysfs[i].filename);
>>>>>> +		if (!igt_sysfs_has_attr(fd_pci_usp, path))
>>>>>> +			continue;
>>>>>> +		ret = igt_sysfs_scanf(fd_pci_usp, path, "%c", &link_state_sysfs[i].state);
>>>>>> +		igt_assert_lt(0, ret);
>>>>>> +		igt_debug("saved %s = %c\n", link_state_sysfs[i].filename,
>>>>>> +			  link_state_sysfs[i].state);
>>>>>> +		ret = igt_sysfs_printf(fd_pci_usp, path, "%c", '0');
>>>>> Disable should be done in separate function, so there should be two:
>>>>>
>>>>> static void save_link_states(void)
>>>>> static void disable_link_states(void)
>>>> any reason for this Kamil, it will be unnecessary extra function.
>>> Because you mix here reading and setting. imho there could be
>>> a problem if setting first few succeeds but in middle fails.
>> ok
> Hmm, other way could be to read and set, and if any error
> occurs break loop, do rollback, and only after that make
> an assert.
okay
>
>>>
>>>>>> +		igt_assert_lt(0, ret);
>>>>>> +	}
>>>>>> +	link_states_saved = true;
>>>>>> +}
>>>>>> +
>>>>>> +static void restore_link_states(void)
>>>>>> +{
>>>>>> +	int i = 0;
>>>>>> +	int ret = 0;
>>>>>> +	char path[256] = {0};
>>>>>> +
>>>>>> +	if (link_states_saved && fd_pci_usp >= 0) {
>>>>>> +		/* Restore saved states of L1 sysfs entries. */
>>>>>> +		for (i = 0 ; i < MAX_LINK_STATES ; i++) {
>>>>>> +			sprintf(path, "%s", link_state_sysfs[i].filename);
>>>>>> +			if (!igt_sysfs_has_attr(fd_pci_usp, path))
>>>>>> +				continue;
>>>>>> +			ret = igt_sysfs_printf(fd_pci_usp, path, "%c", link_state_sysfs[i].state);
>>>>>> +			igt_assert_lt(0, ret);
>>>>>> +			igt_debug("restored %s to %c\n", link_state_sysfs[i].filename,
>>>>>> +				  link_state_sysfs[i].state);
>>>>>> +		}
>>>>>> +		link_states_saved = false;
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>> +static void test_aspm_link_residency(int fd_xe, enum link_state_index aspm_link_state)
>>>>>> +{
>>>>>> +	struct pci_device *pci_dev;
>>>>>> +	char name[PATH_MAX];
>>>>>> +	int ret = 0;
>>>>>> +	uint64_t residency_pre = 0, residency_post = 0;
>>>>>> +
>>>>>> +	igt_assert(aspm_link_state <= LINK_STATE_ASPM_L1_2);
>>>>>> +
>>>>>> +	/* Get upstream port pci_dev */
>>>>>> +	pci_dev = igt_device_get_pci_upstream_port(fd_xe);
>>>>>> +	igt_assert_f(pci_dev, "Couldn't get pci device of upstream port\n");
>>>>>> +	igt_debug("Upstream port PCI device: %04x:%02x:%02x.%01x\n", pci_dev->domain,
>>>>>> +		  pci_dev->bus, pci_dev->dev, pci_dev->func);
>>>>>> +
>>>>>> +	snprintf(name, sizeof(name), "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/link",
>>>>>> +		 pci_dev->domain, pci_dev->bus, pci_dev->dev, pci_dev->func);
>>>>>> +	fd_pci_usp = open(name, O_DIRECTORY);
>>>>>> +	igt_assert_f((fd_pci_usp >= 0), "Can't open link directory upstream port %s, ret %d\n",
>>>>>> +		     name, fd_pci_usp);
>>>>>> +
>>>>>> +	/* Disable runtime PM as link ASPM entry happens during device is in D0 only. */
>>>>>> +	igt_assert(igt_setup_runtime_pm(fd_xe));
>>>>>> +	igt_disable_runtime_pm();
>>>>>> +	rpm_disabled = true;
>>>>>> +
>>>>>> +	/* Check if ASPM sysfs is present. */
>>>>>> +	igt_require_f(igt_sysfs_has_attr(fd_pci_usp, link_state_sysfs[aspm_link_state].filename),
>>>>>> +		      "%s is not present\n", link_state_sysfs[aspm_link_state].filename);
>>>>>> +	ret = igt_sysfs_scanf(fd_pci_usp, link_state_sysfs[aspm_link_state].filename, "%c",
>>>>>> +			      &link_state_sysfs[aspm_link_state].state);
>>>>>> +	igt_assert_f((ret > 0), "Couldn't read residency for %s",
>>>>>> +		     link_state_sysfs[aspm_link_state].filename);
>>>>>> +
>>>>>> +	/* Save current state of all available link sysfs entries and disable all link states. */
>>>>>> +	save_and_disable_link_states();
>>>>>> +
>>>>>> +	/* Enable only the ASPM link state needed for test. */
>>>>>> +	igt_debug("Enabling %s\n", link_state_sysfs[aspm_link_state].filename);
>>>>>> +	ret = igt_sysfs_printf(fd_pci_usp, link_state_sysfs[aspm_link_state].filename, "%c", '1');
>>>>>> +
>>>>>> +	/* Read link state residencies before and after idle wait time. */
>>>>>> +	residency_pre = get_link_state_residency(fd_xe,
>>>>>> +						 link_state_sysfs[aspm_link_state].parse_str);
>>>>>> +	igt_info("Waiting for link to enter idle....\n");
>>>>>> +	sleep(SLEEP_DURATION);
>>>>>> +	residency_post = get_link_state_residency(fd_xe,
>>>>>> +						  link_state_sysfs[aspm_link_state].parse_str);
>>>>>> +
>>>>>> +	restore_link_states();
>>>>>> +	igt_restore_runtime_pm();
>>>>>> +	rpm_disabled = false;
>>>>>> +	close(fd_pci_usp);
>>>>>> +	close(fd_xe);
>>>>>> +
>>>>>> +	igt_assert_f(residency_post > residency_pre,
>>>>>> +		     "ASPM entry failed, pre %"PRIu64", post %"PRIu64"\n", residency_pre,
>>>>>> +		     residency_post);
>>>>>> +}
>>>>>> +
>>>>>> +static void aspm_residency_exit_handler(int sig)
>>>>>> +{
>>>>>> +	restore_link_states();
>>>>>> +	if (rpm_disabled) {
>>>>>> +		igt_restore_runtime_pm();
>>>>>> +		rpm_disabled = false;
>>>>>> +	}
>>>>>> +	if (fd_pci_usp)
>>>>> 	if (fd_pci_usp != -1)
>>>>>
>>>>>> +		close(fd_pci_usp);
>>>>> Add:
>>>>> 	fd_pci_usp = -1;
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>     int igt_main()
>>>>>>     {
>>>>>>     	uint32_t d3cold_allowed;
>>>>>> @@ -444,6 +624,22 @@ int igt_main()
>>>>>>     		cpg_gt_toggle(fd);
>>>>>>     	}
>>>>>> +	igt_describe("ASPM Link residency validation");
>>>>>> +	igt_subtest_with_dynamic("aspm_link_residency") {
>>>>> This should be igt_subtest():
>>>>>
>>>>> 	igt_subtest("aspm_link_residency") {
>>>>>
>>>>>> +		igt_require(xe_has_vram(fd));
>>>>>> +		xe_for_each_gt(fd, gt) {
>>>>>> +			xe_for_each_engine(fd, hwe) {
>>>>>> +				if (gt == hwe->gt_id && !hwe->engine_instance) {
>>>>>> +					igt_dynamic_f("gt%u-engine-%s", gt,
>>>>>> +						      xe_engine_class_string(hwe->engine_class))
>>>>> What are you testing here? If you need to start and stop spinner
>>>>> please describe here why. If you really need it, it should be:
>>>> we want to run a basic workload before testing ASPM entry.
>>>>
>>>> So we are using spinner as workload here.
>>> ok, but why do you need any workload? It should also work without it?
>> Yes, it works even workload also.
>>
>> This was suggested by Anshuman,
>>
>> I believe this is intended to wake up the GPU and ensure the engines execute
>> some workload before entering ASPM.
>>
> So please add a comment before xe_for_each_gt()
> why it is needed.
okay
>
> Regards,
> Kamil
>
>>>>> 				if (gt == hwe->gt_id && !hwe->engine_instance)
>>>>> 						do_spin(fd, hwe);
>>>>>
>>>>>> +						do_spin(fd, hwe);
>>>>>> +				}
>>>>>> +			}
>>>>>> +		}
>>>>> Add newline.
>>>>>
>>>>>> +		igt_install_exit_handler(aspm_residency_exit_handler);
>>>>>> +		test_aspm_link_residency(fd, LINK_STATE_ASPM);
>>>>> Btw any igt_assert outside of dynamic sub-subtest here will result
>>>>> in SIGABORT, not a proper test fail.
>>>>>
>>>>> Add cleanup here:
>>>>> 		aspm_residency_exit_handler(0);
>>>> but installing exit handler already does this !
>>> It could become a problem when someone adds new test after this one.

we don't need to call aspm_residency_exit_handler(), at the end of test 
in pass scenario,

as test_aspm_link_residency() at its end already does the cleanup same 
as aspm_residency_exit_handler()

>>>
>>> Regards,
>>> Kamil
>>>
>>>>>> +	}
>>>>>> +
>>>>>>     	igt_fixture() {
>>>>>>     		close(fd);
>>>>>>     	}
>>>>>> -- 
>>>>>> 2.25.1
>>>>>>

[-- Attachment #2: Type: text/html, Size: 17173 bytes --]