Timur Tabi
2025-Sep-22 19:10 UTC
[PATCH v2 07/10] gpu: nova-core: gsp: Create RM registry and sysinfo commands
On Mon, 2025-09-22 at 21:30 +1000, Alistair Popple wrote:> +const GSP_REGISTRY_NUM_ENTRIES: usize = 2; > +struct RegistryEntry { > + key: &'static str, > + value: u32, > +}Probably should add a comment saying that although GSP-RM technically supports strings as values, we don't intend to use that feature. You just have REGISTRY_TABLE_ENTRY_TYPE_DWORD defined without any explanation that there are other entry types. Maybe REGISTRY_TABLE_ENTRY_TYPE_DWORD should be an enum instead of a const.> +pub(crate) fn build_registry(cmdq: &mut GspCmdq, bar: &Bar0) -> Result { > +??? let registry = RegistryTable { > +??????? entries: [ > +??????????? RegistryEntry { > +??????????????? key: "RMSecBusResetEnable", > +??????????????? value: 1, > +??????????? }, > +??????????? RegistryEntry { > +??????????????? key: "RMForcePcieConfigSave", > +??????????????? value: 1, > +??????????? }, > +??????? ], > +??? };You might want to add another field to RegistryEntry that adds documentation for each of these files? Nouveau documents them as comments. Also, you're missing RMDevidCheckIgnore. See https://lore-kernel.gnuweeb.org/nouveau/c34ccc23-5550-4f26-af66-47979afd197e at kernel.org/T/
Alistair Popple
2025-Sep-23 04:40 UTC
[PATCH v2 07/10] gpu: nova-core: gsp: Create RM registry and sysinfo commands
On 2025-09-23 at 05:10 +1000, Timur Tabi <ttabi at nvidia.com> wrote...> On Mon, 2025-09-22 at 21:30 +1000, Alistair Popple wrote: > > > > +const GSP_REGISTRY_NUM_ENTRIES: usize = 2; > > +struct RegistryEntry { > > + key: &'static str, > > + value: u32, > > +} > > Probably should add a comment saying that although GSP-RM technically supports strings as values, we > don't intend to use that feature. You just have REGISTRY_TABLE_ENTRY_TYPE_DWORD defined without any > explanation that there are other entry types.I think at some point we will want to extend this to support setting arbitrary registry values.> Maybe REGISTRY_TABLE_ENTRY_TYPE_DWORD should be an enum instead of a const.Yeah, although in this case it comes from the bindings so it's not really possible until we can get some richer generation of them.> > +pub(crate) fn build_registry(cmdq: &mut GspCmdq, bar: &Bar0) -> Result { > > +??? let registry = RegistryTable { > > +??????? entries: [ > > +??????????? RegistryEntry { > > +??????????????? key: "RMSecBusResetEnable", > > +??????????????? value: 1, > > +??????????? }, > > +??????????? RegistryEntry { > > +??????????????? key: "RMForcePcieConfigSave", > > +??????????????? value: 1, > > +??????????? }, > > +??????? ], > > +??? }; > > You might want to add another field to RegistryEntry that adds documentation for each of these > files? Nouveau documents them as comments.Yeah, good idea. In general the comments in this series are probably lacking so will update for v3.> Also, you're missing RMDevidCheckIgnore. See > https://lore-kernel.gnuweeb.org/nouveau/c34ccc23-5550-4f26-af66-47979afd197e at kernel.org/T/Thanks