On Mon, 2024-03-04 at 13:57 +0100, Danilo Krummrich wrote:
>
> Thanks for sending a new version of the patch.
>
> Please make sure that for future patches you include a changelog, such that
> it's easier for reviewers to keep track of what has changed.
Will do.
> > .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 6 +
> > .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 375 ++++++++++++++++--
> > 2 files changed, 357 insertions(+), 24 deletions(-)
>
> There are a few issues pointed out by checkpatch.pl, that I think should be fixed.
checkpatch showed me nothing:
$ scripts/checkpatch.pl 0001-v4-nouveau-add-command-line-GSP-RM-registry-
support.patch
total: 0 errors, 0 warnings, 420 lines checked
0001-v4-nouveau-add-command-line-GSP-RM-registry-support.patch has no
obvious style problems and is ready for submission.
> >
> > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > index 6f5d376d8fcc..3fbc57b16a05 100644
> > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > @@ -211,6 +211,12 @@ struct nvkm_gsp {
> > struct mutex mutex;;
> > struct idr idr;
> > } client_id;
> > +
> > + /* A linked list of registry items. The registry RPC will be
> > built from it. */
> > + struct list_head registry_list;
> > +
> > + /* The size of the registry RPC */
> > + size_t registry_rpc_size;
> > };
> >
> > static inline bool
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> > index 1c46e3f919be..40757a21e150 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> > @@ -54,6 +54,8 @@
> > #include <nvrm/535.113.01/nvidia/kernel/inc/vgpu/rpc_global_enums.h>
> >
> > #include <linux/acpi.h>
> > +#include <linux/ctype.h>
> > +#include <linux/parser.h>
> >
> > #define GSP_MSG_MIN_SIZE GSP_PAGE_SIZE
> > #define GSP_MSG_MAX_SIZE GSP_PAGE_MIN_SIZE * 16
> > @@ -1082,51 +1084,376 @@ r535_gsp_rpc_unloading_guest_driver(struct
> > nvkm_gsp *gsp, bool suspend)
> > return nvkm_gsp_rpc_wr(gsp, rpc, true);
> > }
> >
> > +enum registry_type {
> > + REGISTRY_TABLE_ENTRY_TYPE_DWORD = 1, /* 32-bit unsigned
> > integer */
> > + REGISTRY_TABLE_ENTRY_TYPE_BINARY = 2, /* Binary blob */
> > + REGISTRY_TABLE_ENTRY_TYPE_STRING = 3, /* Null-terminated string
> > */
> > +};
> > +
> > +/**
> > + * registry_list_entry - linked list member for a registry key/value
> > + * @head: list_head struct
> > + * @type: dword, binary, or string
> > + * @klen: the length of name of the key
> > + * @vlen: the length of the value
> > + * @v.dword: the data, if REGISTRY_TABLE_ENTRY_TYPE_DWORD
> > + * @v.binary: the data, if TYPE_BINARY or TYPE_STRING
> > + * @key: the key name
> > + *
> > + * Every registry key/value is represented internally by this struct.
> > + *
> > + * Type DWORD is a simple 32-bit unsigned integer, and its value is
> > stored in
> > + * v.dword.
> > + *
> > + * Types BINARY and STRING are variable-length binary blobs. The only
> > real
> > + * difference between BINARY and STRING is that STRING is null-
> > terminated and
> > + * is expected to contain only printable characters.
> > + *
> > + * To avoid having to kmalloc multiple objects, the value for BINARY
> > and
> > + * STRING is appended after the key[] in memory, and v.binary just
> > points to
> > + * that block.
> > + *
> > + * Note: it is technically possible to have multiple keys with the same
> > name
> > + * but different types, but this is not useful since GSP-RM expects
> > keys to
> > + * have only one specific type.
> > + */
> > +struct registry_list_entry {
> > + struct list_head head;
> > + enum registry_type type;
> > + size_t klen;
> > + size_t vlen;
> > + union {
> > + u32 dword; /* TYPE_DWORD */
> > + void *binary; /* TYPE_BINARY or TYPE_STRING */
> > + } v;
> > + char key[] __counted_by(klen);
> > +};
> > +
> > +/**
> > + * add_registry -- adds a registry entry
> > + * @gsp: gsp pointer
> > + * @key: name of the registry key
> > + * @type: type of data
> > + * @data: pointer to value
> > + * @length: size of data, in bytes
> > + *
> > + * Adds a registry key/value pair to the registry database.
> > + *
> > + * This function collects the registry information in a linked list.
> > After
> > + * all registry keys have been added, build_registry() is used to
> > create the
> > + * RPC data structure.
> > + *
> > + * registry_rpc_size is a running total of the size of all registry
> > keys.
> > + * It's used to avoid an O(n) calculation of the size when the RPC is
> > built.
> > + *
> > + * Returns 0 on success, or negative error code on error.
> > + */
> > +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 = strlen(key) + 1;
> > + size_t vlen; /* value length, non-zero if binary or string */
> > +
> > + /* Set an arbitrary limit to avoid problems with broken command
> > lines */
> > + if (nlen > 64)
> > + return -EFBIG;
> > +
> > + vlen = (type == REGISTRY_TABLE_ENTRY_TYPE_DWORD) ? 0 : length;
> > +
> > + reg = kmalloc(sizeof(struct registry_list_entry) + nlen + vlen,
> > GFP_KERNEL);
> > + if (!reg)
> > + return -ENOMEM;
> > +
> > + switch (type) {
> > + case REGISTRY_TABLE_ENTRY_TYPE_DWORD:
> > + reg->v.dword = *(const u32 *)(data);
> > + nvkm_debug(&gsp->subdev, "adding GSP-RM registry
> > '%s=%u'\n",
> > + key, reg->v.dword);
> > + break;
> > + case REGISTRY_TABLE_ENTRY_TYPE_BINARY:
> > + case REGISTRY_TABLE_ENTRY_TYPE_STRING:
> > + reg->v.binary = (void *)®->key + nlen;
>
> I see, you just append the binary at the end of the struct. However, that
> messes with
> the __counted_by() of reg->key. I think we should do a separate allocation
> for binary
> and string types.
Since I arbitrarily restrict the key name to 64 characters, and since this
is a small temporary struct, I can do this:
struct registry_list_entry {
struct list_head head;
enum registry_type type;
size_t klen;
size_t vlen;
u32 dword; /* if TYPE_DWORD */
char key[64] __counted_by(klen);
uint8_t value[] __counted_by(vlen); /* if TYPE_STRING or BINARY */
};
> > + memcpy(reg->v.binary, data, length);
> > + vlen = length;
> > + if (type == REGISTRY_TABLE_ENTRY_TYPE_BINARY)
>
> If you want a debug print for each type, you could move them to a separate
> function with
> it's own switch case.
I'll just remove them.
