[PATCH i-g-t 1/2] tests/intel/xe_mmap: Acquire rpm reference for pci-membarrier tests

[Kamil Konieczny]

Hi Kolanupaka,
On 2025-05-16 at 21:25:32 +0530, Kolanupaka Naveena wrote:
> During the test, the device is entering into runtime suspend, which is
> leading the device to d3hot or d3cold mode. Hence the read is giving
> a special value 0xffffffff.
> 
> To avoid this issue acquired an rpm ref to keep the device awake
> during the access.
> 
> Also created a sub-group for all the pci-membarrier tests to avoid the
> multiple open/close calls to rpm ref.
> 
> Suggested-by: Matthew Auld <matthew.auld at intel.com>
> Signed-off-by: Kolanupaka Naveena <kolanupaka.naveena at intel.com>
> ---
>  tests/intel/xe_mmap.c | 64 +++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/intel/xe_mmap.c b/tests/intel/xe_mmap.c
> index 5fd641075..9421b8503 100644
> --- a/tests/intel/xe_mmap.c
> +++ b/tests/intel/xe_mmap.c
> @@ -11,6 +11,8 @@
>   * Functionality: mmap
>   */
>  
> +#include <fcntl.h>
> +
>  #include "igt.h"
>  
>  #include "xe_drm.h"
> @@ -419,31 +421,43 @@ igt_main
>  		test_mmap(fd, vram_memory(fd, 0) | system_memory(fd),
>  			  DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>  
> -	igt_subtest("pci-membarrier") {
> -		igt_require(is_pci_membarrier_supported(fd));
> -		test_pci_membarrier(fd);
> -	}
> +	igt_subtest_group {
> +		int fw_handle = -1;
>  
> -	igt_subtest("pci-membarrier-parallel") {
> -		int xe;
> -		unsigned int i;
> -		uint32_t *ptr;
> -
> -		igt_require(is_pci_membarrier_supported(fd));
> -		xe = drm_open_driver(DRIVER_XE);
> -		srand(time(0));
> -		i = rand() % (PAGE_SIZE / sizeof(*ptr));
> -		igt_fork(child, 1)
> -			test_pci_membarrier_parallel(xe, child, i);
> -		test_pci_membarrier_parallel(fd, -1, i);
> -		igt_waitchildren();
> -
> -		close(xe);
> -	}
> +		igt_fixture {
> +			igt_require(is_pci_membarrier_supported(fd));

This require should be placed at each subtest.

> +			fw_handle = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
> +			igt_assert_lte(0, fw_handle);

Please no asserts outside of subtest. Imho both open of fw_handle
and assert should be done in a function, and called at each subtest
which needs them, for example:

void open_forcewake(int fd, int *fw_ptr)
{
	if(fw_ptr > 0)
		return;

	igt_require(is_pci_membarrier_supported(fd));
	*fw_ptr = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
	igt_assert_lte(0, *fw_ptr);
}

and use it at each subtest:
		igt_subtest("pci-membarrier") {
			open_forcewake(fd, &fw_handle);
			test_pci_membarrier(fd);
		}

Btw you could name it as other name, like prepare_pci_membarrier_test()
as this is doing two things, not one.

> +		}
> +
> +		igt_subtest("pci-membarrier")
> +			test_pci_membarrier(fd);
> +
> +		igt_subtest("pci-membarrier-parallel") {
> +			int xe;
> +			unsigned int i;
> +			uint32_t *ptr;
> +
> +			xe = drm_open_driver(DRIVER_XE);

This is new code, so
xe = drm_reopen_driver(fd);

> +			srand(time(0));

Drop srand() from here.

> +			i = rand() % (PAGE_SIZE / sizeof(*ptr));
> +			igt_fork(child, 1)
> +				test_pci_membarrier_parallel(xe, child, i);
> +			test_pci_membarrier_parallel(fd, -1, i);
> +			igt_waitchildren();
>  
> -	igt_subtest("pci-membarrier-bad-pagesize") {
> -		igt_require(is_pci_membarrier_supported(fd));
> -		test_bad_pagesize_for_pcimem(fd);
> +			close(xe);

drm_close_driver(fd);

When you are doing code refactoring please make it clean
from the start.

> +		}
> +
> +		igt_subtest("pci-membarrier-bad-pagesize")

Add open_forcewake(fd, &fw_handle);

> +			test_bad_pagesize_for_pcimem(fd);
> +
> +		igt_subtest("pci-membarrier-bad-object")

Add open_forcewake(fd, &fw_handle);

Regards,
Kamil

> +			test_bad_object_for_pcimem(fd);
> +
> +		igt_fixture {
> +			close(fw_handle);
> +		}
>  	}
>  
>  	igt_subtest("bad-flags")
> @@ -455,10 +469,6 @@ igt_main
>  	igt_subtest("bad-object")
>  		test_bad_object(fd);
>  
> -	igt_subtest("pci-membarrier-bad-object") {
> -		igt_require(is_pci_membarrier_supported(fd));
> -		test_bad_object_for_pcimem(fd);
> -	}
>  
>  	igt_subtest("small-bar") {
>  		igt_require(xe_visible_vram_size(fd, 0));
> -- 
> 2.34.1
>