Re: [PATCH 3/9] gpu: nova-core: gsp: expose GSP-RM internal client and subdevice handles

[John Hubbard <jhubbard@nvidia.com>]

On 3/9/26 7:17 PM, Joel Fernandes wrote:
> Hi John,
> 
>> On Mar 9, 2026, at 8:06 PM, John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 3/9/26 4:41 PM, Joel Fernandes wrote:
>>>>> On Mar 9, 2026, at 5:22 PM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>>> On Fri, Feb 27, 2026 at 09:32:08PM +0900, Eliot Courtney wrote:
>>>>> Expose the `hInternalClient` and `hInternalSubdevice` handles. These are
>>>>> needed for RM control calls.
>>>>>
>>>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>>>> ---
>>>>> drivers/gpu/nova-core/gsp/commands.rs    | 16 ++++++++++++++++
>>>>> drivers/gpu/nova-core/gsp/fw/commands.rs | 10 ++++++++++
>>>>> 2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
>>>>> index 4740cda0b51c..2cadfcaf9a8a 100644
>>>>> --- a/drivers/gpu/nova-core/gsp/commands.rs
>>>>> +++ b/drivers/gpu/nova-core/gsp/commands.rs
>>>>> @@ -197,6 +197,8 @@ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>>>>> /// The reply from the GSP to the [`GetGspInfo`] command.
>>>>> pub(crate) struct GetGspStaticInfoReply {
>>>>>    gpu_name: [u8; 64],
>>>>> +    h_client: u32,
>>>>> +    h_subdevice: u32,
>>>>
>>>> I would rather have more descriptive names please. 'client_handle',
>>
>> Maybe it's better to mirror the Open RM names, which are ancient and
>> well known in those circles. Changing them at this point is probably
>> going to result in a slightly worse situation, because there are
>> probably millions of lines of code out there that use the existing
>> nomenclature.
> 
> I have to disagree a bit here. Saying h_ in code is a bit meaningless:
> there is no mention of the word "handle" anywhere near these fields.
> h_ could mean "higher", "hardware", or any number of things. The only
> reason I know it means "handle" is because of expertise with Nvidia
> drivers. The `_handle` suffix is self-documenting; `h_` is not.

Maybe if we write h_client in the code, and "handle..." in a comment
above it? Or the other way around. Just to make the connection to
the Open RM client code that is out there.

> 
>>
>> However...
>>
>>>> 'subdevice_handle'. Also some explanation of what a client and a sub-device
>>>> mean somewhere in the comments or documentation would be nice.
>>
>> Yes, although I expect you can simply refer to some well known pre-
>> existing documentation from NVIDAI for that!
> 
> I apologize but I am a bit concerned with this approach because it feels we
> are drifting into black box dev without encouraging more code comments,
> documentation and cleaner code.
> 
> We need to make the driver as readable and well documented as possible, we
> do not want another Nouveau with magic numbers and magic variable names.
> 
> Very least I would expect at least one or two lines of code comments of
> what is a handle, what is a client, what is an internal client handle
> versus not. I guess I do not understand what is the hesitation?
> 
> Sure external documentation is good but to clarify, I am referring to a few
> code comments. That's not much to ask right?
> 

Sure, a short amount of comments would help, that sounds good.


> Elaborate documentation files in kernel can be optional but there is
> probably no harm in citing external references from in-kernel docs too. But
> again I was more concerned about code comments and variable names.
> 
>>
>>>
>>> Also just checking if we can have repr wrappers around the u32 for clients /
>>> handles.  These concepts are quite common in Nvidia drivers so we should
>>> probably create new types for them.
>>>
>>> And if we can please document the terminology, device, subset, clients handles
>>> etc. and add new Documentation/ entries even.
>>>
>>> Thoughts?
>>>
>>
>> This has already been done countless times by countless people I
>> think, and so we don't need to do it again. Just refer to existing
>> docs.
> 
> Not sure if you are referring to nova-core repr or docs, but the repr
> technique to wrap raw integers is pretty common in the firmware layer of
> nova-core.
> 
>>
>> btw, as an aside:
>>
>> I'm checking with our GSP firmware team to be sure, but my
>> understanding is that much of this is actually very temporary. Because
>> the GSP team does not want to continue on with this model in which
>> GSP has to maintain that kind of state: an internal hierarchy of
>> objects. Instead, they are hoping to move to an API in which nova
>> would directly refer to each object/item in GSP. And subdevice, in
> 
> Even if this is "temporary" we can't just say "Oh, I'll just add these
> legacy things and badly write them, because they're going anyway at some
> unpredictable point in the future". After all, this is the Linux kernel we
> are talking about :-). The bar is quite high.

I am not saying any such thing.

> 
>> particular, is an old SLI term that no one wants to keep around
>> either. It was an ugly hack in Open RM that took more than a decade
>> to recover from, by moving the SLI concept out to user space.
>>
>> So even though we should document what we're doing now, I would like
>> to also note that we suspect a certain amount of this will
>> disappear, to be replaced with a somewhat simpler API, in the
>> somewhat near future.
> 
> Sure, but client handles are a broader GPU driver concept even if this
> particular one is GSP-internal. We are certainly going to need a rust type
> to represent a client right? Other GPU drivers also have concept of
> clients. The point is not that `hInternalClient` represents a GPU user
> today, it may well be temporary as you note, but that using
> `#[repr(transparent)]` new types for raw u32 handles costs nothing and
> makes the code better and more readable. This pattern is already
> well-established in nova-core itself: see `PackedRegistryEntry` for example
> being a repr type. IMHO, there should be little reason that we need the
> struct to have magic u32 numbers in Rust code for concepts like "handles".
> 

We will debate this when it shows up, perhaps I should not have 
mentioned it, other than to remind Eliot to make it easy to delete.

> All I am saying is let us think this through before just doing the shortcut
> of using u32 for client handles, etc. Rust gives us rich types, lets use them.
> 

ohh, that's a whole other topic and idea. I wasn't going into that,
but feel free to explore if Rust can make it better.

thanks,
-- 
John Hubbard