Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Tue, 31 Oct 2023 13:38:45 +0100
From:   Boris Brezillon <boris.brezillon@collabora.com>
To:     Danilo Krummrich <dakr@redhat.com>
Cc:     airlied@gmail.com, daniel@ffwll.ch, matthew.brost@intel.com,
        thomas.hellstrom@linux.intel.com, sarah.walker@imgtec.com,
        donald.robson@imgtec.com, christian.koenig@amd.com,
        faith@gfxstrand.net, dri-devel@lists.freedesktop.org,
        nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH drm-misc-next v7 0/7] [RFC] DRM GPUVM features
Message-ID: <20231031133845.7915c814@collabora.com>
In-Reply-To: <20231023201659.25332-1-dakr@redhat.com>
References: <20231023201659.25332-1-dakr@redhat.com>
Organization: Collabora
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 23 Oct 2023 22:16:46 +0200
Danilo Krummrich <dakr@redhat.com> 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'")
> 
> Changes in V6:
> ==============
>   - add drm_gpuvm_bo::evicted field protected by the drm_gem_object's dma-resv
>     lock (Thomas)
>     - additionally to the original proposal, always use drm_gpuvm_bo::evicted
>       regardless of the used locking scheme and always keep it up to date
>   - remove unneccesary get->put dance in drm_gpuva_unlink() (Thomas)
>   - fix commit message wording (Thomas)
>   - fix kernel doc warnings (kernel test robot)
> 
> Changes in V7:
> ==============
>   - add a patch converting WARN() macros to drm_WARN() variants
>   - allow drivers to pass the number of fences to reserve and the drm_exec flags
>     through struct drm_gpuvm_exec
>   - rename 'root' GEM object to 'resv' GEM object
>   - fix order of private_usage and extobj_usage in drm_gpuvm_resv_add_fence()
>   - always set drm_gpuvm_bo::evicted accordingly
>   - explicitly clear drm_gpuvm_bo from evict list after successful validation
>   - group reference get() calls with pointer assignments
>   - call drm_gem_object_put() after vm_bo_free() callback
>   - make lockdep checks explicit for drm_gpuvm_bo_* functions
>   - improve documentation of struct drm_gpuvm_bo
>   - fix a few documentation typos and style issues
>   - use BIT() instead of shift ops for enum drm_gpuvm_flags
> 
> Danilo Krummrich (7):
>   drm/gpuvm: convert WARN() to drm_WARN() variants
>   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

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
>  drivers/gpu/drm/drm_gpuvm.c             | 1054 +++++++++++++++++++++--
>  drivers/gpu/drm/nouveau/nouveau_bo.c    |   15 +-
>  drivers/gpu/drm/nouveau/nouveau_bo.h    |    5 +
>  drivers/gpu/drm/nouveau/nouveau_exec.c  |   57 +-
>  drivers/gpu/drm/nouveau/nouveau_exec.h  |    4 -
>  drivers/gpu/drm/nouveau/nouveau_gem.c   |   10 +-
>  drivers/gpu/drm/nouveau/nouveau_sched.c |    9 +-
>  drivers/gpu/drm/nouveau/nouveau_sched.h |    7 +-
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c  |  189 ++--
>  drivers/gpu/drm/nouveau/nouveau_uvmm.h  |    1 -
>  include/drm/drm_gem.h                   |   32 +-
>  include/drm/drm_gpuvm.h                 |  492 ++++++++++-
>  12 files changed, 1673 insertions(+), 202 deletions(-)
> 
> 
> base-commit: f5b55f32ce4ba953c270b2e9c3f5d4cd6951b1a1