Hi Miguel, thanks for having a look at this: > On 24 Mar 2025, at 14:36, Miguel Ojeda <miguel.ojeda.sando...@gmail.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.alme...@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.
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
