Message-ID: <ec74271d-93b3-1c37-969b-aecc2d3deb7d@redhat.com>
Date:   Mon, 9 Oct 2023 00:48:59 +0200
From:   Danilo Krummrich <dakr@...hat.com>
To:     Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>, airlied@...il.com,
        daniel@...ll.ch, matthew.brost@...el.com, sarah.walker@...tec.com,
        donald.robson@...tec.com, boris.brezillon@...labora.com,
        christian.koenig@....com, faith@...strand.net
Cc:     dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-misc-next v5 0/6] [RFC] DRM GPUVM features

Hi Thomas,

On 10/5/23 11:35, Thomas Hellström wrote:
> Hi, Danilo
> 
> On 9/28/23 21:16, Danilo Krummrich wrote:
>> Currently GPUVM offers common infrastructure to track GPU VA allocations
>> and mappings, generically connect GPU VA mappings to their backing
>> buffers and perform more complex mapping operations on the GPU VA space.
>>
>> However, there are more design patterns commonly used by drivers, which
>> can potentially be generalized in order to make GPUVM represent the
>> basis of a VM implementation. In this context, this patch series aims at
>> generalizing the following elements.
>>
>> 1) Provide a common dma-resv for GEM objects not being used outside of
>>     this GPU-VM.
>>
>> 2) Provide tracking of external GEM objects (GEM objects which are
>>     shared with other GPU-VMs).
>>
>> 3) Provide functions to efficiently lock all GEM objects dma-resv the
>>     GPU-VM contains mappings of.
>>
>> 4) Provide tracking of evicted GEM objects the GPU-VM contains mappings
>>     of, such that validation of evicted GEM objects is accelerated.
>>
>> 5) Provide some convinience functions for common patterns.
>>
>> The implementation introduces struct drm_gpuvm_bo, which serves as abstraction
>> combining a struct drm_gpuvm and struct drm_gem_object, similar to what
>> amdgpu does with struct amdgpu_bo_vm. While this adds a bit of complexity it
>> improves the efficiency of tracking external and evicted GEM objects.
>>
>> This patch series is also available at [3].
>>
>> [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/commits/gpuvm-next
>>
>> Changes in V2:
>> ==============
>>    - rename 'drm_gpuva_manager' -> 'drm_gpuvm' which generally leads to more
>>      consistent naming
>>    - properly separate commits (introduce common dma-resv, drm_gpuvm_bo
>>      abstraction, etc.)
>>    - remove maple tree for tracking external objects, use a list drm_gpuvm_bos
>>      per drm_gpuvm instead
>>    - rework dma-resv locking helpers (Thomas)
>>    - add a locking helper for a given range of the VA space (Christian)
>>    - make the GPUVA manager buildable as module, rather than drm_exec
>>      builtin (Christian)
>>
>> Changes in V3:
>> ==============
>>    - rename missing function and files (Boris)
>>    - warn if vm_obj->obj != obj in drm_gpuva_link() (Boris)
>>    - don't expose drm_gpuvm_bo_destroy() (Boris)
>>    - unlink VM_BO from GEM in drm_gpuvm_bo_destroy() rather than
>>      drm_gpuva_unlink() and link within drm_gpuvm_bo_obtain() to keep
>>      drm_gpuvm_bo instances unique
>>    - add internal locking to external and evicted object lists to support drivers
>>      updating the VA space from within the fence signalling critical path (Boris)
>>    - unlink external objects and evicted objects from the GPUVM's list in
>>      drm_gpuvm_bo_destroy()
>>    - add more documentation and fix some kernel doc issues
>>
>> Changes in V4:
>> ==============
>>    - add a drm_gpuvm_resv() helper (Boris)
>>    - add a drm_gpuvm::<list_name>::local_list field (Boris)
>>    - remove drm_gpuvm_bo_get_unless_zero() helper (Boris)
>>    - fix missing NULL assignment in get_next_vm_bo_from_list() (Boris)
>>    - keep a drm_gem_object reference on potential vm_bo destroy (alternatively we
>>      could free the vm_bo and drop the vm_bo's drm_gem_object reference through
>>      async work)
>>    - introduce DRM_GPUVM_RESV_PROTECTED flag to indicate external locking through
>>      the corresponding dma-resv locks to optimize for drivers already holding
>>      them when needed; add the corresponding lock_assert_held() calls (Thomas)
>>    - make drm_gpuvm_bo_evict() per vm_bo and add a drm_gpuvm_bo_gem_evict()
>>      helper (Thomas)
>>    - pass a drm_gpuvm_bo in drm_gpuvm_ops::vm_bo_validate() (Thomas)
>>    - documentation fixes
>>
>> Changes in V5:
>> ==============
>>    - use a root drm_gem_object provided by the driver as a base for the VM's
>>      common dma-resv (Christian)
>>    - provide a helper to allocate a "dummy" root GEM object in case a driver
>>      specific root GEM object isn't available
>>    - add a dedicated patch for nouveau to make use of the GPUVM's shared dma-resv
>>    - improve documentation (Boris)
>>    - the following patches are removed from the series, since they already landed
>>      in drm-misc-next
>>      - f72c2db47080 ("drm/gpuvm: rename struct drm_gpuva_manager to struct drm_gpuvm")
>>      - fe7acaa727e1 ("drm/gpuvm: allow building as module")
>>      - 78f54469b871 ("drm/nouveau: uvmm: rename 'umgr' to 'base'")
>>
>> Danilo Krummrich (6):
>>    drm/gpuvm: add common dma-resv per struct drm_gpuvm
>>    drm/gpuvm: add drm_gpuvm_flags to drm_gpuvm
>>    drm/gpuvm: add an abstraction for a VM / BO combination
>>    drm/gpuvm: track/lock/validate external/evicted objects
>>    drm/nouveau: make use of the GPUVM's shared dma-resv
>>    drm/nouveau: use GPUVM common infrastructure
>>
>>   drivers/gpu/drm/drm_gpuvm.c             | 1036 +++++++++++++++++++++--
>>   drivers/gpu/drm/nouveau/nouveau_bo.c    |   15 +-
>>   drivers/gpu/drm/nouveau/nouveau_bo.h    |    5 +
>>   drivers/gpu/drm/nouveau/nouveau_exec.c  |   52 +-
>>   drivers/gpu/drm/nouveau/nouveau_exec.h  |    4 -
>>   drivers/gpu/drm/nouveau/nouveau_gem.c   |   10 +-
>>   drivers/gpu/drm/nouveau/nouveau_sched.h |    4 +-
>>   drivers/gpu/drm/nouveau/nouveau_uvmm.c  |  183 ++--
>>   drivers/gpu/drm/nouveau/nouveau_uvmm.h  |    1 -
>>   include/drm/drm_gem.h                   |   32 +-
>>   include/drm/drm_gpuvm.h                 |  465 +++++++++-
>>   11 files changed, 1625 insertions(+), 182 deletions(-)
>>
>>
>> base-commit: a4ead6e37e3290cff399e2598d75e98777b69b37
> 
> One comment I had before on the GPUVM code in general was the licensing, but I'm not sure there was a reply. Is it possible to have this code dual MIT / GPLV2?

Personally, I don't have any objections. Please feel free to send a patch to change it, I'm happy to ACK it.

- Danilo

> 
> Thanks,
> 
> Thomas
> 
> 
> 

Powered by blists - more mailing lists