[PATCH i-g-t v3 00/10] tests/intel/xe_svm: Add tests for Shared Virtual Memory (SVM)

Wed May 22 16:53:05 UTC 2024

Hi Matt,

We will investigate your tests and see whether we can achieve the same coverage with our approach.

See also comments inline.

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, May 22, 2024 7:42 AM
> To: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>
> Cc: igt-dev at lists.freedesktop.org; Brost, Matthew
> <matthew.brost at intel.com>; intel-xe at lists.freedesktop.org; Zeng, Oak
> <oak.zeng at intel.com>
> Subject: Re: [PATCH i-g-t v3 00/10] tests/intel/xe_svm: Add tests for Shared
> Virtual Memory (SVM)
> 
> On Wed, May 22, 2024 at 11:38:18AM +0000, Matthew Brost wrote:
> > On Fri, May 17, 2024 at 05:16:48PM +0530, Bommu Krishnaiah wrote:
> > > Introduce helper functions for object creation, binding, submission,
> > > and destruction, applicable for SVM and other tests
> > >
> > > xe-basic test is validating the helper function introduced in 'lib/xe/xe_util:
> helper function'
> > >
> > > svm test cases:
> > > svm-basic-malloc
> > > svm-basic-mmap
> > > svm-random-access
> > > svm-huge-page
> > > svm-atomic-access
> > > svm-atomic-access
> > > svm_invalid_va
> > > svm-mprotect
> > > svm-benchmark
> > > svm-sparse-access
> > >
> >
> > There's a lot here I dislike. Let me be direct.
> >
> > Firstly, this isn't an SVM test; it explicitly binds userptrs and BOs. So using
> > SVM prefixes in the test names doesn't make sense. 

It is SVM test. Those test use BO for command (batch) buffer. But the buffer GPU read/write is malloc'ed or mmap'ed. So it does test SVM/system allocator functionality.

Malloc'ed and mmap'ed memory can also be used for command buffer. We can add a test case. 

And having RBs with
> those
> > names makes even less sense. With that; This email is directed at
> EVERYONE
> > replying to this series.
> >
> 
> Didn't send everyone, CCing interested parties.
> 
> > At a high level, I've expressed my views on generic helpers hindering the
> > ability to create truly powerful tests that provide the necessary coverage
> for a
> > complex driver. See my thoughts here [1] [2]. I don't see anything here
> that
> > comes close to addressing my concerns. 

I replied again to [1] with more details. Please take a look.

These tests are so simple they
> won't find
> > any bugs in the KMD beyond the implementation being completely broken.
> >

Agree those tests in this series are simple. But this is just a start. We plan to do more complex tests which are documented in https://jira.devtools.intel.com/browse/VLK-59767

We will also look at your series to see whether we can have the same coverage.

> > This is what I think an SVM test should look like [3] (for the record, I have
> > shared an eariler baseline of this code too). While it has some complex
> coding
> > patterns, once understood by a developer, it's quite easy to extend for a
> new
> > test case. Let me give an example...
> >
> > Take a look at this diff [4]. It's a few lines of code that add HUGE_TLB
> > testing. This simple change spawns over 100 tests of various varieties (e.g.,
> a
> > single exec with HUGE_TLB, two execs, many execs, multi-threaded
> versions,
> > multi-process versions, etc...).
> >
> > Before change:
> > ./build/tests/xe_exec_system_allocator --l | wc
> >     938     938   32872
> >
> > After change:
> > ./build/tests/xe_exec_system_allocator --l | wc
> >    1070    1070   37852
> >
> > Being extendable like this ultimately reduces maintenance costs over time.
> >
> > The test I've shared creates true coverage and is just a starting point. It will
> > need to become more complex over time as the SVM uAPI grows.
> >
> > I suggest investing the time to learn how the code I've shared works and
> develop
> > within that framework. If the same level of coverage/extendability can be
> > achieved with helpers, great. Anything less, in either area, I don't think is
> > acceptable.

There are two fundamental design difference b/t your approach and the approach we applied here:

1) Whether to create/use helper functions:

Our helper functions created concept of xe-buffer, xe-command-buffer. I still think with helper functions, we can simply test writing.

2) use one big test to test many functionalities vs many small tests, each test one functionality

It is obvious, our approach is much easier to maintain and understand.

So the key is, whether we can achieve the same coverage as your series. As said, we will take a closer look of your series and give it a try.

Oak

> >
> > Matt
> >
> > I can't find PW links for [1][2] so instead have shared the email thread
> subject
> > lines. Everyone on this email was on those chains as well.
> >
> > [1] [PATCH RFC i-g-t 0/2] helper function
> > [2] [PATCH] lib/xe/xe_util: Creating the helper functions
> > [3]
> https://patchwork.freedesktop.org/patch/594807/?series=133846&rev=1
> > [4] https://pastebin.com/jUN5BwUy
> >
> > > svm kernel implimentation:
> > > https://gitlab.freedesktop.org/oak/xe-kernel-driver-svm.git
> > > branch: origin/drm-xe-next-svm-unify-userptr
> > >
> > > v2 igt patch: https://patchwork.freedesktop.org/series/133096/
> > >
> > > Note: xe-basic test is validated without SVM. Remaining tests are not
> validated because the SVM driver code is still under development
> > >
> > > Bommu Krishnaiah (10):
> > >   lib/xe/xe_util: Introduce helper functions for buffer creation and
> > >     command submission etc
> > >   tests/intel/xe_svm: basic xe-basic test
> > >   tests/intel/xe_svm: Add SVM basic tests using malloc and mmap
> > >   tests/intel/xe_svm: add random access test for SVM
> > >   tests/intel/xe_svm: add huge page access test for SVM
> > >   tests/intel/xe_svm: Add support for GPU atomic access test for svm
> > >   tests/intel/xe_svm: Add svm-invalid-va test to verify SVM
> > >     functionality with invalid address access
> > >   tests/intel/xe_svm: Add svm-benchmark test to measure SVM
> performance
> > >     with a simple benchmark
> > >   tests/intel/xe_svm: Add svm-mprotect test to verify SVM functionality
> > >     with read-only memory access
> > >   tests/intel/xe_svm: Add svm-sparse-access test to verify sparsely
> > >     accessing two memory locations with SVM
> > >
> > >  include/drm-uapi/xe_drm.h |   1 +
> > >  lib/xe/xe_util.c          | 214 +++++++++++++++++
> > >  lib/xe/xe_util.h          |  40 ++++
> > >  tests/intel/xe_svm.c      | 476
> ++++++++++++++++++++++++++++++++++++++
> > >  tests/meson.build         |   1 +
> > >  5 files changed, 732 insertions(+)
> > >  create mode 100644 tests/intel/xe_svm.c
> > >
> > > --
> > > 2.25.1
> > >