[PATCH RFC i-g-t 0/2] helper function

Wed May 1 12:05:17 UTC 2024

Hi Matt,

Per your comments, we will target those helpers for system allocator test only for now. 

See a few comments inline also

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, May 1, 2024 12:44 AM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; igt-
> dev at lists.freedesktop.org; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH RFC i-g-t 0/2] helper function
> 
> On Tue, Apr 30, 2024 at 09:34:19PM -0600, Zeng, Oak wrote:
> > @Brost, Matthew This is the improved version of the igt helpers. It has
> some clear concepts of creating buffer and command buffer, and command
> submission. With those simple concepts, we are able to write an igt test in
> <20 LOC, see patch 2.
> >
> 
> Sure you can write an IGT with 20 LOC but these helpers ultimately put
> limits on the complexity of tests you can build. To get true coverage,
> you almost always are going to open code something. Or the helpers get
> so complex it actually makes things more confusing...
> 
> The open coding admittedly could be a lot better in the current IGT
> suite. A lot of these tests I just wrote as quickly as possible while
> writing Xe in a PoC stage and then they got merged. If I could rewrite
> these tests, they would a look a bit different.
> 
> Let give some examples of the limitations of the current helpers in this
> series.
> 
> xe_create_buffer - Things I could conviable want it to do beyond what it
> does for a system allocator test...
> 	- I want to use mmap with no file backing, with
> 	  several different flag combos (MAP_SHARED, MAP_PRIVATE,
> 	  MAP_ANONYMOUS, etc...)
> 	- I want to use mmap with file backing
> 	- I want various possible alignments
> 	- I want various possible sizes

We have a size parameter already. Maybe do whatever alignment in the caller and pass the aligned size to xe_create_buffer

> 	- I already have an exec queue

Create buffer function doesn't create an exec queue.

Create command buffer helper create one. You can always make exec queue creation part of create command buffer, right?

> 	- I don't want to bind it (e.g. already have system allocator
> 	  bind for it)

This would end up with another parameter: is_system_allocator, similar to the current is_userptr parameter

> 	- I don't want to alloc a new ufence

The ufence in xe_create_buffer is used for vm bind the buffer. You need this fence anyway. If you don't create it in create_buffer function, you would have to create in another place. Creating in xe_create_buffer makes things clean to me.

> 	- I may or may not want to memset it
> 	- I want to mlock it

Yes, all those would be new parameters

> 
> This is quickly gotten out of hand for a single helper. I could go on
> with the other helpers too.
> 
> If you really want to build powerful, complex tests in a generic way
> from my experience you really need to build an entire framework.
> Typcially these involve building operations lists and the passing these
> off these opertaions to a test runner which executes them. Building
> something like that takes quite a bit of skill and time. I haven't seen
> anything like that in the IGT suites and is likely out scope without a
> serious commitment (like a year plus dev on just the framework).
> 
> With all this, I'm not saying don't do this but I'm very sceptical this
> is going to be all that useful beyond fairly simple tests. I could be
> wrong or other might have different opinions than me too.

Let's first practice  and fine tune those helpers with system allocator igt. Let's see whether we can use them to create complex igt tests.

We can always come back after that.

Oak

> 
> Matt
> 
> > We might still fine tune those helpers but roughly the concept shouldn't
> change much.
> >
> > Do you think we can simplify existing igt tests such as xe-exec-fault-mode
> using those helpers? Do you suggest us to give it a try? If not, we will just use
> those helpers for svm test only.
> >
> > Hi Krishna,
> >
> > See comments inline
> >
> > > -----Original Message-----
> > > From: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>
> > > Sent: Tuesday, April 30, 2024 2:28 PM
> > > To: igt-dev at lists.freedesktop.org
> > > Cc: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; Zeng, Oak
> > > <oak.zeng at intel.com>; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray at intel.com>
> > > Subject: [PATCH RFC i-g-t 0/2] helper function
> >
> > The subject can be something like: Introduce helper functions for buffer
> creation and command submission etc
> >
> > Oak
> >
> > >
> > > Introduce helper functions for object creation, binding, submission,
> > > and destruction, applicable for SVM and other tests
> > >
> > > xe_svm test is validating the helper function introduced in 'lib/xe/xe_util:
> > > helper function'
> > >
> > > In this test I haven’t included the svm functionality, next patch I will
> include
> > > svm functionality
> > >
> > > Test results
> > > root at DUT7032PVCMella:/home/gta/xe/new/igt-gpu-
> > > tools# ./build/tests/xe_svm
> > > IGT-Version: 1.28-g365f81646 (x86_64) (Linux: 6.8.0-rc5-xedrmtip+ x86_64)
> > > Using IGT_SRANDOM=1714498247 for randomisation
> > > Opened device: /dev/dri/card1
> > > Starting subtest: svm-basic-malloc
> > > Subtest svm-basic-malloc: SUCCESS (0.022s)
> > >
> > > Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> > > Cc: Oak Zeng <oak.zeng at intel.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > >
> > > Bommu Krishnaiah (2):
> > >   lib/xe/xe_util: helper function
> > >   tests/intel/xe_svm: basic xe_svm test
> > >
> > >  lib/xe/xe_util.c     | 113
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  lib/xe/xe_util.h     |  32 ++++++++++++
> > >  tests/intel/xe_svm.c |  99
> +++++++++++++++++++++++++++++++++++++
> > >  tests/meson.build    |   1 +
> > >  4 files changed, 245 insertions(+)
> > >  create mode 100644 tests/intel/xe_svm.c
> > >
> > > --
> > > 2.25.1
> >