Message-ID: <2CF462DB-D2C8-473F-9D70-522E6AFEDCE4@nvidia.com>
Date: Thu, 16 Oct 2025 19:28:55 +0000
From: Joel Fernandes <joelagnelf@...dia.com>
To: Yury Norov <yury.norov@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"dakr@...nel.org" <dakr@...nel.org>, Alexandre Courbot <acourbot@...dia.com>,
	Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
 Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
	<gary@...yguo.net>, "bjorn3_gh@...tonmail.com" <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>, 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>, John Hubbard
	<jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
	"joel@...lfernandes.org" <joel@...lfernandes.org>, Elle Rhumsaa
	<elle@...thered-steel.dev>, Daniel Almeida <daniel.almeida@...labora.com>,
	"nouveau@...ts.freedesktop.org" <nouveau@...ts.freedesktop.org>, Edwin Peer
	<epeer@...dia.com>
Subject: Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific
 code from register! into new macro



> On Oct 16, 2025, at 1:48 PM, Yury Norov <yury.norov@...il.com> wrote:
> 
> On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
>> Move the bitfield-specific code from the register macro into a new macro
>> called bitfield. This will be used to define structs with bitfields,
>> similar to C language.
> 
> Can you please fix line length issues before v8?
> 
> $ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
>      1 118
>      1 116
>      1 113
>      1 109
>      1 105
>      1 103

That is intentional. I will look again but long lines can be a matter of style
and if wrapping effects readability then we do not want to do that. That is
why it is a checkpatch warning not an error. We have to look it case by case.

>      ...
> 
>> Reviewed-by: Elle Rhumsaa <elle@...thered-steel.dev>
>> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
>> Reviewed-by: Edwin Peer <epeer@...dia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
>> ---
>> drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
>> drivers/gpu/nova-core/nova_core.rs   |   3 +
>> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>> 3 files changed, 332 insertions(+), 249 deletions(-)
>> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> 
> ...
> 
>> +///
>> +/// bitfield! {
>> +///     struct ControlReg {
>> +///         3:0 mode as u8 ?=> Mode;
>> +///         7:7 state as bool => State;
>> +///     }
>> +/// }
> 
> This notation is really unwelcome this days. It may be OK for a random
> macro in some local driver, but doesn't really work for a global basic
> data type:
> 
> https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/
> 
> I've already shared this link with you, and shared my concern.
> 
> I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
> and GENMASK() derives verilog or hardware specs popular notations. But
> software people prefer lo:hi. I'm probably OK if you choose C-style
> start:nbits, if you prefer. But let's stop this hi:lo early, please.
> 
> Let me quote Linus from the link above:
> 
>  It does "high, low", which is often very unintuitive, and in fact the
>  very commit that introduced this thing from hell had to convert the
>  sane "low,high" cases to the other way around.

I agree with Linus but I disagree with comparing it with these macros.
I agree with Linus it is oddly unreadable when used as function parameters.
But that is a different syntax. Over here we are using colons with sufficient whitespace around hi:lo.

> 
>  See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
>  macro"), and notice how ALMOST ALL use cases were switched around to
>  the illogical "high,low" format by that introductory phase.

Again this is different syntax so assuming that Linus will not be ok with it is a stretch IMO.

The rust macros here have their own syntax and are not function parameters.

> 
>  And yes, I understand why that person did it: many datasheets show
>  bits in a register graphically, and then you see that "high .. low"
>  thing in a rectangle that describes the register, and that ordering
>  them makes 100% sense IN THAT CONTEXT.
> 
>  But it damn well does not make sense in most other contexts.
> 
>  In fact, even in the context of generating mask #defines, it actually
>  reads oddly, because you end up having things like
> 
>    /* Status register (SR) */
>    #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
>    #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
>    #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
>    #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
>    #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

Sure this is super odd indeed. But again not apples to apples here.

thanks,

 - Joel


> 
>  ...
> 
> Now compare it to what we've got in nova right now:
> 
>  register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
>      3:0     minor_revision as u8, "Minor revision of the chip";
>      7:4     major_revision as u8, "Major revision of the chip";
>      8:8     architecture_1 as u8, "MSB of the architecture";
>      23:20   implementation as u8, "Implementation version of the architecture";
>      28:24   architecture_0 as u8, "Lower bits of the architecture";
>  });
> 
> There's so far 36 thousands of GENMASK()s in the kernel, and only 67
> register!()s. It's a separate topic what to do with the GENMASK()
> codebase. But now that you do this massive refactoring for the
> register!() macro, let's convert those 67 users to the lo:hi notation.
> 
> As a side note, for GENMASKs, I tried this trick:
> 
>        #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b))
> 
> It works, but bloats defconfig kernel for another 1K. I don't think it
> would add to readability on both C and rust sides.
> 
> Thanks,
> Yury

Powered by blists - more mailing lists