<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div>On Mon, 2024-06-17 at 21:54 +0200, Danilo Krummrich wrote:</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Hi Timur,</pre>
<pre><br></pre>
<pre>thanks for the follow-up on this patch series.</pre>
<pre><br></pre>
<pre>On Wed, Jun 12, 2024 at 06:52:53PM -0500, Timur Tabi wrote:</pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>The LOGINIT, LOGINTR, LOGRM, and LOGPMU buffers are circular buffers</pre>
<pre>that have printf-like logs from GSP-RM and PMU encoded in them.</pre>
<pre><br></pre>
<pre>LOGINIT, LOGINTR, and LOGRM are allocated by Nouveau and their DMA</pre>
<pre>addresses are passed to GSP-RM during initialization. The buffers are</pre>
<pre>required for GSP-RM to initialize properly.</pre>
<pre><br></pre>
<pre>LOGPMU is also allocated by Nouveau, but its contents are updated</pre>
<pre>when Nouveau receives an NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT RPC from</pre>
<pre>GSP-RM. Nouveau then copies the RPC to the buffer.</pre>
<pre><br></pre>
<pre>The messages are encoded as an array of variable-length structures that</pre>
<pre>contain the parameters to an NV_PRINTF call. The format string and</pre>
<pre>parameter count are stored in a special ELF image that contains only</pre>
<pre>logging strings. This image is not currently shipped with the Nvidia</pre>
<pre>driver.</pre>
<pre><br></pre>
<pre>There are two methods to extract the logs.</pre>
<pre><br></pre>
<pre>OpenRM tries to load the logging ELF, and if present, parses the log</pre>
<pre>buffers in real time and outputs the strings to the kernel console.</pre>
<pre><br></pre>
<pre>Alternatively, and this is the method used by this patch, the buffers</pre>
<pre>can be exposed to user space, and a user-space tool (along with the</pre>
<pre>logging ELF image) can parse the buffer and dump the logs.</pre>
<pre><br></pre>
<pre>This method has the advantage that it allows the buffers to be parsed</pre>
<pre>even when the logging ELF file is not available to the user. However,</pre>
<pre>it has the disadvantage the debubfs entries need to remain until the</pre>
<pre>driver is unloaded.</pre>
<pre><br></pre>
<pre>The buffers are exposed via debugfs. If GSP-RM fails to initialize,</pre>
<pre>then Nouveau immediately shuts down the GSP interface. This would</pre>
<pre>normally also deallocate the logging buffers, thereby preventing the</pre>
<pre>user from capturing the debug logs.</pre>
<pre><br></pre>
<pre>To avoid this, introduce the keep-gsp-logging command line parameter.</pre>
<pre>If specified, and if at least one logging buffer has content, then</pre>
<pre>Nouveau will migrate these buffers into new debugfs entries that are</pre>
<pre>retained until the driver unloads.</pre>
<pre><br></pre>
<pre>An end-user can capture the logs using the following commands:</pre>
<pre><br></pre>
<pre> cp /sys/kernel/debug/dri/<path>/loginit loginit</pre>
<pre> cp /sys/kernel/debug/dri/<path>/logrm logrm</pre>
<pre> cp /sys/kernel/debug/dri/<path>/logintr logintr</pre>
<pre> cp /sys/kernel/debug/dri/<path>/logpmu logpmu</pre>
<pre><br></pre>
<pre>where <path> is the PCI ID of the GPU (e.g. 0000:65:00.0).</pre>
<pre><br></pre>
<pre>Since LOGPMU is not needed for normal GSP-RM operation, it is only</pre>
<pre>created if debugfs is available. Otherwise, the</pre>
<pre>NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT RPCs are ignored.</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>rebased to drm-misc-next</pre>
<pre>repaced nvkm_gsp_log with nvif_log</pre>
<pre>minor rearrangement of some code</pre>
<pre><br></pre>
<pre> drivers/gpu/drm/nouveau/include/nvif/log.h | 32 ++</pre>
<pre> .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 13 +</pre>
<pre> drivers/gpu/drm/nouveau/nouveau_drm.c | 19 +</pre>
<pre> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 360 +++++++++++++++++-</pre>
<pre> 4 files changed, 423 insertions(+), 1 deletion(-)</pre>
<pre> create mode 100644 drivers/gpu/drm/nouveau/include/nvif/log.h</pre>
<pre><br></pre>
<pre>diff --git a/drivers/gpu/drm/nouveau/include/nvif/log.h b/drivers/gpu/drm/nouveau/include/nvif/log.h</pre>
<pre>new file mode 100644</pre>
<pre>index 000000000000..c89ba85a701d</pre>
<pre>--- /dev/null</pre>
<pre>+++ b/drivers/gpu/drm/nouveau/include/nvif/log.h</pre>
<pre>@@ -0,0 +1,32 @@</pre>
<pre>+/* SPDX-License-Identifier: MIT */</pre>
<pre>+/* SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. */</pre>
<pre>+</pre>
<pre>+#ifndef __NVIF_LOG_H__</pre>
<pre>+#define __NVIF_LOG_H__</pre>
<pre>+</pre>
<pre>+/**</pre>
<pre>+ * nvif_log - structure for tracking logging buffers</pre>
<pre>+ * @head: list head</pre>
</blockquote>
<pre><br></pre>
<pre>What about "@entry: an entry in a list of struct nvif_logs".</pre>
</blockquote>
<div><br>
</div>
<div>Done.</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>+ * @shutdown: pointer to function to call to clean up</pre>
</blockquote>
<pre><br></pre>
<pre>I'd be a bit more specific and say that this frees all backing resources, such</pre>
<pre>as logging buffer, etc.</pre>
</blockquote>
<div><br>
</div>
<div>Done.</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>+ *</pre>
<pre>+ * Structure used to track logging buffers so that they can be cleaned up</pre>
<pre>+ * when the driver exits.</pre>
<pre>+ */</pre>
<pre>+struct nvif_log {</pre>
<pre>+ struct list_head head;</pre>
</blockquote>
<pre><br></pre>
<pre>I suggest to call this 'entry', since that's what it actually represents, and</pre>
<pre>entry in a list.</pre>
</blockquote>
<div><br>
</div>
<div>Oh yeah, I see what you mean now. Done.</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>+ void (*shutdown)(struct nvif_log *log);</pre>
<pre>+};</pre>
<pre>+</pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+/**</pre>
<pre>+ * gsp_logs - list of nvif_log GSP-RM logging buffers</pre>
<pre>+ *</pre>
<pre>+ * Head pointer to a a list of nvif_log buffers that is created for each GPU</pre>
<pre>+ * upon GSP shutdown if the "keep_gsp_logging" command-line parameter is</pre>
<pre>+ * specified. This is used to track the alternative debugfs entries for the</pre>
<pre>+ * GSP-RM logs.</pre>
<pre>+ */</pre>
<pre>+extern struct list_head gsp_logs;</pre>
</blockquote>
<pre><br></pre>
<pre>Better wrap this in a struct nvif_logs (or similar) and pass this down through</pre>
<pre>nvkm_device_pci_new() / nvkm_device_tegra_new() instead of relying on sharing</pre>
<pre>a global.</pre>
</blockquote>
<div><br>
</div>
<div>I'm still having difficulty understanding what you mean by this. I think you need to be more explicit in what you want.</div>
<div><br>
</div>
<div>struct nvif_logs</div>
<div>{</div>
<div><span class="Apple-tab-span" style="white-space:pre"></span>struct list_head logs;</div>
<div>} gsp_logs;</div>
<div><br>
</div>
<div>First, I don't understand what this gets me. It just wraps one struct inside another.</div>
<div><br>
</div>
<div>Should I add this to struct nvkm_device_func? And then do something like this in pci.c:</div>
<div><br>
</div>
<div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Ubuntu; font-size: 14.666667px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-tap-highlight-color: rgba(0, 0, 0, 0.4); -webkit-text-stroke-width: 0px; text-decoration: none;">
static struct nvif_logs gsp_logs;</div>
<div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Ubuntu; font-size: 14.666667px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-tap-highlight-color: rgba(0, 0, 0, 0.4); -webkit-text-stroke-width: 0px; text-decoration: none;">
<br>
</div>
<div>static const struct nvkm_device_func</div>
<div>nvkm_device_pci_func = {</div>
<div><br>
</div>
<div>...</div>
<div><span class="Apple-tab-span" style="white-space:pre"></span>.nvif_logs = &gsp_logs,</div>
<div>};</div>
<div><br>
</div>
<div>So nvkm_device_pci_new() calls nvkm_device_ctor(). <span style="font-size: 14.666667px;">nvkm_device_ctor() then calls the .ctor() for every subdevice via NVKM_LAYOUT_ONCE() and NVKM_LAYOUT_INST(). This is where I get lost, because I don't see how I
make sure only the GSP constructor tries to initialize gsp_logs.</span></div>
<div style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-family: Ubuntu; font-size: 14.666667px; font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; text-align: start; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-tap-highlight-color: rgba(0, 0, 0, 0.4); -webkit-text-stroke-width: 0px; text-decoration: none;">
<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><br></pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+/**</pre>
<pre>+ * gsp_logs - list of GSP debugfs logging buffers</pre>
<pre>+ */</pre>
<pre>+LIST_HEAD(gsp_logs);</pre>
</blockquote>
<pre><br></pre>
<pre>Better wrap this in a NVIF_LOGS_DECLARE() macro.</pre>
</blockquote>
<div><br>
</div>
<div>Like this?</div>
<div><br>
</div>
<div>#define NVIF_LOGS_DECLARE(x) LIST_HEAD(x)</div>
<div><br>
</div>
<div>Do you want a macro to replace this as well?</div>
<div><br>
</div>
<div>extern struct list_head gsp_logs;</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>+#endif</pre>
<pre>+</pre>
<pre> static u64</pre>
<pre> nouveau_pci_name(struct pci_dev *pdev)</pre>
<pre> {</pre>
<pre>@@ -1446,6 +1454,17 @@ nouveau_drm_exit(void)</pre>
<pre> #endif</pre>
<pre> if (IS_ENABLED(CONFIG_DRM_NOUVEAU_SVM))</pre>
<pre> mmu_notifier_synchronize();</pre>
<pre>+</pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+ if (!list_empty(&gsp_logs)) {</pre>
<pre>+ struct nvif_log *log, *n;</pre>
<pre>+</pre>
<pre>+ list_for_each_entry_safe(log, n, &gsp_logs, head) {</pre>
<pre>+ /* shutdown() should also delete the log entry */</pre>
<pre>+ log->shutdown(log);</pre>
<pre>+ }</pre>
<pre>+ }</pre>
<pre>+#endif</pre>
</blockquote>
<pre><br></pre>
<pre>Please move this to include/nvif/log.h as nvif_log_shutdown().</pre>
</blockquote>
<div><br>
</div>
<div>Sure, but I don't understand why. This code will only be called in nouveau_drm_exit().</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>+/**</pre>
<pre>+ * r535_gsp_msg_libos_print - capture log message from the PMU</pre>
<pre>+ * @priv: gsp pointer</pre>
<pre>+ * @fn: function number (ignored)</pre>
<pre>+ * @repv: pointer to libos print RPC</pre>
<pre>+ * @repc: message size</pre>
<pre>+ *</pre>
<pre>+ * See _kgspRpcUcodeLibosPrint</pre>
</blockquote>
<pre><br></pre>
<pre>What is this reference?</pre>
</blockquote>
<div><br>
</div>
<div>It's in OpenRM. Pretty much all of the "See" commands are references to the corresponding function in OpenRM".</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>+ */</pre>
<pre>+static int r535_gsp_msg_libos_print(void *priv, u32 fn, void *repv, u32 repc)</pre>
<pre>+{</pre>
<pre>+ struct nvkm_gsp *gsp = priv;</pre>
<pre>+ struct nvkm_subdev *subdev = &gsp->subdev;</pre>
<pre>+ struct {</pre>
<pre>+ u32 ucodeEngDesc;</pre>
<pre>+ u32 libosPrintBufSize;</pre>
<pre>+ u8 libosPrintBuf[];</pre>
</blockquote>
<pre><br></pre>
<pre>Why are those camelCase? Please use snake_case instead.</pre>
</blockquote>
<div><br>
</div>
<div>Sure. They match the names from OpenRM (see g_rpc-structures.h). I'll change them, though. When I copy a struct directly from OpenRM, I prefer to keep it as identical as possible to make it easier to see the connection.</div>
<div><br>
</div>
<div>I'll move this to a distinct struct and rename the fields.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre><br></pre>
<pre>Also, please add a few notes on the fields GSP returns to us here. Admittedly,</pre>
<pre>two of those seem rather obvious - a log buffer and a size - but maybe we can</pre>
<pre>write down whether there is some maximum or minimum size? Can we get called with</pre>
<pre>a zero-size buffer?</pre>
<pre><br></pre>
<pre>What about 'ucodeEngDesc'? This one seems to behave more like a register value,</pre>
<pre>can we document the "register layout" for this one?</pre>
</blockquote>
<div><br>
</div>
<div>I've added some documentation. Basically, the field is divided into a "class ID" and an "instance ID". I'm guessing the instance ID is just a 0-based index for the engine. The class ID is just some seemingly random number that distinguishes the various
engines. Building RM involves a lot of pre-processing with config files to generate much of the code that is compiled, and these class IDs are one example.</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>+ i_size_write(d_inode(dir_init), gsp->blob_init.size);</pre>
<pre>+ i_size_write(d_inode(dir_intr), gsp->blob_intr.size);</pre>
<pre>+ i_size_write(d_inode(dir_rm), gsp->blob_rm.size);</pre>
<pre>+ i_size_write(d_inode(dir_pmu), gsp->blob_pmu.size);</pre>
<pre>+</pre>
<pre>+ r535_gsp_msg_ntfy_add(gsp, 0x0000100C, r535_gsp_msg_libos_print, gsp);</pre>
</blockquote>
<pre><br></pre>
<pre>Please, don't add random magic values. Add a useful explanation instead that</pre>
<pre>also new contributors are able to make sense of.</pre>
</blockquote>
<div><br>
</div>
<div>Ah, I can just use the macro that's already defined in rpc_global_enums.h.</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>+</pre>
<pre>+ nvkm_debug(&gsp->subdev, "created debugfs GSP-RM logging entries\n");</pre>
<pre>+</pre>
<pre>+ if (keep_gsp_logging) {</pre>
<pre>+ nvkm_warn(&gsp->subdev,</pre>
<pre>+ "logging buffers will be retained on failure\n");</pre>
<pre>+ }</pre>
<pre>+</pre>
<pre>+ return;</pre>
<pre>+</pre>
<pre>+error:</pre>
<pre>+ debugfs_remove(dir_init);</pre>
<pre>+ debugfs_remove(dir_intr);</pre>
<pre>+ debugfs_remove(dir_rm);</pre>
<pre>+}</pre>
<pre>+</pre>
<pre>+#endif</pre>
<pre>+</pre>
<pre> static inline u64</pre>
<pre> r535_gsp_libos_id8(const char *name)</pre>
<pre> {</pre>
<pre>@@ -2112,7 +2277,11 @@ static void create_pte_array(u64 *ptes, dma_addr_t addr, size_t size)</pre>
<pre> * written to directly by GSP-RM and can be any multiple of GSP_PAGE_SIZE.</pre>
<pre> *</pre>
<pre> * The physical address map for the log buffer is stored in the buffer</pre>
<pre>- * itself, starting with offset 1. Offset 0 contains the "put" pointer.</pre>
<pre>+ * itself, starting with offset 1. Offset 0 contains the "put" pointer (pp).</pre>
<pre>+ * Initially, pp is equal to 0. If the buffer has valid logging data in it,</pre>
<pre>+ * then pp points to index into the buffer where the next logging entry will</pre>
<pre>+ * be written. Therefore, the logging data is valid if:</pre>
<pre>+ * 1 <= pp < sizeof(buffer)/sizeof(u64)</pre>
<pre> *</pre>
<pre> * The GSP only understands 4K pages (GSP_PAGE_SIZE), so even if the kernel is</pre>
<pre> * configured for a larger page size (e.g. 64K pages), we need to give</pre>
<pre>@@ -2183,6 +2352,11 @@ r535_gsp_libos_init(struct nvkm_gsp *gsp)</pre>
<pre> args[3].size = gsp->rmargs.size;</pre>
<pre> args[3].kind = LIBOS_MEMORY_REGION_CONTIGUOUS;</pre>
<pre> args[3].loc = LIBOS_MEMORY_REGION_LOC_SYSMEM;</pre>
<pre>+</pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+ r535_gsp_libos_debugfs_init(gsp);</pre>
<pre>+#endif</pre>
<pre>+</pre>
<pre> return 0;</pre>
<pre> }</pre>
<pre> </pre>
<pre>@@ -2493,6 +2667,182 @@ r535_gsp_dtor_fws(struct nvkm_gsp *gsp)</pre>
<pre> gsp->fws.rm = NULL;</pre>
<pre> }</pre>
<pre> </pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+</pre>
<pre>+struct r535_gsp_log {</pre>
<pre>+ struct nvif_log log;</pre>
<pre>+</pre>
<pre>+ /*</pre>
<pre>+ * Logging buffers in debugfs. The wrapper objects need to remain</pre>
<pre>+ * in memory until the dentry is deleted.</pre>
<pre>+ */</pre>
<pre>+ struct dentry *debugfs_logging_dir;</pre>
<pre>+ struct debugfs_blob_wrapper blob_init;</pre>
<pre>+ struct debugfs_blob_wrapper blob_intr;</pre>
<pre>+ struct debugfs_blob_wrapper blob_rm;</pre>
<pre>+ struct debugfs_blob_wrapper blob_pmu;</pre>
<pre>+};</pre>
<pre>+</pre>
<pre>+/**</pre>
<pre>+ * r535_debugfs_shutdown - delete GSP-RM logging buffers for one GPU</pre>
<pre>+ * @_log: nvif_log struct for this GPU</pre>
<pre>+ *</pre>
<pre>+ * Called when the driver is shutting down, to clean up the retained GSP-RM</pre>
<pre>+ * logging buffers.</pre>
<pre>+ */</pre>
<pre>+static void r535_debugfs_shutdown(struct nvif_log *_log)</pre>
<pre>+{</pre>
<pre>+ struct r535_gsp_log *log = container_of(_log, struct r535_gsp_log, log);</pre>
<pre>+</pre>
<pre>+ debugfs_remove(log->debugfs_logging_dir);</pre>
<pre>+</pre>
<pre>+ kfree(log->blob_init.data);</pre>
<pre>+ kfree(log->blob_intr.data);</pre>
<pre>+ kfree(log->blob_rm.data);</pre>
<pre>+ kfree(log->blob_pmu.data);</pre>
<pre>+</pre>
<pre>+ /* We also need to delete the list object */</pre>
<pre>+ kfree(log);</pre>
<pre>+}</pre>
<pre>+</pre>
<pre>+/**</pre>
<pre>+ * is_empty - return true if the logging buffer was never written to</pre>
<pre>+ * @b: blob wrapper with ->data field pointing to logging buffer</pre>
<pre>+ *</pre>
<pre>+ * The first 64-bit field of loginit, and logintr, and logrm is the 'put'</pre>
<pre>+ * pointer, and it is initialized to 0. If the pointer is still 0 when</pre>
</blockquote>
<pre><br></pre>
<pre>Double space. </pre>
</blockquote>
<div><br>
</div>
<div>Uh, are you only now noticing that I always put two spaces after a "."? I do that everywhere.</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Also, what would 'put' point to in case it's non-zero? Is it a</pre>
<pre>pointer actually, or is it just some kind of counter?</pre>
</blockquote>
<div><br>
</div>
<div>It's called the "put pointer" internally, but it's actually a dword-based index into the buffer. So a value of 1 is an iffset of 4, a value of 2 is an offset of 8, etc.</div>
<div><br>
</div>
<div>I'll update the docs.</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>+ * GSP-RM is shut down, that means that it was never written do, so it</pre>
</blockquote>
<pre><br></pre>
<pre>"written to"</pre>
</blockquote>
<div><br>
</div>
<div>Fixed, thanks.</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> void</pre>
<pre> r535_gsp_dtor(struct nvkm_gsp *gsp)</pre>
<pre> {</pre>
<pre>@@ -2514,6 +2864,14 @@ r535_gsp_dtor(struct nvkm_gsp *gsp)</pre>
<pre> nvkm_gsp_mem_dtor(&gsp->rmargs);</pre>
<pre> nvkm_gsp_mem_dtor(&gsp->wpr_meta);</pre>
<pre> nvkm_gsp_mem_dtor(&gsp->shm.mem);</pre>
<pre>+</pre>
<pre>+#ifdef CONFIG_DEBUG_FS</pre>
<pre>+ r535_gsp_retain_logging(gsp);</pre>
<pre>+</pre>
<pre>+ kfree(gsp->blob_pmu.data);</pre>
<pre>+ gsp->blob_pmu.data = NULL;</pre>
</blockquote>
<pre><br></pre>
<pre>Please move this to r535_gsp_libos_debugfs_fini() to make it a bit more obvious</pre>
<pre>why this needs to be cleaned up here.</pre>
</blockquote>
<div><br>
</div>
<div>So first, I made a mistake, and the #ifdef is only supposed to be around the call to r535_gsp_retain_logging().</div>
<div><br>
</div>
<div>Second, are you saying you want this?</div>
<div><br>
</div>
<div>/** </div>
<div> * <span style="font-size: 14.666667px;">r535_gsp_libos_debugfs_fini - retrain debugfs buffers if necessary</span></div>
<div> *</div>
<div> * bla bla some text about why we're doing this</div>
<div> */</div>
<div>static void r535_gsp_libos_debugfs_fini(struct nvkm_gsp __maybe_unused *gsp)</div>
<div>{</div>
<div>#ifdef CONFIG_DEBUG_FS</div>
<div><span class="Apple-tab-span" style="white-space:pre"></span>r535_gsp_retain_logging(gsp);</div>
<div>#endif</div>
<div>}</div>
<div><br>
</div>
<div><br>
</div>
<div><span></span></div>
</body>
</html>