Re: [PATCH i-g-t V2 2/2] tests/intel: Add xe_pci_membarrier test

[Matthew Auld <matthew.auld@intel.com>]

On 06/11/2024 13:12, Upadhyay, Tejas wrote:
> 
> 
>> -----Original Message-----
>> From: Upadhyay, Tejas
>> Sent: Wednesday, November 6, 2024 5:30 PM
>> To: Auld, Matthew <matthew.auld@intel.com>; igt-dev@lists.freedesktop.org;
>> intel-xe@lists.freedesktop.org
>> Subject: RE: [PATCH i-g-t V2 2/2] tests/intel: Add xe_pci_membarrier test
>>
>>
>>
>>> -----Original Message-----
>>> From: Auld, Matthew <matthew.auld@intel.com>
>>> Sent: Friday, November 1, 2024 7:25 PM
>>> To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; igt-
>>> dev@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>>> Subject: Re: [PATCH i-g-t V2 2/2] tests/intel: Add xe_pci_membarrier
>>> test
>>>
>>> On 23/10/2024 10:43, Tejas Upadhyay wrote:
>>>> We want to make sure that direct mmap mapping of physical page at
>>>> doorbell space and whole page is accessible in order to use pci
>>>> memory barrier effect effectively.
>>>>
>>>> This is basic pci memory barrier test to showcase xe driver support
>>>> for feature. In follow up patches we will have more of corner and
>>>> negative tests added later.
>>>>
>>>> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
>>>> ---
>>>>    tests/intel/xe_pci_membarrier.c | 80
>>>> +++++++++++++++++++++++++++++++++
>>>
>>> Could probably just plonk this in the existing mmap offset tests file?
>>
>> I tried this, but looks like none of existing methods which are using bo-create
>> looks related to this test. Also negative tests are also going to be different for
>> this test. I would think it should be good to keep this test separate. What you
>> think!
> 
> Or I can do atlest below :

Yes, somethng like that seems fine to me.

