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

[Matthew Brost <matthew.brost@intel.com>]

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. 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. These tests are so simple they won't find
> any bugs in the KMD beyond the implementation being completely broken.
> 
> 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.
> 
> 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
> >