Message-Id: <509EADD7-607B-4DED-ADAC-152D7338EB50@collabora.com>
Date: Mon, 24 Mar 2025 16:25:07 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH 2/2] rust: drm: Add GPUVM abstraction

Hi Miguel, thanks for having a look at this:

> On 24 Mar 2025, at 14:36, Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:
> 
> Hi Daniel,
> 
> A few quick notes for future versions on style/docs to try to keep
> things consistent upstream -- not an actual review.
> 
> On Mon, Mar 24, 2025 at 4:14 PM Daniel Almeida
> <daniel.almeida@...labora.com> wrote:
>> 
>> +#[allow(type_alias_bounds)]
> 
> The documentation says this is highly discouraged -- it may be good to
> mention why it is OK in this instance in a comment or similar.

Someone correct me here, but I see no issue with this warning. That’s
because we need the bound to make `<T::Driver as drv::Driver>` work in the
first place. Otherwise, we’d get a compiler error saying that there’s
no `Driver` associated type (assuming the case where a random T gets
passed in)

So, for this to be a problem, we would need to mix this up with something that
also has a `Driver` associated type, and this associated type would also need a
drv::Driver bound.

In other words, we would need a lot of things to align for this to actually
have a chance of being misused. When you consider that this is then only used
in a few places, the balance tips heavily in favor of the convenience of having
the type alias IMHO.

In fact, the docs point to the exact thing I am trying to do, i.e.:

>  these bounds may have secondary effects such as enabling the use of “shorthand” associated type paths

>  I.e., paths of the form T::Assoc where T is a type parameter bounded by trait Trait which defines an associated type called Assoc as opposed to a fully qualified path of the form <T as Trait>::Assoc. 

> 
> Also, could this be `expect`? (e.g. if it triggers in all compiler
> versions we support)
> 
>> +// A convenience type for the driver's GEM object.
> 
> Should this be a `///` comment, i.e. docs?
> 
>> +/// Trait that must be implemented by DRM drivers to represent a DRM GpuVm (a GPU address space).
> 
> (Throughout the file) Markdown in documentation, e.g. `GpuVm`.

By the way, maybe we should have a lint for CamelCase in docs? I tried my best to
cover all of these, but some slip through :/

i.e.: if you write something in CamelCase somewhere in the docs, there's a high
chance that you should actually use Markdown and link as appropriate.

I have no idea whether this would actually work in practice, to be honest. It’s just
a random suggestion (that I'd be willing to help with).


> 
> (Throughout the file) Intra-doc links where they work, e.g. [`GpuVm`]
> (assuming it works this one).
> 
>> +        // - Ok(()) is always returned.
> 
> (Throughout the file) Markdown in normal comments too.
> 
>> +/// A transparent wrapper over `drm_gpuva_op_map`.
> 
> (Throughout the file) A link to C definitions is always nice if there
> is a good one, e.g.
> 
>    [`drm_gpuva_op_map`]:
> https://docs.kernel.org/gpu/drm-mm.html#c.drm_gpuva_op_map
> 
> Ideally we will eventually have a better way to link these
> automatically, but for the time being, this helps (and later we can do
> a replace easier).
> 
>> +/// `None`.
>> +
>> +/// Note: the reason for a dedicated remap operation, rather than arbitrary
> 
> Missing `///` (?).
> 
>> +#[repr(C)]
>> +#[pin_data]
>> +/// A GPU VA range.
>> +///
>> +/// Drivers can use `inner` to store additional data.
> 
> (Throughout the file) We typically place attributes go below the
> documentation -- or is there a reason to do it like this?

I will be honest with you here: I never remember the right order for docs and attributes.

I’ll fix this.

> 
> We had cases with e.g. Clippy bugs regarding safety comments that
> could be workarounded with "attribute movement", but it does not seem
> to be the case here.
> 
>> +        if p.is_null() {
>> +            Err(ENOMEM)
> 
> For error cases, we typically try to do early returns instead.
> 
>> +    /// Iterates the given range of the GPU VA space. It utilizes
>> +    /// [`DriverGpuVm`] to call back into the driver providing the split and
>> +    /// merge steps.
> 
> This title (and the next one) may be a bit too long (or not -- please
> check in the rendered docs), i.e. the first paragraph is the "title",
> which is used differently in the rendered docs. If there is a way to
> have a shorter title that still differentiates between the two
> methods, that would be nice.
> 
>> +    /// # Arguments
>> +    ///
>> +    /// - `ctx`: A driver-specific context.
>> +    /// - `req_obj`: The GEM object to map.
>> +    /// - `req_addr`: The start address of the new mapping.
>> +    /// - `req_range`: The range of the mapping.
>> +    /// - `req_offset`: The offset into the GEM object.
> 
> Normally we try to avoid this kind of sections and instead reference
> the arguments from the text (e.g. "...the range of the mapping
> (`req_range`)...") -- but if there is no good way to do it, then it is
> OK.

Ack.

> 
>> +// SAFETY: All our trait methods take locks
> 
> (Throughout the file) Period at the end.
> 
> Thanks!
> 
> Cheers,
> Miguel

— Daniel

Powered by blists - more mailing lists