Re: [PATCH i-g-t v8] tests/intel/xe_pm: Fix d3 subtest

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 12.12.2023 19.46, Thasleem, Mohammed wrote:
> 
> 
> -----Original Message-----
> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Sent: Tuesday, December 12, 2023 5:54 PM
> To: Thasleem, Mohammed <mohammed.thasleem@intel.com>; igt-dev@lists.freedesktop.org
> Subject: Re: [PATCH i-g-t v8] tests/intel/xe_pm: Fix d3 subtest
> 
> Hi Mohammed,
> 
> On 11.12.2023 10.49, Mohammed Thasleem wrote:
>> xe_pm d3 test requires to turn off all display in order to enter to d3
>> state therefore, adding the support to triggering "DRM_MODE_DPMS_OFF"
>> in setup_d3() and "DRM_MODE_DPMS_ON" in cleanup_d3().
>>
>> v2: Use IGT_CRTC_ACTIVE for displays On/Off.
>> v3: -Update commit message and helper names.
>>       -Update display on and off helpers.
>> v4: Remove redundant code. (Bhanu)
>> v5: Move helper calls to test file.
>> v6: -Update function name as dpms_on_off. (Anshuman)
>>       -Update subject and commit message. (Kamil)
>> v7: -Update subjact. (Anshuman)
>>       -Move device.res to dpms_on_off(). (Bhanu)
>>       -Free device.res in igt_fixture. (Bhanu)
>>       -Call kmstest_set_connector_dpms if DRM_MODE_CONNECTED is found.
>> (Kamil)
>> v8: Update if condition to fix dereferencing of NULL pointer. (Bhanu)
>>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
>> ---
>>    tests/intel/xe_pm.c | 36 ++++++++++++++++++++++++++++++++++++
>>    1 file changed, 36 insertions(+)
>>
>> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c index
>> c899bd67a..3d6acfbab 100644
>> --- a/tests/intel/xe_pm.c
>> +++ b/tests/intel/xe_pm.c
>> @@ -16,6 +16,7 @@
>>    
>>    #include "igt.h"
>>    #include "lib/igt_device.h"
>> +#include "lib/igt_kms.h"
> 
> Is this include needed?
> 
>>    #include "lib/igt_pm.h"
>>    #include "lib/igt_sysfs.h"
>>    #include "lib/igt_syncobj.h"
>> @@ -36,10 +37,32 @@ typedef struct {
>>    	struct pci_device *pci_xe;
>>    	struct pci_device *pci_root;
>>    	char pci_slot_name[NAME_MAX];
>> +	drmModeResPtr res;
>>    } device_t;
>>    
>>    uint64_t orig_threshold;
>>    
>> +static void dpms_on_off(device_t device, int mode) {
>> +	int i;
>> +
>> +	if (!device.res)
>> +		return;
> 
> When commit message said these dpms setting are required for the test, should this device.res be decorated here with igt_require() instead of checking against null?
> -->Here we exclude following code to run if device.res not found but not to skip the test because this test expected to run more on headless scenarios.
>       In case, if we found device.res then turn off the displays using below loop to run the test and set it back to turn on the displays using dpms_on_off once test gets executed.

ack.

With those couple of other nitpicks fixed you can add

Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

>> +
>> +	for (i = 0; i < device.res->count_connectors; i++) {
>> +		drmModeConnector *connector = drmModeGetConnectorCurrent(device.fd_xe,
>> +					      device.res->connectors[i]);
>                                                 ^ indentation
> 
> Other than these couple of minor comments, things look ok to me.
> 
> /Juha-Pekka
>> +
>> +		if (!connector)
>> +			continue;
>> +
>> +		if (connector->connection == DRM_MODE_CONNECTED)
>> +			kmstest_set_connector_dpms(device.fd_xe, connector, mode);
>> +
>> +		drmModeFreeConnector(connector);
>> +	}
>> +}
>> +
>>    /* runtime_usage is only available if kernel build CONFIG_PM_ADVANCED_DEBUG */
>>    static bool runtime_usage_available(struct pci_device *pci)
>>    {
>> @@ -94,6 +117,8 @@ static void vram_d3cold_threshold_restore(int sig)
>>    
>>    static bool setup_d3(device_t device, enum igt_acpi_d_state state)
>>    {
>> +	dpms_on_off(device, DRM_MODE_DPMS_OFF);
>> +
>>    	switch (state) {
>>    	case IGT_ACPI_D3Cold:
>>    		igt_require(igt_pm_acpi_d3cold_supported(device.pci_root));
>> @@ -110,6 +135,11 @@ static bool setup_d3(device_t device, enum igt_acpi_d_state state)
>>    	return false;
>>    }
>>    
>> +static void cleanup_d3(device_t device) {
>> +	dpms_on_off(device, DRM_MODE_DPMS_ON); }
>> +
>>    static bool in_d3(device_t device, enum igt_acpi_d_state state)
>>    {
>>    	uint16_t val;
>> @@ -477,6 +507,7 @@ igt_main
>>    		igt_pm_get_d3cold_allowed(device.pci_slot_name, &d3cold_allowed);
>>    		igt_assert(igt_setup_runtime_pm(device.fd_xe));
>>    		sysfs_fd = igt_sysfs_open(device.fd_xe);
>> +		device.res = drmModeGetResources(device.fd_xe);
>>    	}
>>    
>>    	for (const struct s_state *s = s_states; s->name; s++) { @@ -511,6
>> +542,7 @@ igt_main
>>    				xe_for_each_engine(device.fd_xe, hwe)
>>    					test_exec(device, hwe, 1, 2, s->state,
>>    						  NO_RPM);
>> +				cleanup_d3(device);
>>    			}
>>    		}
>>    	}
>> @@ -519,6 +551,7 @@ igt_main
>>    		igt_subtest_f("%s-basic", d->name) {
>>    			igt_assert(setup_d3(device, d->state));
>>    			igt_assert(in_d3(device, d->state));
>> +			cleanup_d3(device);
>>    		}
>>    
>>    		igt_subtest_f("%s-basic-exec", d->name) { @@ -526,6 +559,7 @@
>> igt_main
>>    			xe_for_each_engine(device.fd_xe, hwe)
>>    				test_exec(device, hwe, 1, 1,
>>    					  NO_SUSPEND, d->state);
>> +			cleanup_d3(device);
>>    		}
>>    
>>    		igt_subtest_f("%s-multiple-execs", d->name) { @@ -533,6 +567,7 @@
>> igt_main
>>    			xe_for_each_engine(device.fd_xe, hwe)
>>    				test_exec(device, hwe, 16, 32,
>>    					  NO_SUSPEND, d->state);
>> +			cleanup_d3(device);
>>    		}
>>    	}
>>    
>> @@ -547,6 +582,7 @@ igt_main
>>    		close(sysfs_fd);
>>    		igt_pm_set_d3cold_allowed(device.pci_slot_name, d3cold_allowed);
>>    		igt_restore_runtime_pm();
>> +		drmModeFreeResources(device.res);
>>    		drm_close_driver(device.fd_xe);
>>    	}
>>    }
>