> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny@xxxxxxxxxxxxxxx>
> Sent: Wednesday, October 9, 2024 11:00 PM
> To: igt-dev@xxxxxxxxxxxxxxxxxxxxx
> Cc: Upadhyay, Tejas <tejas.upadhyay@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; Piecielska, Katarzyna
> <katarzyna.piecielska@xxxxxxxxx>
> 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
>
> Also see nit about a test name.
Sure
>
> > 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> > + */
> > +
> > +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?
Fair
>
> > + * 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.
Value should never be hold here as it is expected that all writes here will be dropped immediately. So this debug below will never be executed and while will break immediately. In testing not seen even once, but just kept in case we hit on any coming platform or in any corner case. So we should be fine as it is now.
>
> > + 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.
>
> > + }
> > + 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()
Will change, thanks.
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
> >
