[igt-dev] [PATCH i-g-t v2 1/3] tests/xe_pm: Add d3-mmap IGT test

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Friday, September 22, 2023 7:53 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Dixit, Ashutosh
> <ashutosh.dixit at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> Tauro, Riana <riana.tauro at intel.com>; Sundaresan, Sujaritha
> <sujaritha.sundaresan at intel.com>
> Subject: Re: [PATCH i-g-t v2 1/3] tests/xe_pm: Add d3-mmap IGT test
> 
> On Fri, Sep 22, 2023 at 06:53:59PM +0530, Anshuman Gupta wrote:
> > Adding a test to validate mmap memory mappings along with runtime
> > suspend and resume for both xe device and it's pci parent bridge in
> > device hierarchy.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> >  tests/intel/xe_pm.c | 94
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >
> > diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c index
> > fd28d5630..5495da154 100644
> > --- a/tests/intel/xe_pm.c
> > +++ b/tests/intel/xe_pm.c
> > @@ -39,6 +39,7 @@ typedef struct {
> >  } device_t;
> >
> >  uint64_t orig_threshold;
> > +int fw_handle = -1;
> >
> >  /* runtime_usage is only available if kernel build
> > CONFIG_PM_ADVANCED_DEBUG */  static bool
> > runtime_usage_available(struct pci_device *pci) @@ -166,6 +167,14 @@
> static bool out_of_d3(device_t device, enum igt_acpi_d_state state)
> >  	return true;
> >  }
> >
> > +static void close_fw_handle(int sig)
> > +{
> > +	if (fw_handle < 0)
> > +		return;
> > +
> > +	close(fw_handle);
> > +}
> 
> it took me a while to understand why global fw_handle here and the exit
> handler install at the end and then the conversion of the second patch. It
> makes total sense to ensure that we don't leave the fw awake after a test
> issue.
> But could you please do this in a separated patch before the addition of this
> new test?
Sure I will do that.
> 
> > +
> >  /**
> >   * SUBTEST: %s-basic
> >   * Description: set GPU state to %arg[1] and test suspend/autoresume
> > @@ -437,6 +446,68 @@ static void test_vram_d3cold_threshold(device_t
> device, int sysfs_fd)
> >  	igt_assert(in_d3(device, IGT_ACPI_D3Cold));  }
> >
> > +/**
> > + * SUBTEST: d3-mmap-%s
> > + * Description:
> > + *	Validate mmap memory mapping with d3 state, for %arg[1] region,
> > + *	if supported by device.
> > + * arg[1]:
> > + *
> > + * @vram:	vram region
> > + * @system:	system region
> > + *
> > + * Functionality: pm-d3
> > + * Run type: FULL
> > + */
> > +static void test_mmap(device_t device, uint32_t flags) {
> > +	size_t bo_size = 8192;
> > +	uint8_t *map = NULL;
> > +	uint32_t bo;
> > +	int i;
> > +
> > +	igt_require_f(flags, "Device doesn't support such memory region\n");
> > +
> > +	bo = xe_bo_create_flags(device.fd_xe, 0, bo_size, flags);
> > +	map = xe_bo_map(device.fd_xe, bo, bo_size);
> > +	igt_assert(map);
> > +
> > +	fw_handle = igt_debugfs_open(device.fd_xe, "forcewake_all",
> O_RDONLY);
> > +	igt_assert(fw_handle >= 0);
> > +	igt_assert(igt_get_runtime_pm_status() ==
> > +IGT_RUNTIME_PM_STATUS_ACTIVE);
> > +
> > +	for (i = 0; i < bo_size; i++)
> > +		map[i] = i & 0xFF;
> > +
> > +	for (i = 0; i < bo_size; i++)
> > +		igt_assert(map[i] == (i & 0xFF));
> > +
> > +	/* Runtime suspend and validate the pattern and clear the pattern */
> > +	close(fw_handle);
> > +
> 	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSP
> ENDED));
> > +
> > +	for (i = 0; i < bo_size; i++)
> > +		igt_assert(map[i] == (i & 0xFF));
> > +
> > +
> 	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSP
> ENDED));
> > +
> > +	for (i = 0; i < bo_size; i++)
> > +		map[i] = (~i & 0xFF);
> > +
> > +
> 	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSP
> ENDED));
> > +
> > +	/* Runtime resume and check the pattern */
> > +	fw_handle = igt_debugfs_open(device.fd_xe, "forcewake_all",
> O_RDONLY);
> > +	igt_assert(fw_handle >= 0);
> > +	igt_assert(igt_get_runtime_pm_status() ==
> > +IGT_RUNTIME_PM_STATUS_ACTIVE);
> 
> You are using the forcewake to wake the gpu up, but  I wonder if what we
> want here is to also give a time to get to the suspend, then assert SUSPENDED,
> and then do the read to the mapped area and ensure that we are ACTIVE
> because of the read?
> I mean, with this we would be really testing the Badal's case of exit on page
> fault, right?
This test is just validating the pattern in the bo during runtime suspend and resume, this test is agnostic for vram region and system region.
Bo. 
To validate the page fault case, we would need a test specific to vram region. I will do that change. 

Thanks,
Anshuman.
  
> 
> > +	for (i = 0; i < bo_size; i++)
> > +		igt_assert(map[i] == (~i & 0xFF));
> 
> should we have some kind of 0xc0ffee check instead?
> I'm afraid of some cases that we might read garbage 1s and be okay with it.
> 
> > +
> > +	igt_assert(munmap(map, bo_size) == 0);
> > +	gem_close(device.fd_xe, bo);
> > +	close(fw_handle);
> > +}
> > +
> >  igt_main
> >  {
> >  	struct drm_xe_engine_class_instance *hwe; @@ -542,6 +613,29 @@
> > igt_main
> >  		test_vram_d3cold_threshold(device, sysfs_fd);
> >  	}
> >
> > +	igt_subtest_group {
> > +		igt_fixture {
> > +			igt_install_exit_handler(close_fw_handle);
> > +		}
> > +
> > +		igt_describe("Validate mmap memory mappings with system
> region,"
> > +			     "when device along with parent bridge in d3");
> > +		igt_subtest("d3-mmap-system") {
> > +			test_mmap(device, system_memory(device.fd_xe));
> > +		}
> > +
> > +		igt_describe("Validate mmap memory mappings with vram
> region,"
> > +			     "when device along with parent bridge in d3");
> > +		igt_subtest("d3-mmap-vram") {
> > +			if (device.pci_root != device.pci_xe) {
> > +
> 	igt_pm_enable_pci_card_runtime_pm(device.pci_root, NULL);
> > +
> 	igt_pm_set_d3cold_allowed(device.pci_slot_name, 1);
> > +			}
> > +
> > +			test_mmap(device,
> visible_vram_memory(device.fd_xe, 0));
> > +		}
> > +	}
> > +
> >  	igt_fixture {
> >  		close(sysfs_fd);
> >  		igt_pm_set_d3cold_allowed(device.pci_slot_name,
> d3cold_allowed);
> > --
> > 2.25.1
> >