Message-ID: <ce8b4bd9-5122-4b85-b605-a1bf10556208@arm.com>
Date: Mon, 28 Jul 2025 14:25:25 +0100
From: Steven Price <steven.price@....com>
To: Alexandre Courbot <acourbot@...dia.com>,
 Daniel Almeida <daniel.almeida@...labora.com>
Cc: Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Beata Michalska <beata.michalska@....com>, nouveau@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org, John Hubbard <jhubbard@...dia.com>,
 Nouveau <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v2 01/19] gpu: nova-core: register: minor grammar and
 spelling fixes

On 28/07/2025 12:43, Alexandre Courbot wrote:
> On Mon Jul 28, 2025 at 4:51 PM JST, Steven Price wrote:
>> On 28/07/2025 05:59, Alexandre Courbot wrote:
>>> Hi Daniel, thanks for the review!
>>>
>>> On Sat Jul 26, 2025 at 1:14 AM JST, Daniel Almeida wrote:
>>>> Hi Alex. Thank you and John for working on this in general. It will be useful
>>>> for the whole ecosystem! :) 
>>>>
>>>>> On 18 Jul 2025, at 04:26, Alexandre Courbot <acourbot@...dia.com> wrote:
>>>>>
>>>>> From: John Hubbard <jhubbard@...dia.com>
>>>>>
>>>>> There is only one top-level macro in this file at the moment, but the
>>>>> "macros.rs" file name allows for more. Change the wording so that it
>>>>> will remain valid even if additional macros are added to the file.
>>>>>
>>>>> Fix a couple of spelling errors and grammatical errors, and break up a
>>>>> run-on sentence, for clarity.
>>>>>
>>>>> Cc: Alexandre Courbot <acourbot@...dia.com>
>>>>> Cc: Danilo Krummrich <dakr@...nel.org>
>>>>> Signed-off-by: John Hubbard <jhubbard@...dia.com>
>>>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>>>>> ---
>>>>> drivers/gpu/nova-core/regs/macros.rs | 14 +++++++-------
>>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
>>>>> index cdf668073480ed703c89ffa8628f5c9de6494687..864d1e83bed2979f5661e038f4c9fd87d33f69a7 100644
>>>>> --- a/drivers/gpu/nova-core/regs/macros.rs
>>>>> +++ b/drivers/gpu/nova-core/regs/macros.rs
>>>>> @@ -1,17 +1,17 @@
>>>>> // SPDX-License-Identifier: GPL-2.0
>>>>>
>>>>> -//! Macro to define register layout and accessors.
>>>>> +//! `register!` macro to define register layout and accessors.
>>>>
>>>> I would have kept this line as-is. Users will most likely know the name of the
>>>> macro already. At this point, they will be looking for what it does, so
>>>> mentioning "register" here is a bit redundant IMHO.
>>>>
>>>>> //!
>>>>> //! A single register typically includes several fields, which are accessed through a combination
>>>>> //! of bit-shift and mask operations that introduce a class of potential mistakes, notably because
>>>>> //! not all possible field values are necessarily valid.
>>>>> //!
>>>>> -//! The macro in this module allow to define, using an intruitive and readable syntax, a dedicated
>>>>> -//! type for each register with its own field accessors that can return an error is a field's value
>>>>> -//! is invalid.
>>>>> +//! The `register!` macro in this module provides an intuitive and readable syntax for defining a
>>>>> +//! dedicated type for each register. Each such type comes with its own field accessors that can
>>>>> +//! return an error if a field's value is invalid.
>>>>>
>>>>> -/// Defines a dedicated type for a register with an absolute offset, alongside with getter and
>>>>> -/// setter methods for its fields and methods to read and write it from an `Io` region.
>>>>> +/// Defines a dedicated type for a register with an absolute offset, including getter and setter
>>>>> +/// methods for its fields and methods to read and write it from an `Io` region.
>>>>
>>>> +cc Steven Price,
>>>>
>>>> Sorry for hijacking this patch, but I think that we should be more flexible and
>>>> allow for non-literal offsets in the macro.
>>>>
>>>> In Tyr, for example, some of the offsets need to be computed at runtime, i.e.:
>>>>
>>>> +pub(crate) struct AsRegister(usize);
>>>> +
>>>> +impl AsRegister {
>>>> +    fn new(as_nr: usize, offset: usize) -> Result<Self> {
>>>> +        if as_nr >= 32 {
>>>> +            Err(EINVAL)
>>>> +        } else {
>>>> +            Ok(AsRegister(mmu_as(as_nr) + offset))
>>>> +        }
>>>> +    }
>>>>
>>>> Or:
>>>>
>>>> +pub(crate) struct Doorbell(usize);
>>>> +
>>>> +impl Doorbell {
>>>> +    pub(crate) fn new(doorbell_id: usize) -> Self {
>>>> +        Doorbell(0x80000 + (doorbell_id * 0x10000))
>>>> +    }
>>>>
>>>> I don't think this will work with the current macro, JFYI.
>>>
>>> IIUC from the comments on the next patches, your need is covered with
>>> the relative and array registers definitions, is that correct?
>>
>> My Rust is somewhat shaky, but I believe "non-contiguous register 
>> arrays" will do what we want. Although I'll admit it would be neater for 
>> the likes of the AS registers if there was a way to define a "block" of 
>> registers and then use an array of blocks. Something vaguely like this 
>> (excuse the poor Rust):
>>
>> register_block!(MMU_AS_CONTROL @ 0x2400[16 ; 64], "MMU Address Space registers" {
>> 	register!(TRANSTAB @ 0x0000, "Translation table base address" {
>> 		31:0	base as u32;
>> 	});
>> 	register!(MEMATTR @ 0x0008, "Memory attributes" {
>> 		7:0	attr0 as u8;
>> 		7:0	attr1 as u8;
>> 		// ...
>> 	});
>> 	// More registers
>> });
> 
> I can think of two ways to achieve something similar using the current
> patchset:
> 
> - As you mentioned, a set of non-contiguous register arrays. This should
>   work rather well, as you could just do
>   `MMU_AS_CONTROL_MEMATTR::read(bar, 4)` to read the `MMU_AS_CONTROL_MEMATTR`
>   register of the 5th instance, with compile-time bound validation. It's
>   not what register arrays are for originally, but it does the job.

Sadly we generally don't want a compile time index - the whole point is
that each address space is functionally the same, so the index (address
space ID) is going to be dynamic in the code. The disadvantage here is
that every register access will involve a bounds check - the compiler
might be able to optimise but the code will still have to deal with a
potential error from every access.

> - As a set of relative offset registers sharing the same group. This is
>   more in line with the idea of a register block, but it also means that
>   each instance needs to have its own type declared, which is a bit
>   cumbersome but can be mitigated with a macro. More inconvenient if the
>   fact that you cannot address using a simple number anymore...

Yeah this does sound cumbersome. Would you end up with a macro
duplicating the code 16 times (once for each type of the 16 register
blocks) and hoping the compiler can optimise it all back together?

> The idea of register blocks is interesting. I wonder how that would
> translate in terms of access to invididual registers, i.e. does the
> block end up just being a prefix into the full register name, or is it
> something else? From your example declaration I picture that accesses
> would look something like `MMU_AS_CONTROL[4]::MEMATTR::read(bar)`, which
> ngl looks great, but I also cannot think of a construct that would allow
> such a syntax... Happy to think more about it though.

Yes, that is the sort of syntax I was imagining, although I was hoping
you could do something like:

  let as = MMU_AS_CONTROL[as_id]::try_get(&bar)?;

  let memattr = as.MEMATTR.read(&bar);
  memattr.set_attr0(3).write(&bar);
  as.TRANSTAB.write(&bar, 0x1000);

Which I'm sure shows how little Rust I've written, but hopefully you get
the idea - only the first line is a try_xxx which can fail and takes the
address space ID from a variable and bounds checks it. The other
accesses we already know the bounds so there's no need to deal with
failure, and we don't have to consider the situation where MEMATTR is
written but the TRANSTAB write fails (which couldn't actually happen
with non-contiguous register arrays but the compiler wouldn't be able to
tell).

[And of course having written the above I realise that MEMATTR being
split up as separate named fields is also broken - we want to generate
it by looping over the fields.]

Steve

Powered by blists - more mailing lists