Message-Id: <D9ICBZ1DGGVU.2AKUD3X883TMB@nvidia.com>
Date: Mon, 28 Apr 2025 23:27:26 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
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>, "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>, "Jonathan Corbet"
 <corbet@....net>, "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs"
 <bskeggs@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, "Alistair Popple" <apopple@...dia.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 06/16] gpu: nova-core: define registers layout using
 helper macro

Hi Danilo,

On Tue Apr 22, 2025 at 7:29 PM JST, Danilo Krummrich wrote:
> On Sun, Apr 20, 2025 at 09:19:38PM +0900, Alexandre Courbot wrote:
>> Add the register!() macro, which defines a given register's layout and
>> provide bit-field accessors with a way to convert them to a given type.
>> This macro will allow us to make clear definitions of the registers and
>> manipulate their fields safely.
>> 
>> The long-term goal is to eventually move it to the kernel crate so it
>> can be used my other drivers as well, but it was agreed to first land it
>> into nova-core and make it mature there.
>> 
>> To illustrate its usage, use it to define the layout for the Boot0
>> register and use its accessors through the use of the convenience
>> with_bar!() macro, which uses Revocable::try_access() and converts its
>
> s/try_access/try_access_with/

Fixed, thanks.

>
>> returned Option into the proper error as needed.
>
> Using try_access_with() / with_bar! should be a separate patch.

Agreed - done.

