<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div>On Wed, 2024-04-10 at 13:30 +0200, Danilo Krummrich wrote:</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>On Tue, Apr 09, 2024 at 06:15:52PM -0500, Timur Tabi wrote:</pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Add the NVreg_RegistryDwords command line parameter, which allows</pre>
<pre>specifying additional registry keys to be sent to GSP-RM. This</pre>
<pre>allows additional configuration, debugging, and experimentation</pre>
<pre>with GSP-RM, which uses these keys to alter its behavior.</pre>
<pre><br></pre>
<pre>Note that these keys are passed as-is to GSP-RM, and Nouveau does</pre>
<pre>not parse them. This is in contrast to the Nvidia driver, which may</pre>
<pre>parse some of the keys to configure some functionality in concert with</pre>
<pre>GSP-RM. Therefore, any keys which also require action by the driver</pre>
<pre>may not function correctly when passed by Nouveau. Caveat emptor.</pre>
<pre><br></pre>
<pre>The name and format of NVreg_RegistryDwords is the same as used by</pre>
<pre>the Nvidia driver, to maintain compatibility.</pre>
<pre><br></pre>
<pre>Signed-off-by: Timur Tabi <<a href="mailto:ttabi@nvidia.com">ttabi@nvidia.com</a>></pre>
<pre>---</pre>
<pre>v5:</pre>
<pre>Add REGISTRY_MAX_KEY_LENGTH</pre>
<pre>registry_list_entry.key is now char[64] instead of char *</pre>
<pre>use strnlen instead of strlen</pre>
<pre>removed some debug printks</pre>
<pre>fixed most checkpatch complaints</pre>
<pre>rebased to drm-fixes</pre>
</blockquote>
<pre><br></pre>
<pre>This patch seems to be based on drm-misc-fixes instead. For this patch the</pre>
<pre>correct target branch would be drm-misc-next though.</pre>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>You can, additionally, use '--base' when running git format-patch. This will</pre>
<pre>include the hash of the base commit.</pre>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+struct registry_list_entry {</pre>
<pre>+ struct list_head head;</pre>
<pre>+ enum registry_type type;</pre>
<pre>+ size_t klen;</pre>
<pre>+ size_t vlen;</pre>
<pre>+ char key[REGISTRY_MAX_KEY_LENGTH] __counted_by(klen);</pre>
</blockquote>
<pre><br></pre>
<pre>Now that this is an array, we should remove the __counted_by() annotation.</pre>
</blockquote>
<div><br>
</div>
<div>Ok.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+ u32 dword; /* TYPE_DWORD */</pre>
<pre>+ u8 binary[] __counted_by(vlen); /* TYPE_BINARY or TYPE_STRING */</pre>
</blockquote>
<pre><br></pre>
<pre>NIT: Can't we put these two into a union?</pre>
</blockquote>
<div><br>
</div>
<div>Sure.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+static int add_registry(struct nvkm_gsp *gsp, const char *key,</pre>
<pre>+ enum registry_type type, const void *data, size_t length)</pre>
<pre>+{</pre>
<pre>+ struct registry_list_entry *reg;</pre>
<pre>+ size_t nlen = strnlen(key, REGISTRY_MAX_KEY_LENGTH) + 1;</pre>
</blockquote>
<pre><br></pre>
<pre>Guess the only reason for strnlen() here is that you want to stop counting once</pre>
<pre>you exceed REGISTRY_MAX_KEY_LENGTH anyway, right?</pre>
</blockquote>
<div><br>
</div>
<div>Exactly. </div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+ size_t vlen; /* value length, non-zero if binary or string */</pre>
<pre>+</pre>
<pre>+ if (nlen > REGISTRY_MAX_KEY_LENGTH)</pre>
<pre>+ return -EFBIG;</pre>
</blockquote>
<pre><br></pre>
<pre>Still prefer EINVAL, EFBIG doesn't really apply here.</pre>
</blockquote>
<div><br>
</div>
<div>I knew I was forgetting something.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+ vlen = (type == REGISTRY_TABLE_ENTRY_TYPE_DWORD) ? 0 : length;</pre>
<pre>+</pre>
<pre>+ reg = kmalloc(sizeof(*reg) + vlen, GFP_KERNEL);</pre>
<pre>+ if (!reg)</pre>
<pre>+ return -ENOMEM;</pre>
<pre>+</pre>
<pre>+ switch (type) {</pre>
<pre>+ case REGISTRY_TABLE_ENTRY_TYPE_DWORD:</pre>
<pre>+ reg->dword = *(const u32 *)(data);</pre>
<pre>+ break;</pre>
<pre>+ case REGISTRY_TABLE_ENTRY_TYPE_BINARY:</pre>
<pre>+ case REGISTRY_TABLE_ENTRY_TYPE_STRING:</pre>
<pre>+ memcpy(reg->binary, data, length);</pre>
</blockquote>
<pre><br></pre>
<pre>Prefer to use vlen here...</pre>
</blockquote>
<div><br>
</div>
<div>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 ...</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>+ break;</pre>
<pre>+ default:</pre>
<pre>+ nvkm_error(&gsp->subdev, "unrecognized registry type %u for '%s'\n",</pre>
<pre>+ type, key);</pre>
<pre>+ kfree(reg);</pre>
<pre>+ return -EINVAL;</pre>
<pre>+ }</pre>
<pre>+</pre>
<pre>+ memcpy(reg->key, key, nlen);</pre>
<pre>+ reg->klen = nlen;</pre>
<pre>+ reg->vlen = length;</pre>
</blockquote>
<pre><br></pre>
<pre>...and here.</pre>
</blockquote>
<div><br>
</div>
<div>... it can't be vlen here because the value has to be '4' for dwords, and 'vlen' is 0 for dwords.</div>
<div><br>
</div>
<div><span></span></div>
</body>
</html>