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

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

[Matthew Brost]

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

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

[Zeng, Oak]

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@intel.com>
> Sent: Wednesday, May 22, 2024 7:42 AM
> To: Bommu, Krishnaiah <krishnaiah.bommu@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Brost, Matthew
> <matthew.brost@intel.com>; intel-xe@lists.freedesktop.org; Zeng, Oak
> <oak.zeng@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
> > >

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

[Matthew Brost]

On Wed, May 22, 2024 at 10:53:05AM -0600, Zeng, Oak wrote:
> 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@intel.com>
> > Sent: Wednesday, May 22, 2024 7:42 AM
> > To: Bommu, Krishnaiah <krishnaiah.bommu@intel.com>
> > Cc: igt-dev@lists.freedesktop.org; Brost, Matthew
> > <matthew.brost@intel.com>; intel-xe@lists.freedesktop.org; Zeng, Oak
> > <oak.zeng@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.
> 

I did miss that read/write addresses are malloc'd or mmap'd.

However, I don't see a huge bind anywhere in the series. Buffer Objects
(BOs) are only unmapped, not returned to the system allocator bindings,
creating holes in the system allocator space. Additionally, BO addresses
are statically assigned, which does not align with the system allocator
design of calling malloc to reserve address space. This was the source
of my confusion.

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

So we agree that my concerns raised in [1] [2] about only being able to
build simple tests with this design were not addressed.

Having plans is not the same as addressing concerns. If you believe you
can build complex tests achieving good coverage using this design,
great. Please post that. Until then, I remain unconvinced.

I briefly read the Jira; a lot of it is covered by the code shared. What
is not can likely be added in a similar way to the HUGE_TLB example I
shared in the previous email.

One thing I noticed in Jira is that it's written basically as unit tests
(e.g., test feature A, test feature B). Unit tests are okay, but truly
powerful tests do a lot of things all at the same time, in parallel.
This is the advantage of the style of tests I write. Six lines of code
in my example (data path code, that is; tables and documents added more)
spawned over 100 tests doing this in the aforementioned way.

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

This is an opinion, disrespectful, and rude.

I've provided a concrete example of how the style of tests I've written
in Xe are quite powerful in this email thread. In my opinion, it has
been responded to with speculative arguments and opinions rather than
code.

I have very, very valid reasons for being concerned about testing. This
series [5] was posted in a completely non-functional, untested state. As
reminding and to record this publicly, this was developed off a basline
which was tested with userptr SVM faulting working. I provided the
working baseline and a flavor of the test I have shared.

Going from a tested working KMD with an IGT to untested broken KMD
plus a promise of 'obvious, our approach is much easier to maintain and
understand', well I'll let everyone decide how that looks.

[5] https://patchwork.freedesktop.org/series/132229/

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

I highly suggest you study the code that I shared.

Matt

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

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

[Zeng, Oak]

Hi Matt,

> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Thursday, May 23, 2024 1:27 PM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; igt-
> dev@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> 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 10:53:05AM -0600, Zeng, Oak wrote:
> > 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@intel.com>
> > > Sent: Wednesday, May 22, 2024 7:42 AM
> > > To: Bommu, Krishnaiah <krishnaiah.bommu@intel.com>
> > > Cc: igt-dev@lists.freedesktop.org; Brost, Matthew
> > > <matthew.brost@intel.com>; intel-xe@lists.freedesktop.org; Zeng, Oak
> > > <oak.zeng@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.
> >
> 
> I did miss that read/write addresses are malloc'd or mmap'd.
> 
> However, I don't see a huge bind anywhere in the series. 

Yes, I pointed out the same in the code review. We need to fix it.

Buffer Objects
> (BOs) are only unmapped, not returned to the system allocator bindings,
> creating holes in the system allocator space. Additionally, BO addresses
> are statically assigned, which does not align with the system allocator
> design of calling malloc to reserve address space. This was the source
> of my confusion.

Good points. We need to fix the issues.

> 
> > 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
> >
> 
> So we agree that my concerns raised in [1] [2] about only being able to
> build simple tests with this design were not addressed.
> 
> Having plans is not the same as addressing concerns. If you believe you
> can build complex tests achieving good coverage using this design,
> great. Please post that. Until then, I remain unconvinced.
> 
> I briefly read the Jira; a lot of it is covered by the code shared. What
> is not can likely be added in a similar way to the HUGE_TLB example I
> shared in the previous email.
> 
> One thing I noticed in Jira is that it's written basically as unit tests
> (e.g., test feature A, test feature B). Unit tests are okay, but truly
> powerful tests do a lot of things all at the same time, in parallel.
> This is the advantage of the style of tests I write. Six lines of code
> in my example (data path code, that is; tables and documents added more)
> spawned over 100 tests doing this in the aforementioned way.

Yes, I had the same thoughts. 

They are basically two categories of tests:

unit tests. We will try to cover as much as possible in the simple unit test.

integrated test or stress test. 

I tend to think we want to have both. Of course we might have some duplications, such as some functions are tested from both categories. Hopefully this is still acceptable. 

> 
> > 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.
> >
> 
> This is an opinion, disrespectful, and rude.

I sincerely apologize if those words hurt you. It is not intended to.

> 
> I've provided a concrete example of how the style of tests I've written
> in Xe are quite powerful in this email thread. In my opinion, it has
> been responded to with speculative arguments and opinions rather than
> code.
> 
> I have very, very valid reasons for being concerned about testing. This
> series [5] was posted in a completely non-functional, untested state. As
> reminding and to record this publicly, this was developed off a basline
> which was tested with userptr SVM faulting working. I provided the
> working baseline and a flavor of the test I have shared.
> 
> Going from a tested working KMD with an IGT to untested broken KMD
> plus a promise of 'obvious, our approach is much easier to maintain and
> understand', well I'll let everyone decide how that looks.
> 
> [5] https://patchwork.freedesktop.org/series/132229/
> 
> > 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.
> >
> 
> I highly suggest you study the code that I shared.

Yes, will do. Let's see what we can come up.

Oak
> 
> Matt
> 
> > 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
> > > > >