>> +register!(Boot0@...0000000, "Basic revision information about the GPU";
>> +    3:0     minor_rev => as u8, "minor revision of the chip";
>> +    7:4     major_rev => as u8, "major revision of the chip";
>> +    28:20   chipset => try_into Chipset, "chipset model"
>
> Should we preserve the information that this is the combination of two register
> fields?

I've tried to reproduce what the current code did, but indeed according
to OpenRM these are two different fields, `architecture` and
`implementation`.

There's also more: `architecture` is a split field, with its MSB at a
different index from the rest. Right now the MSB is always 0 but the
lower bits are dangerously close to overflowing.

Thankfully, the macro doesn't prevent from extending its definition with
an extra impl block, so I've done just that to provide the correct
architecture as well as the `chipset` pseudo-field that will be the one
we use in the code anyway.

>
>> +);
>> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fa9bd6b932048113de997658b112885666e694c9
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/regs/macros.rs
>> @@ -0,0 +1,297 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Types and macros to define register layout and accessors.
>> +//!
>> +//! 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 macros 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. They also provide a builder type allowing to construct a register value to be
>> +//! written by combining valid values for its fields.
>> +
>> +/// Helper macro for the `register` macro.
>> +///
>> +/// Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`, `BitOr`,
>> +/// and conversion to regular `u32`).
>> +macro_rules! __reg_def_common {
>> +    ($name:ident $(, $type_comment:expr)?) => {
>> +        $(
>> +        #[doc=$type_comment]
>> +        )?
>> +        #[repr(transparent)]
>> +        #[derive(Clone, Copy, Default)]
>> +        pub(crate) struct $name(u32);
>> +
>> +        // TODO: should we display the raw hex value, then the value of all its fields?
>
> To me it seems useful to have both.

Agreed. However this macro has changed A LOT since this first revision,
and I have used TT bundling to make the rules a bit shorter. This makes
the details of the fields inaccessible from the rule that generates the
Debug implementation...

I'll probably try to rework that later, or when we move the macro into
the kernel crate - meanwhile, I hope we can be excused with just the hex
value.

>
>> +        impl ::core::fmt::Debug for $name {
>> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> +                f.debug_tuple(stringify!($name))
>> +                    .field(&format_args!("0x{0:x}", &self.0))
>> +                    .finish()
>> +            }
>> +        }
>> +
>> +        impl core::ops::BitOr for $name {
>> +            type Output = Self;
>> +
>> +            fn bitor(self, rhs: Self) -> Self::Output {
>> +                Self(self.0 | rhs.0)
>> +            }
>> +        }
>> +
>> +        impl From<$name> for u32 {
>
> Here and in a few more cases below: This needs the full path; also remember to
> use absolute paths everwhere.

Indeed, thanks for the reminder. I hope I got them all.

>> +            const MASK: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
>> +            const SHIFT: u32 = MASK.trailing_zeros();
>> +            let field = (self.0 & MASK) >> SHIFT;
>> +
>> +            $( field as $as_type )?
>> +            $(
>> +            // TODO: it would be nice to throw a compile-time error if $hi != $lo as this means we
>> +            // are considering more than one bit but returning a bool...
>
> Would the following work?
>
> 	const _: () = {
> 	   build_assert!($hi != $lo);
> 	   ()
> 	};

It does! I can even provide a useful error message. Added this check as
well as the one making sure that $hi >= $lo.

>> +macro_rules! register {
>> +    // Create a register at a fixed offset of the MMIO space.
>> +    (
>> +        $name:ident@...fset:expr $(, $type_comment:expr)?
>
> Can we use this as doc-comment?

Oops, I forgot to propagate it somehow. This is fixed.

>
>> +        $(; $hi:tt:$lo:tt $field:ident
>> +            $(=> as $as_type:ty)?
>> +            $(=> as_bit $bit_type:ty)?
>> +            $(=> into $type:ty)?
>> +            $(=> try_into $try_type:ty)?
>> +        $(, $field_comment:expr)?)* $(;)?
>> +    ) => {
>> +        __reg_def_common!($name);
>> +
>> +        #[allow(dead_code)]
>> +        impl $name {
>> +            #[inline]
>> +            pub(crate) fn read<const SIZE: usize, T: Deref<Target=Io<SIZE>>>(bar: &T) -> Self {
>
> Not necessarily a PCI bar, could be any I/O type.

Indeed, renamed to `io`.

>
>> +                Self(bar.read32($offset))
>> +            }
>> +
>> +            #[inline]
>> +            pub(crate) fn write<const SIZE: usize, T: Deref<Target=Io<SIZE>>>(self, bar: &T) {
>> +                bar.write32(self.0, $offset)
>> +            }
>> +
>> +            #[inline]
>> +            pub(crate) fn alter<const SIZE: usize, T: Deref<Target=Io<SIZE>>, F: FnOnce(Self) -> Self>(bar: &T, f: F) {
>> +                let reg = f(Self::read(bar));
>> +                reg.write(bar);
>> +            }
>> +        }
>> +
>> +        __reg_def_getters!($name; $( $hi:$lo $field $(=> as $as_type)? $(=> as_bit $bit_type)? $(=> into $type)? $(=> try_into $try_type)? $(, $field_comment)? );*);
>> +
>> +        __reg_def_setters!($name; $( $hi:$lo $field $(=> as $as_type)? $(=> as_bit $bit_type)? $(=> into $type)? $(=> try_into $try_type)? $(, $field_comment)? );*);
>> +    };
>> +
>> +    // Create a register at a relative offset from a base address.
>> +    (
>> +        $name:ident@...ffset:expr $(, $type_comment:expr)?
>> +        $(; $hi:tt:$lo:tt $field:ident
>> +            $(=> as $as_type:ty)?
>> +            $(=> as_bit $bit_type:ty)?
>> +            $(=> into $type:ty)?
>> +            $(=> try_into $try_type:ty)?
>> +        $(, $field_comment:expr)?)* $(;)?
>> +    ) => {
>
> I assume this is for cases where we have multiple instances of the same
> controller, engine, etc. I think it would be good to add a small example for
> this one too.

I'll add one.

You probably won't recognize this macro in its next revision. I've
finally read the little book of Rust macros and hopefully it is looking
a bit better - the definition of register fields notable should feel
more natural. All in all I think it is definitely better, but that
doesn't necessarily means it will be easier to review. ^_^;

One note, as agreed on Zulip I will rename all the register names to
capital snake case and disable the `camel_case_types` lint on the `regs`
module, so we use the exact same names as OpenRM. I will also make sure
that the names of the fields match (but will keep the accessors in
non-capital snake case).

In parallel I am also prototyping another design based on ZST constants.
If it works it would allow a few more things like register arrays, a
more natural way to perform I/O, and would remove the naming convention
issues since registers would be accessed by constants which should be
named in capital snake-case anyway. My hope is that this version will be
the one we can use in the kernel crate, but I don't think it will be
ready before a couple of cycles at least (if it works at all), so in the
meantime let's keep refining this one.

Cheers,
Alex.

Powered by blists - more mailing lists