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

Wed May 22 16:44:42 UTC 2024

Hi Matt,

I replied this before. I had a closer look to the example, see inline

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

Those are all options for system allocator and userptr. Those options doesn't apply to BO based buffer.

The concept behind the xe_create_buffer is, we combine the BO creation and vm_bind/wait-for-vm-bind-complete into one function, so it is easier to use.

@Bommu, Krishnaiah we can drop the is_userptr support in xe_create_buffer. Renaming xe_buffer/xe_create_buffer to xe_bo_buffer/xe_create_bo_buffer, only deal with BO type buffer in this function. The cmdbuf concept can still be on top of xe_bo_buffer.

For userptr, we can create xe-userptr-buffer and introduce more parameters to cover what Matt said above. Or we can decide to not have a wrapper for userptr. We don't use the userptr in system allocator tests. We can leave it for now. Those who write the userptr test can decide.

For system allocator, obviously we don't need any helper because there is no vm_bind for system allocator (except the gigantic vm_bind).

> 	- I want various possible alignments
> 	- I want various possible sizes

There is already a size parameter. User can always pass in an aligned sized to achieve alignment.

> 	- I already have an exec queue

To be more clear, we can remove the exec_queue from xe-bo-buffer, and create a xe_cmd_buf concept:

Struct xe_cmd_buf
{
	Struct xe_bo_buffer;
	Bool already_have_exec_queue;
	Uint32_t exec_queue;
};

Create/destroy exec queue conditionally.

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

We don't plan to have any wrapper for system allocator. By definition of system allocator, it just re-use existing Linux API which is well defined. No need to wrap it.

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

Similar to the exec_queue solution. If you want to use an existing bind_fence for xe-bo-buffer, we can define it:

Struct xe_bo_buffer {
	Bool already_have_bind_fence;
	Uint32_t *bind_fence;
.....
};

Then create/destroy the bind_fence conditionally.

> 	- I may or may not want to memset it

Introduce a bool memset parameter to xe-bo-buffer

> 	- I want to mlock it

I think mlock only applies to system allocator and userptr. It doesn't apply to BO type buffer. So we don't need this in xe-bo-buffer concept.

So by a close look of your example, we can address all your concerns by:

1) narrow down the xe-buffer concept to xe-bo-buffer to only cover BO type buffer

2) introduce a few more parameters to make the creation behavior more flexible.

The key point is, introducing helper functions and concept can simplify test writing. Combine a BO creation and vm_bind/bo_map is very natural: you need to to vm_bind/map BO sooner or later. So why not do it in one helper function to simply coding?

Same thing with the command buffer concept. with each command buffer (batch buffer some people call it), you would need a exec queue for later submission; you also need to fill the command buffer with GPU commands. Wrapping those into a command buffer concept simply codes.

Oak

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