Re: [Intel-xe] [PATCH v5 0/8] Port Xe to use GPUVA and implement NULL VM binds

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

On Tue, Apr 04, 2023 at 03:20:47PM +0200, Thomas Hellström wrote:
> Hi, Matthew,
> 
> 
> On 4/4/23 03:42, Matthew Brost wrote:
> > GPUVA is common code written primarily by Danilo with the idea being a
> > common place to track GPUVAs (VMAs in Xe) within an address space (VMs
> > in Xe), track all the GPUVAs attached to GEMs, and a common way
> > implement VM binds / unbinds with MMAP / MUNMAP semantics via creating
> > operation lists. All of this adds up to a common way to implement VK
> > sparse bindings.
> > 
> > This series pulls in the GPUVA code written by Danilo plus some small
> > fixes by myself into 1 large patch. Once the GPUVA makes it upstream, we
> > can rebase and drop this patch. I believe what lands upstream should be
> > nearly identical to this patch at least from an API perspective.
> > 
> > The last three patches port Xe to GPUVA and add support for NULL VM binds
> > (writes dropped, read zero, VK sparse support). An example of the
> > semantics of this is below.
> 
> Going through thew new code in xe_vm.c I'm still concerned about our error
> handling. In the cases we attempt to handle, for example an -ENOMEM, the
> sync ioctl appears to be doing a fragile unwind whereas the async worker
> puts the VM in an error state that can only be exited by a RESTART ioctl
> (pls correct me if I'm wrong here). We could of course do the same for sync
> ioctls, but I'm still wondering what happens if, for example a MAP operation
> on a dual-gt VM fails after binding on the first gt?
> 

So sync mode we don't allow advanced binds (more than 1 map operation)
but I failed to take into account dual-gt VMs, yea that might be an
issue foe sync binds. This should just for work async though. Maybe we
depreciate the sync mode except for unbinds when in an error state.

> I think we need to clearly define the desired behaviour in the uAPI and then
> make sure we do the necessary changes with that in mind, and Even if I
> prefer the !async_worker solution this can be don orthogonally to that
> discussion. Ideally I'd see we find a way to *not* put the VM  in an error
> state like this, and I figure there are various ways to aid in this, for
> example keeping a progress cookie in the user-space data or deferring any
> failed bind operations to the next rebind worker or exec, but in the end I
> think this boils down to allocating all needed resources up-front for
> operations or groups of operations that are not allowed to fail once we
> start to modify the page-tables.
>

Agree on clearly defined uAPI behavior, I'm not sure if I have answer
for the error handling. Personally I like the stop / restart mechanism
as it essentially mean binds never can fail with a well behavied user.
wrt to Allocating up front, I think that is what Nouveau is doing? We
could study that code but allocating everything up front will be a
pretty large change.

Obviously we need to do some work here and this is one of our biggest
opens left but I don't view this as a true blocker until we remove force
probe. GPUVA regardless is a step in the right direction which will make
any changes going forward easier + enable VK to get sparse implemented.
I'd say let's get GPUVA merged and next focus on these issues you have
raised.

Also another VM bind issue we need address soon:
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/41

Matt

> Thoughts?
> 
> /Thomas
> 
> 
> 
> 
> > 
> > MAP 0x0000-0x8000 to NULL 	- 0x0000-0x8000 writes dropped + read zero
> > MAP 0x4000-0x5000 to a GEM 	- 0x0000-0x4000, 0x5000-0x8000 writes dropped + read zero; 0x4000-0x5000 mapped to a GEM
> > UNMAP 0x3000-0x6000		- 0x0000-0x3000, 0x6000-0x8000 writes dropped + read zero
> > UNMAP 0x0000-0x8000		- Nothing mapped
> > 
> > No changins to existing behavior, rather just new functionality./
> > 
> > v2: Fix CI build failure
> > v3: Export mas_preallocate, add patch to avoid rebinds
> > v5: Bug fixes, rebase, xe_vma size optimizations
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > Danilo Krummrich (2):
> >    maple_tree: split up MA_STATE() macro
> >    drm: manager to keep track of GPUs VA mappings
> > 
> > Matthew Brost (6):
> >    maple_tree: Export mas_preallocate
> >    drm/xe: Port Xe to GPUVA
> >    drm/xe: NULL binding implementation
> >    drm/xe: Avoid doing rebinds
> >    drm/xe: Reduce the number list links in xe_vma
> >    drm/xe: Optimize size of xe_vma allocation
> > 
> >   Documentation/gpu/drm-mm.rst                |   31 +
> >   drivers/gpu/drm/Makefile                    |    1 +
> >   drivers/gpu/drm/drm_debugfs.c               |   56 +
> >   drivers/gpu/drm/drm_gem.c                   |    3 +
> >   drivers/gpu/drm/drm_gpuva_mgr.c             | 1890 ++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_bo.c                  |   10 +-
> >   drivers/gpu/drm/xe/xe_bo.h                  |    1 +
> >   drivers/gpu/drm/xe/xe_device.c              |    2 +-
> >   drivers/gpu/drm/xe/xe_exec.c                |    4 +-
> >   drivers/gpu/drm/xe/xe_gt_pagefault.c        |   29 +-
> >   drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c |   14 +-
> >   drivers/gpu/drm/xe/xe_guc_ct.c              |    6 +-
> >   drivers/gpu/drm/xe/xe_migrate.c             |    8 +-
> >   drivers/gpu/drm/xe/xe_pt.c                  |  176 +-
> >   drivers/gpu/drm/xe/xe_trace.h               |   10 +-
> >   drivers/gpu/drm/xe/xe_vm.c                  | 1947 +++++++++----------
> >   drivers/gpu/drm/xe/xe_vm.h                  |   76 +-
> >   drivers/gpu/drm/xe/xe_vm_madvise.c          |   87 +-
> >   drivers/gpu/drm/xe/xe_vm_types.h            |  276 ++-
> >   include/drm/drm_debugfs.h                   |   24 +
> >   include/drm/drm_drv.h                       |    7 +
> >   include/drm/drm_gem.h                       |   75 +
> >   include/drm/drm_gpuva_mgr.h                 |  734 +++++++
> >   include/linux/maple_tree.h                  |    7 +-
> >   include/uapi/drm/xe_drm.h                   |    8 +
> >   lib/maple_tree.c                            |    1 +
> >   26 files changed, 4203 insertions(+), 1280 deletions(-)
> >   create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c
> >   create mode 100644 include/drm/drm_gpuva_mgr.h
> >