Re: [PATCH 2/2] rust: drm: Add GPUVM abstraction
From: Daniel Almeida
Date: Mon Mar 24 2025 - 15:26:03 EST
Hi Miguel, thanks for having a look at this:
> On 24 Mar 2025, at 14:36, Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> 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@xxxxxxxxxxxxx> 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