| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250822115221.24fffc2c@fedora>
Date: Fri, 22 Aug 2025 11:52:21 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Danilo Krummrich
<dakr@...nel.org>, Daniel Almeida <daniel.almeida@...labora.com>, Steven
Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>, Rob Clark
<robin.clark@....qualcomm.com>, Rob Herring <robh@...nel.org>, Miguel Ojeda
<ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
<gary@...yguo.net>, "Björn Roy Baron"
<bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Andreas
Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/3] drm_gem: add mutex to drm_gem_object.gpuva
On Fri, 22 Aug 2025 09:28:24 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:
> There are two main ways that GPUVM might be used:
>
> * staged mode, where VM_BIND ioctls update the GPUVM immediately so that
> the GPUVM reflects the state of the VM *including* staged changes that
> are not yet applied to the GPU's virtual address space.
> * immediate mode, where the GPUVM state is updated during run_job(),
> i.e., in the DMA fence signalling critical path, to ensure that the
> GPUVM and the GPU's virtual address space has the same state at all
> times.
>
> Currently, only Panthor uses GPUVM in immediate mode, but the Rust
> drivers Tyr and Nova will also use GPUVM in immediate mode, so it is
> worth to support both staged and immediate mode well in GPUVM. To use
> immediate mode, the GEMs gpuva list must be modified during the fence
> signalling path, which means that it must be protected by a lock that is
> fence signalling safe.
>
> For this reason, a mutex is added to struct drm_gem_object that is
> intended to achieve this purpose. Adding it directly in the GEM object
> both makes it easier to use GPUVM in immediate mode, but also makes it
> possible to take the gpuva lock from core drm code.
>
> As a follow-up, another change that should probably be made to support
> immediate mode is a mechanism to postpone cleanup of vm_bo objects, as
> dropping a vm_bo object in the fence signalling path is problematic for
> two reasons:
>
> * When using DRM_GPUVM_RESV_PROTECTED, you cannot remove the vm_bo from
> the extobj/evicted lists during the fence signalling path.
> * Dropping a vm_bo could lead to the GEM object getting destroyed.
> The requirement that GEM object cleanup is fence signalling safe is
> dubious and likely to be violated in practice.
>
> Panthor already has its own custom implementation of postponing vm_bo
> cleanup.
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
Reviewed-by: Boris Brezillon <boris.brezillon@...labora.com>
One minor thing below.
> ---
> drivers/gpu/drm/drm_gem.c | 2 ++
> include/drm/drm_gem.h | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 4a89b6acb6af39720451ac24033b89e144d282dc..8d25cc65707d5b44d931beb0207c9d08a3e2de5a 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -187,6 +187,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
> kref_init(&obj->refcount);
> obj->handle_count = 0;
> obj->size = size;
> + mutex_init(&obj->gpuva.lock);
> dma_resv_init(&obj->_resv);
> if (!obj->resv)
> obj->resv = &obj->_resv;
> @@ -210,6 +211,7 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj)
> WARN_ON(obj->dma_buf);
>
> dma_resv_fini(&obj->_resv);
> + mutex_destroy(&obj->gpuva.lock);
> }
> EXPORT_SYMBOL(drm_gem_private_object_fini);
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index d3a7b43e2c637b164eba5af7cc2fc8ef09d4f0a4..5934d8dc267a65aaf62d2d025869221cd110b325 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -403,11 +403,13 @@ struct drm_gem_object {
> * Provides the list of GPU VAs attached to this GEM object.
> *
> * Drivers should lock list accesses with the GEMs &dma_resv lock
> - * (&drm_gem_object.resv) or a custom lock if one is provided.
> + * (&drm_gem_object.resv) or a custom lock if one is provided. The
> + * mutex inside this struct may be used as the custom lock.
> */
> struct {
> struct list_head list;
>
> + struct mutex lock;
Maybe it's time we start moving some bits of the gpuva field docs next
to the fields they describe:
/**
* @gpuva: Fields used by GPUVM to manage mappings pointing to this GEM object.
*/
struct {
/**
* @gpuva.list: list of GPU VAs attached to this GEM object.
*
* Drivers should lock list accesses with the GEMs &dma_resv lock
* (&drm_gem_object.resv) or &drm_gem_object.gpuva.lock if the
* list is being updated in places where the resv lock can't be
* acquired (fence signalling path).
*/
struct list_head list;
/**
* @gpuva.lock: lock protecting access to &drm_gem_object.gpuva.list
* when the resv lock can't be used.
*
* Should only be used when the VM is being modified in a fence
* signalling path, otherwise you should use &drm_gem_object.resv to
* protect accesses to &drm_gem_object.gpuva.list.
*/
struct mutex lock;
...
};
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map *lock_dep_map;
> #endif
>
Powered by blists - more mailing lists