Re: [PATCH 2/2] rust: drm: Add GPUVM abstraction

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

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@collabora.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.

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`.

(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?

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.

> +// SAFETY: All our trait methods take locks

(Throughout the file) Period at the end.

Thanks!

Cheers,
Miguel