> 
> diff --git a/tests/intel/xe_mmap.c b/tests/intel/xe_mmap.c
> index fc5d73d59..8ae6405b5 100644
> --- a/tests/intel/xe_mmap.c
> +++ b/tests/intel/xe_mmap.c
> @@ -64,6 +64,54 @@ test_mmap(int fd, uint32_t placement, uint32_t flags)
>          gem_close(fd, bo);
>   }
> 
> +#define PAGE_SHIFT 12
> +#define PAGE_SIZE 4096
> +
> +/**
> + * SUBTEST: pci_membarrier
> + * Description: create pci memory barrier with write on defined mmap offset.
> + * Test category: functionality test
> + *
> + */
> +static void test_pci_membarrier(int xe)
> +{
> +        uint64_t flags = MAP_SHARED;
> +        unsigned int prot = PROT_WRITE;
> +        uint32_t *ptr;
> +        uint64_t size = PAGE_SIZE;
> +        struct timespec tv;
> +        struct drm_xe_gem_mmap_offset mmo = {
> +                .handle = 0,
> +                .flags = DRM_XE_MMAP_OFFSET_FLAG_PCI_BARRIER,
> +        };
> +
> +        igt_assert_eq(igt_ioctl(xe, DRM_IOCTL_XE_GEM_MMAP_OFFSET, &mmo), 0);
> +        ptr = mmap(NULL, size, prot, flags, xe, mmo.offset);
> +        igt_assert(ptr != MAP_FAILED);
> +
> +        /* Check whole page for any errors, also check as
> +         * we should not read written values back
> +         */
> +        for (int i = 0; i < size / sizeof(*ptr); i++) {
> +                /* It is expected unconfigured doorbell space
> +                 * will return read value 0xdeadbeef
> +                 */
> +                igt_assert_eq_u32(READ_ONCE(ptr[i]), 0xdeadbeef);
> +
> +                igt_gettime(&tv);
> +                ptr[i] = i;
> +                if (READ_ONCE(ptr[i]) == i) {
> +                        while (READ_ONCE(ptr[i]) == i)
> +                                ;
> +                        igt_info("fd:%d value retained for %"PRId64"ns pos:%d\n",
> +                                 xe, igt_nsec_elapsed(&tv), i);
> +                }
> +                igt_assert_neq(READ_ONCE(ptr[i]), i);
> +        }
> +
> +        munmap(ptr, size);
> +}
> +
>   /**
>    * SUBTEST: bad-flags
>    * Description: Test mmap offset with bad flags.
> @@ -273,6 +321,9 @@ igt_main
>                  test_mmap(fd, vram_memory(fd, 0) | system_memory(fd),
>                            DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
> 
> +       igt_subtest("pci-membarrier")
> +               test_pci_membarrier(fd);
> +
>          igt_subtest("bad-flags")
>                  test_bad_flags(fd);
> 
> diff --git a/tests/meson.build b/tests/meson.build
> index 2724c7a9a..0daed46da 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -306,6 +306,7 @@ intel_xe_progs = [
>          'xe_noexec_ping_pong',
>          'xe_oa',
>          'xe_pat',
> +        'xe_pci_membarrier',
> 
> 
>>
>> Tejas
>>
>>>
>>>>    tests/meson.build               |  1 +
>>>>    2 files changed, 81 insertions(+)
>>>>    create mode 100644 tests/intel/xe_pci_membarrier.c
>>>>
>>>> diff --git a/tests/intel/xe_pci_membarrier.c
>>>> b/tests/intel/xe_pci_membarrier.c new file mode 100644 index
>>>> 000000000..d0bf447b6
>>>> --- /dev/null
>>>> +++ b/tests/intel/xe_pci_membarrier.c
>>>> @@ -0,0 +1,80 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright(c) 2024 Intel Corporation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include "xe_drm.h"
>>>> +#include "igt.h"
>>>> +
>>>> +/**
>>>> + * TEST: Test if the driver is capable of putting pci memory
>>>> +barrier using mmap
>>>> + * Category: Core
>>>> + * Mega feature: General Core features
>>>> + * Sub-category: Memory management tests
>>>> + * Functionality: mmap with pre-defined offset  */
>>>> +
>>>> +IGT_TEST_DESCRIPTION("Basic MMAP tests pci memory barrier effect
>>>> +with special offset"); #define PAGE_SHIFT 12 #define PAGE_SIZE 4096
>>>> +
>>>> +/**
>>>> + * SUBTEST: basic
>>>> + * Description: create pci memory barrier with write on defined
>>>> +mmap
>>> offset.
>>>> + * Test category: functionality test
>>>> + *
>>>> + */
>>>> +
>>>> +static void pci_membarrier(int xe)
>>>> +{
>>>> +	uint64_t flags = MAP_SHARED;
>>>> +	unsigned int prot = PROT_WRITE;
>>>> +	uint32_t *ptr;
>>>> +	uint64_t size = PAGE_SIZE;
>>>> +	struct timespec tv;
>>>> +        struct drm_xe_gem_mmap_offset mmo = {
>>>> +                .handle = 0,
>>>> +                .flags = DRM_XE_MMAP_OFFSET_FLAG_PCI_BARRIER,
>>>> +        };
>>>> +
>>>> +        igt_assert_eq(igt_ioctl(xe, DRM_IOCTL_XE_GEM_MMAP_OFFSET,
>>> &mmo), 0);
>>>> +        ptr = mmap(NULL, size, prot, flags, xe, mmo.offset);
>>>> +	igt_assert(ptr != MAP_FAILED);
>>>
>>> nit: formatting
>>>
>>>> +
>>>> +	/* Check whole page for any errors, also check as
>>>> +	 * we should not read written values back
>>>> +	 */
>>>> +	for (int i = 0; i < size / sizeof(*ptr); i++) {
>>>> +		/* It is expected unconfigured doorbell space
>>>> +		 * will return read value 0xdeadbeef
>>>> +		 */
>>>> +		igt_assert_eq_u32(READ_ONCE(ptr[i]), 0xdeadbeef);
>>>> +
>>>> +		igt_gettime(&tv);
>>>> +		ptr[i] = i;
>>>> +		if (READ_ONCE(ptr[i]) == i) {
>>>
>>> Can this actually happen where the value is written?
>>>
>>> I think also consider adding some negative testcases. For example:
>>>
>>> - Try to mmap something larger than 4K. Ensure we get an error.
>>> - Try BARRIER mmap_offset, and also supply a BO. Ensure we get an error.
>>>
>>>> +			while (READ_ONCE(ptr[i]) == i)
>>>> +				;
>>>> +			igt_info("fd:%d value retained for %"PRId64"ns
>>> pos:%d\n",
>>>> +				 xe, igt_nsec_elapsed(&tv), i);
>>>> +		}
>>>> +		igt_assert_neq(READ_ONCE(ptr[i]), i);
>>>> +	}
>>>> +
>>>> +	munmap(ptr, size);
>>>> +}
>>>> +
>>>> +igt_main
>>>> +{
>>>> +	int xe;
>>>> +
>>>> +	igt_fixture {
>>>> +		xe = drm_open_driver(DRIVER_XE);
>>>> +	}
>>>> +
>>>> +	igt_subtest_f("basic")
>>>> +		pci_membarrier(xe);
>>>> +
>>>> +	igt_fixture
>>>> +		drm_close_driver(xe);
>>>> +}
>>>> diff --git a/tests/meson.build b/tests/meson.build index
>>>> 34b87b125..15131d812 100644
>>>> --- a/tests/meson.build
>>>> +++ b/tests/meson.build
>>>> @@ -306,6 +306,7 @@ intel_xe_progs = [
>>>>    	'xe_noexec_ping_pong',
>>>>    	'xe_oa',
>>>>    	'xe_pat',
>>>> +        'xe_pci_membarrier',
>>>>    	'xe_peer2peer',
>>>>    	'xe_pm',
>>>>    	'xe_pm_residency',