[PATCH i-g-t] xe: Add test to check pci memory barrier capability

[Upadhyay, Tejas]

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Wednesday, October 9, 2024 11:00 PM
> To: igt-dev at lists.freedesktop.org
> Cc: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> gfx at lists.freedesktop.org; Piecielska, Katarzyna
> <katarzyna.piecielska at intel.com>
> Subject: Re: [PATCH i-g-t] xe: Add test to check pci memory barrier capability
> 
> Hi Tejas,
> On 2024-10-09 at 15:26:08 +0530, Tejas Upadhyay wrote:
> 
> one more nit, imho a patch with new test should have in subject
> 
> tests/intel: Add xe_pci_membarrier test

Ok. 

> 
> Also see nit about a test name.
> 
> > 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 at intel.com>
> > ---
> >  include/drm-uapi/xe_drm.h       |  3 ++
> >  tests/intel/xe_pci_membarrier.c | 77
> +++++++++++++++++++++++++++++++++
> >  tests/meson.build               |  1 +
> >  3 files changed, 81 insertions(+)
> >  create mode 100644 tests/intel/xe_pci_membarrier.c
> >
> > diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h
> > index f0a450db9..866dc8060 100644
> > --- a/include/drm-uapi/xe_drm.h
> > +++ b/include/drm-uapi/xe_drm.h
> > @@ -823,6 +823,9 @@ struct drm_xe_gem_mmap_offset {
> >  	/** @offset: The fake offset to use for subsequent mmap call */
> >  	__u64 offset;
> >
> > +	/* Specific MMAP offset for PCI memory barrier */ #define
> > +DRM_XE_PCI_BARRIER_MMAP_OFFSET (0x50 << PAGE_SHIFT)
> > +
> >  	/** @reserved: Reserved */
> >  	__u64 reserved[2];
> >  };
> > diff --git a/tests/intel/xe_pci_membarrier.c
> > b/tests/intel/xe_pci_membarrier.c new file mode 100644 index
> > 000000000..a28a01d4b
> > --- /dev/null
> > +++ b/tests/intel/xe_pci_membarrier.c
> > @@ -0,0 +1,77 @@
> > +// 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
> ------^^^^^^^^^^^^^
> Not sure about this, +cc Katarzyna
> Cc: Katarzyna Piecielska <katarzyna.piecielska at intel.com>
> 
> > + */
> > +
> > +IGT_TEST_DESCRIPTION("Basic MMAP tests pci memory barrier effect with
> > +special offset"); #define PAGE_SHIFT 12 #define PAGE_SIZE 4096
> > +
> > +/**
> > + * SUBTEST: pci-membarrier-basic
> 
> Why not just basic?

Fine to me to change to 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;
> > +	uint64_t offset = DRM_XE_PCI_BARRIER_MMAP_OFFSET;
> > +	unsigned int prot = PROT_WRITE;
> > +	uint32_t *ptr;
> > +	uint64_t size = PAGE_SIZE;
> > +	struct timespec tv;
> > +
> > +	ptr = mmap(NULL, size, prot, flags, xe, 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)
> > +				;
> 
> What if this while never break?
> imho use igt_until_timeout around 'for' loop.

This is write on mmio via mapped space. It is expected that value will be hold for while before it gets invalidated or flushed. There will not be ever case that value will be hold forever. So this looks ok without timeout.

> 
> > +			igt_info("fd:%d value retained for %"PRId64"ns
> pos:%d\n",
> > +				 xe, igt_nsec_elapsed(&tv), i);
> 
> igt_debug is preferred unless this print should never happen, but even then
> please limit number of prints.

As mentioned rarely we will see value getting hold for while, where this print might come. So far never seen this debug.

> 
> > +		}
> > +		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("pci-membarrier-basic")
> > +		pci_membarrier(xe);
> > +
> > +	igt_fixture {
> > +		close(xe);
> 
> drm_close_driver()

Yes thanks for catching this, I will correct it.

Tejas
> 
> Regards,
> Kamil
> 
> 
> > +	}
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build index
> > e5d8852f3..310ef0e0d 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -304,6 +304,7 @@ intel_xe_progs = [
> >  	'xe_noexec_ping_pong',
> >  	'xe_oa',
> >  	'xe_pat',
> > +        'xe_pci_membarrier',
> >  	'xe_peer2peer',
> >  	'xe_pm',
> >  	'xe_pm_residency',
> > --
> > 2.34.1
> >