Message-Id: <89BB40E5-24CA-44E8-AE67-3E58A118A5A8@collabora.com>
Date: Tue, 5 Aug 2025 14:56:24 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <thatslyude@...il.com>
Cc: Onur Özkan <work@...rozkan.dev>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 ojeda@...nel.org,
 alex.gaynor@...il.com,
 boqun.feng@...il.com,
 gary@...yguo.net,
 lossin@...nel.org,
 a.hindborg@...nel.org,
 aliceryhl@...gle.com,
 tmgross@...ch.edu,
 dakr@...nel.org,
 peterz@...radead.org,
 mingo@...hat.com,
 will@...nel.org,
 longman@...hat.com,
 felipe_life@...e.com,
 daniel@...lak.dev,
 bjorn3_gh@...tonmail.com
Subject: Re: [PATCH v5 0/3] rust: add `ww_mutex` support



> On 5 Aug 2025, at 13:22, Lyude Paul <thatslyude@...il.com> wrote:
> 
> Hey! Onur, if you could make sure that future emails get sent to
> 
> lyude at redhat dot com
> 
> That would be appreciated! I usually am paying much closer attention to that
> email address. That being said, some comments down below:
> 
> On Thu, 2025-07-24 at 16:53 +0300, Onur Özkan wrote:
>> Hi again,
>> 
>> Just finished going over the C-side use of `ww_mutex` today and I
>> wanted to share some notes and thoughts based on that.
>> 
>> To get the full context, you might want to take a look at this thread
>> [1].
>> 
>> - The first note I took is that we shouldn't allow locking without
>> `WwAcquireCtx` (which is currently possible in v5). As explained in
>> ww_mutex documentation [2], this basically turns it into a regular
>> mutex and you don't get benefits of `ww_mutex`.
> 
> I disagree about this conclusion actually, occasionally you do just need to
> acquire a single mutex and not multiple. Actually - we even have a
> drm_modeset_lock_single_*() set of functions in KMS specifically for this
> purpose. 
> 
> Here's an example where we use it in nouveau for inspecting the atomic display
> state of a specific crtc:
> 
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/nouveau/dispnv50/crc.c#L682
> 
> This isn't actually too unusual of a usecase tbh, especially considering that
> the real reason we have ww_mutexes in KMS is to deal with the atomic
> transaction model that's used for modesetting in the kernel.
> 
> A good example, which also doubles as a ww_mutex example you likely missed on
> your first skim since all of it is done through the drm_modeset_lock API and
> not ww_mutex directly:
> 
> https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/nouveau/dispnv50/crc.c#L544
> 
> drm_modeset_acquire_init() is a wrapper around ww_mutex_init() which actually
> does pretty much exactly what Daniel is describing lower in the thread:
> keeping track of a list of each acquired lock so that they can be dropped once
> the context is released.
> 
> drm_atomic_get_crtc_state() grabs the CRTC context and ensures that the crtc's
> modeset lock (e.g. a ww_mutex) is actually acquired
> 
> drm_atomic_commit() performs the checking of the atomic modeset transaction,
> e.g. going through the requested display settings and ensuring that the
> display hardware is actually capable of supporting it before allowing the
> modeset to continue. Often times for GPU drivers this process can involve not
> just checking limitations on the modesetting object in question, but
> potentially adding other modesetting objects into the transaction that the
> driver needs to also inspect the state of. Adding any of these modesetting
> objects potentially means having to acquire their modeset locks using the same
> context, and we can't and don't really want to force users to have an idea of
> exactly how many locks can ever be acquired. Display hardware is wonderful at
> coming up with very wacky limitations we can't really know ahead of time
> because they can even depend on the global display state.
> 
> So tracking locks is definitely the way to go, but we should keep in mind
> there's already infrastructure in the kernel doing this that we want to be
> able to handle with these APIs as well.
> 

Well, the API I proposed would be implemented as a separate type, so the whole
thing would be opt-in anyways. There’s nothing stopping us from providing
abstractions for the current infrastructure as well and both things should
co-exist fine, at least that’s my understanding at this point.

IOW: there is nothing stopping us from implementing say, a drm_exec abstraction
(or whatever else, really) if needed for whatever reason. But the proposed API
should be much more idiomatic for Rust code.

— Daniel

Powered by blists - more mailing lists