Re: [PATCH i-g-t 4/5] tests/intel/xe_pm: Only check the rpm resume after the first mmap operation

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 14-05-2024 00:25, Rodrigo Vivi wrote:
> The very first memory operation on a mmaped region after runtime suspend,
> the device will be resumed if the memory used is vram.
> However, after this first operation with page fault, the memory
> can be migrated to the system memory.
> 
> During this migration, Xe kernel will get a notification at
> xe_bo_move_notify, and the bo will be removed from the
> vram_userfault list. Then, on the next suspend, we won't mark
> bo for page fault and resume won't happen and the active count
> won't increase. This is okay, since the bo is now in the system
> memory we don't need to wake up the device. But the current
> test is wrong, because it assumes that the bo keeps forever
> in vram, what is the issue here.
> 
> Only checking the resume after the first operation is the
> enough. But also, ensure that both read and write operations
> are actually validated.
> 
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>   tests/intel/xe_pm.c | 36 +++++++++++++++++++++++++-----------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> index 72d484163..81226a910 100644
> --- a/tests/intel/xe_pm.c
> +++ b/tests/intel/xe_pm.c
> @@ -37,6 +37,11 @@
>   #define PREFETCH (0x1 << 1)
>   #define UNBIND_ALL (0x1 << 2)
>   
> +enum mem_op {
> +	READ,
> +	WRITE,
> +};
> +
>   typedef struct {
>   	int fd_xe;
>   	struct pci_device *pci_xe;
> @@ -525,7 +530,8 @@ static void test_vram_d3cold_threshold(device_t device, int sysfs_fd)
>    *
>    * Functionality: pm-d3
>    */
> -static void test_mmap(device_t device, uint32_t placement, uint32_t flags)
> +static void test_mmap(device_t device, uint32_t placement, uint32_t flags,
> +		      enum mem_op first_op)
>   {
>   	size_t bo_size = 8192;
>   	uint32_t *map = NULL;
> @@ -563,8 +569,12 @@ static void test_mmap(device_t device, uint32_t placement, uint32_t flags)
>   	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>   	active_time = igt_pm_get_runtime_active_time(device.pci_xe);
>   
> -	for (i = 0; i < bo_size / sizeof(*map); i++)
> -		igt_assert(map[i] == MAGIC_1);
> +	for (i = 0; i < bo_size / sizeof(*map); i++) {
> +		if (first_op == READ)
> +			igt_assert(map[i] == MAGIC_1);
> +		else
> +			map[i] = MAGIC_2;
> +	}
>   
>   	/* dgfx page-fault on mmaping should wake the gpu */
>   	if (xe_has_vram(device.fd_xe) && flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
> @@ -574,12 +584,12 @@ static void test_mmap(device_t device, uint32_t placement, uint32_t flags)
>   	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>   	active_time = igt_pm_get_runtime_active_time(device.pci_xe);
Above line can be removed.
>   
> -	for (i = 0; i < bo_size / sizeof(*map); i++)
> -		map[i] = MAGIC_2;
> -
> -	if (xe_has_vram(device.fd_xe) && flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
> -		igt_assert(igt_pm_get_runtime_active_time(device.pci_xe) >
> -			   active_time);
> +	for (i = 0; i < bo_size / sizeof(*map); i++) {
> +		if (first_op == READ)
> +			map[i] = MAGIC_2;
> +		else
> +			igt_assert(map[i] == MAGIC_2);
> +	}
>   
>   	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>   
> @@ -781,7 +791,8 @@ igt_main
>   			     "when device along with parent bridge in d3");
>   		igt_subtest("d3-mmap-system") {
>   			dpms_on_off(device, DRM_MODE_DPMS_OFF);
> -			test_mmap(device, system_memory(device.fd_xe), 0);
> +			test_mmap(device, system_memory(device.fd_xe), 0, READ);
> +			test_mmap(device, system_memory(device.fd_xe), 0, WRITE);
>   			dpms_on_off(device, DRM_MODE_DPMS_ON);
>   		}
>   
> @@ -800,7 +811,10 @@ igt_main
>   			/* Give some auto suspend delay to validate rpm active during page fault */
>   			igt_pm_set_autosuspend_delay(device.pci_xe, 1000);
>   			dpms_on_off(device, DRM_MODE_DPMS_OFF);
> -			test_mmap(device, vram_memory(device.fd_xe, 0), DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
> +			test_mmap(device, vram_memory(device.fd_xe, 0),
> +				  DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM, READ);
> +			test_mmap(device, vram_memory(device.fd_xe, 0),
> +				  DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM, WRITE);With a comment this is
Reviewed-by: Badal Nilawar <badal.nilawar@intel.com>

Regards,
Badal
>   			dpms_on_off(device, DRM_MODE_DPMS_ON);
>   			igt_pm_set_autosuspend_delay(device.pci_xe, delay_ms);
>   		}