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 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.

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
>