[PATCH] [v5] nouveau: add command-line GSP-RM registry support

[Timur Tabi]

On Wed, 2024-04-10 at 13:30 +0200, Danilo Krummrich wrote:

On Tue, Apr 09, 2024 at 06:15:52PM -0500, Timur Tabi wrote:

Add the NVreg_RegistryDwords command line parameter, which allows

specifying additional registry keys to be sent to GSP-RM.  This

allows additional configuration, debugging, and experimentation

with GSP-RM, which uses these keys to alter its behavior.


Note that these keys are passed as-is to GSP-RM, and Nouveau does

not parse them.  This is in contrast to the Nvidia driver, which may

parse some of the keys to configure some functionality in concert with

GSP-RM.  Therefore, any keys which also require action by the driver

may not function correctly when passed by Nouveau.  Caveat emptor.


The name and format of NVreg_RegistryDwords is the same as used by

the Nvidia driver, to maintain compatibility.


Signed-off-by: Timur Tabi <ttabi at nvidia.com<mailto:ttabi at nvidia.com>>

---

v5:

Add REGISTRY_MAX_KEY_LENGTH

registry_list_entry.key is now char[64] instead of char *

use strnlen instead of strlen

removed some debug printks

fixed most checkpatch complaints

rebased to drm-fixes


This patch seems to be based on drm-misc-fixes instead. For this patch the

correct target branch would be drm-misc-next though.

Ok, but then you need to pick up Kees' patch "nouveau/gsp: Avoid addressing beyond end of rpc->entries" because I expect my patch to be applied on top of it.  That's why I chose that branch.


You can, additionally, use '--base' when running git format-patch. This will

include the hash of the base commit.

I will take drm-misc-next, add Kees' patch, and rebase onto that.  I'll use --base but I'm not sure whether it will print something useful at this point.


+struct registry_list_entry {

+       struct list_head head;

+       enum registry_type type;

+       size_t klen;

+       size_t vlen;

+       char key[REGISTRY_MAX_KEY_LENGTH] __counted_by(klen);


Now that this is an array, we should remove the __counted_by() annotation.

Ok.


+       u32 dword;                              /* TYPE_DWORD */

+       u8 binary[] __counted_by(vlen); /* TYPE_BINARY or TYPE_STRING */


NIT: Can't we put these two into a union?

Sure.


+static int add_registry(struct nvkm_gsp *gsp, const char *key,

+                       enum registry_type type, const void *data, size_t length)

+{

+       struct registry_list_entry *reg;

+       size_t nlen = strnlen(key, REGISTRY_MAX_KEY_LENGTH) + 1;


Guess the only reason for strnlen() here is that you want to stop counting once

you exceed REGISTRY_MAX_KEY_LENGTH anyway, right?

Exactly.



+       size_t vlen; /* value length, non-zero if binary or string */

+

+       if (nlen > REGISTRY_MAX_KEY_LENGTH)

+               return -EFBIG;


Still prefer EINVAL, EFBIG doesn't really apply here.

I knew I was forgetting something.


+       vlen = (type == REGISTRY_TABLE_ENTRY_TYPE_DWORD) ? 0 : length;

+

+       reg = kmalloc(sizeof(*reg) + vlen, GFP_KERNEL);

+       if (!reg)

+               return -ENOMEM;

+

+       switch (type) {

+       case REGISTRY_TABLE_ENTRY_TYPE_DWORD:

+               reg->dword = *(const u32 *)(data);

+               break;

+       case REGISTRY_TABLE_ENTRY_TYPE_BINARY:

+       case REGISTRY_TABLE_ENTRY_TYPE_STRING:

+               memcpy(reg->binary, data, length);


Prefer to use vlen here...

I'll change it, but I think 'length' is more correct, since it's supposed to be the actual size of the data, whereas 'vlen' is just used to determine the size of the structure.  Especially since ...



+               break;

+       default:

+               nvkm_error(&gsp->subdev, "unrecognized registry type %u for '%s'\n",

+                          type, key);

+               kfree(reg);

+               return -EINVAL;

+       }

+

+       memcpy(reg->key, key, nlen);

+       reg->klen = nlen;

+       reg->vlen = length;


...and here.

... it can't be vlen here because the value has to be '4' for dwords, and 'vlen' is 0 for dwords.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20240415/74a70f18/attachment.htm>