On Mon, 2024-06-17 at 21:54 +0200, Danilo Krummrich wrote:
Hi Timur,thanks for the follow-up on this patch series.On Wed, Jun 12, 2024 at 06:52:53PM -0500, Timur Tabi wrote:The LOGINIT, LOGINTR, LOGRM, and LOGPMU buffers are circular buffersthat have printf-like logs from GSP-RM and PMU encoded in them.LOGINIT, LOGINTR, and LOGRM are allocated by Nouveau and their DMAaddresses are passed to GSP-RM during initialization. The buffers arerequired for GSP-RM to initialize properly.LOGPMU is also allocated by Nouveau, but its contents are updatedwhen Nouveau receives an NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT RPC fromGSP-RM. Nouveau then copies the RPC to the buffer.The messages are encoded as an array of variable-length structures thatcontain the parameters to an NV_PRINTF call. The format string andparameter count are stored in a special ELF image that contains onlylogging strings. This image is not currently shipped with the Nvidiadriver.There are two methods to extract the logs.OpenRM tries to load the logging ELF, and if present, parses the logbuffers in real time and outputs the strings to the kernel console.Alternatively, and this is the method used by this patch, the bufferscan be exposed to user space, and a user-space tool (along with thelogging ELF image) can parse the buffer and dump the logs.This method has the advantage that it allows the buffers to be parsedeven when the logging ELF file is not available to the user. However,it has the disadvantage the debubfs entries need to remain until thedriver is unloaded.The buffers are exposed via debugfs. If GSP-RM fails to initialize,then Nouveau immediately shuts down the GSP interface. This wouldnormally also deallocate the logging buffers, thereby preventing theuser from capturing the debug logs.To avoid this, introduce the keep-gsp-logging command line parameter.If specified, and if at least one logging buffer has content, thenNouveau will migrate these buffers into new debugfs entries that areretained until the driver unloads.An end-user can capture the logs using the following commands:cp /sys/kernel/debug/dri/<path>/loginit loginitcp /sys/kernel/debug/dri/<path>/logrm logrmcp /sys/kernel/debug/dri/<path>/logintr logintrcp /sys/kernel/debug/dri/<path>/logpmu logpmuwhere <path> is the PCI ID of the GPU (e.g. 0000:65:00.0).Since LOGPMU is not needed for normal GSP-RM operation, it is onlycreated if debugfs is available. Otherwise, theNV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT RPCs are ignored.Signed-off-by: Timur Tabi <ttabi@xxxxxxxxxx>---v5:rebased to drm-misc-nextrepaced nvkm_gsp_log with nvif_logminor rearrangement of some codedrivers/gpu/drm/nouveau/include/nvif/log.h | 32 ++.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 13 +drivers/gpu/drm/nouveau/nouveau_drm.c | 19 +.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 360 +++++++++++++++++-4 files changed, 423 insertions(+), 1 deletion(-)create mode 100644 drivers/gpu/drm/nouveau/include/nvif/log.hdiff --git a/drivers/gpu/drm/nouveau/include/nvif/log.h b/drivers/gpu/drm/nouveau/include/nvif/log.hnew file mode 100644index 000000000000..c89ba85a701d--- /dev/null+++ b/drivers/gpu/drm/nouveau/include/nvif/log.h@@ -0,0 +1,32 @@+/* SPDX-License-Identifier: MIT */+/* SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. */++#ifndef __NVIF_LOG_H__+#define __NVIF_LOG_H__++/**+ * nvif_log - structure for tracking logging buffers+ * @head: list headWhat about "@entry: an entry in a list of struct nvif_logs".
Done.
+ * @shutdown: pointer to function to call to clean upI'd be a bit more specific and say that this frees all backing resources, suchas logging buffer, etc.
Done.
+ *+ * Structure used to track logging buffers so that they can be cleaned up+ * when the driver exits.+ */+struct nvif_log {+ struct list_head head;I suggest to call this 'entry', since that's what it actually represents, andentry in a list.
Oh yeah, I see what you mean now. Done.
+ void (*shutdown)(struct nvif_log *log);+};++#ifdef CONFIG_DEBUG_FS+/**+ * gsp_logs - list of nvif_log GSP-RM logging buffers+ *+ * Head pointer to a a list of nvif_log buffers that is created for each GPU+ * upon GSP shutdown if the "keep_gsp_logging" command-line parameter is+ * specified. This is used to track the alternative debugfs entries for the+ * GSP-RM logs.+ */+extern struct list_head gsp_logs;Better wrap this in a struct nvif_logs (or similar) and pass this down throughnvkm_device_pci_new() / nvkm_device_tegra_new() instead of relying on sharinga global.
I'm still having difficulty understanding what you mean by this. I think you need to be more explicit in what you want.
struct nvif_logs
{
struct list_head logs;
} gsp_logs;
First, I don't understand what this gets me. It just wraps one struct inside another.
Should I add this to struct nvkm_device_func? And then do something like this in pci.c:
static struct nvif_logs gsp_logs;
static const struct nvkm_device_func
nvkm_device_pci_func = {
...
.nvif_logs = &gsp_logs,
};
So nvkm_device_pci_new() calls nvkm_device_ctor(). 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.
+#ifdef CONFIG_DEBUG_FS+/**+ * gsp_logs - list of GSP debugfs logging buffers+ */+LIST_HEAD(gsp_logs);Better wrap this in a NVIF_LOGS_DECLARE() macro.
Like this?
#define NVIF_LOGS_DECLARE(x) LIST_HEAD(x)
Do you want a macro to replace this as well?
extern struct list_head gsp_logs;
+#endif+static u64nouveau_pci_name(struct pci_dev *pdev){@@ -1446,6 +1454,17 @@ nouveau_drm_exit(void)#endifif (IS_ENABLED(CONFIG_DRM_NOUVEAU_SVM))mmu_notifier_synchronize();++#ifdef CONFIG_DEBUG_FS+ if (!list_empty(&gsp_logs)) {+ struct nvif_log *log, *n;++ list_for_each_entry_safe(log, n, &gsp_logs, head) {+ /* shutdown() should also delete the log entry */+ log->shutdown(log);+ }+ }+#endifPlease move this to include/nvif/log.h as nvif_log_shutdown().
Sure, but I don't understand why. This code will only be called in nouveau_drm_exit().
+/**+ * r535_gsp_msg_libos_print - capture log message from the PMU+ * @priv: gsp pointer+ * @fn: function number (ignored)+ * @repv: pointer to libos print RPC+ * @repc: message size+ *+ * See _kgspRpcUcodeLibosPrintWhat is this reference?
It's in OpenRM. Pretty much all of the "See" commands are references to the corresponding function in OpenRM".
+ */+static int r535_gsp_msg_libos_print(void *priv, u32 fn, void *repv, u32 repc)+{+ struct nvkm_gsp *gsp = priv;+ struct nvkm_subdev *subdev = &gsp->subdev;+ struct {+ u32 ucodeEngDesc;+ u32 libosPrintBufSize;+ u8 libosPrintBuf[];Why are those camelCase? Please use snake_case instead.
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.
I'll move this to a distinct struct and rename the fields.
Also, please add a few notes on the fields GSP returns to us here. Admittedly,two of those seem rather obvious - a log buffer and a size - but maybe we canwrite down whether there is some maximum or minimum size? Can we get called witha zero-size buffer?What about 'ucodeEngDesc'? This one seems to behave more like a register value,can we document the "register layout" for this one?
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.
+ i_size_write(d_inode(dir_init), gsp->blob_init.size);+ i_size_write(d_inode(dir_intr), gsp->blob_intr.size);+ i_size_write(d_inode(dir_rm), gsp->blob_rm.size);+ i_size_write(d_inode(dir_pmu), gsp->blob_pmu.size);++ r535_gsp_msg_ntfy_add(gsp, 0x0000100C, r535_gsp_msg_libos_print, gsp);Please, don't add random magic values. Add a useful explanation instead thatalso new contributors are able to make sense of.
Ah, I can just use the macro that's already defined in rpc_global_enums.h.
++ nvkm_debug(&gsp->subdev, "created debugfs GSP-RM logging entries\n");++ if (keep_gsp_logging) {+ nvkm_warn(&gsp->subdev,+ "logging buffers will be retained on failure\n");+ }++ return;++error:+ debugfs_remove(dir_init);+ debugfs_remove(dir_intr);+ debugfs_remove(dir_rm);+}++#endif+static inline u64r535_gsp_libos_id8(const char *name){@@ -2112,7 +2277,11 @@ static void create_pte_array(u64 *ptes, dma_addr_t addr, size_t size)* written to directly by GSP-RM and can be any multiple of GSP_PAGE_SIZE.** The physical address map for the log buffer is stored in the buffer- * itself, starting with offset 1. Offset 0 contains the "put" pointer.+ * itself, starting with offset 1. Offset 0 contains the "put" pointer (pp).+ * Initially, pp is equal to 0. If the buffer has valid logging data in it,+ * then pp points to index into the buffer where the next logging entry will+ * be written. Therefore, the logging data is valid if:+ * 1 <= pp < sizeof(buffer)/sizeof(u64)** The GSP only understands 4K pages (GSP_PAGE_SIZE), so even if the kernel is* configured for a larger page size (e.g. 64K pages), we need to give@@ -2183,6 +2352,11 @@ r535_gsp_libos_init(struct nvkm_gsp *gsp)args[3].size = gsp->rmargs.size;args[3].kind = LIBOS_MEMORY_REGION_CONTIGUOUS;args[3].loc = LIBOS_MEMORY_REGION_LOC_SYSMEM;++#ifdef CONFIG_DEBUG_FS+ r535_gsp_libos_debugfs_init(gsp);+#endif+return 0;}@@ -2493,6 +2667,182 @@ r535_gsp_dtor_fws(struct nvkm_gsp *gsp)gsp->fws.rm = NULL;}+#ifdef CONFIG_DEBUG_FS++struct r535_gsp_log {+ struct nvif_log log;++ /*+ * Logging buffers in debugfs. The wrapper objects need to remain+ * in memory until the dentry is deleted.+ */+ struct dentry *debugfs_logging_dir;+ struct debugfs_blob_wrapper blob_init;+ struct debugfs_blob_wrapper blob_intr;+ struct debugfs_blob_wrapper blob_rm;+ struct debugfs_blob_wrapper blob_pmu;+};++/**+ * r535_debugfs_shutdown - delete GSP-RM logging buffers for one GPU+ * @_log: nvif_log struct for this GPU+ *+ * Called when the driver is shutting down, to clean up the retained GSP-RM+ * logging buffers.+ */+static void r535_debugfs_shutdown(struct nvif_log *_log)+{+ struct r535_gsp_log *log = container_of(_log, struct r535_gsp_log, log);++ debugfs_remove(log->debugfs_logging_dir);++ kfree(log->blob_init.data);+ kfree(log->blob_intr.data);+ kfree(log->blob_rm.data);+ kfree(log->blob_pmu.data);++ /* We also need to delete the list object */+ kfree(log);+}++/**+ * is_empty - return true if the logging buffer was never written to+ * @b: blob wrapper with ->data field pointing to logging buffer+ *+ * The first 64-bit field of loginit, and logintr, and logrm is the 'put'+ * pointer, and it is initialized to 0. If the pointer is still 0 whenDouble space.
Uh, are you only now noticing that I always put two spaces after a "."? I do that everywhere.
Also, what would 'put' point to in case it's non-zero? Is it apointer actually, or is it just some kind of counter?
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.
I'll update the docs.
+ * GSP-RM is shut down, that means that it was never written do, so it"written to"
Fixed, thanks.
voidr535_gsp_dtor(struct nvkm_gsp *gsp){@@ -2514,6 +2864,14 @@ r535_gsp_dtor(struct nvkm_gsp *gsp)nvkm_gsp_mem_dtor(&gsp->rmargs);nvkm_gsp_mem_dtor(&gsp->wpr_meta);nvkm_gsp_mem_dtor(&gsp->shm.mem);++#ifdef CONFIG_DEBUG_FS+ r535_gsp_retain_logging(gsp);++ kfree(gsp->blob_pmu.data);+ gsp->blob_pmu.data = "">Please move this to r535_gsp_libos_debugfs_fini() to make it a bit more obviouswhy this needs to be cleaned up here.
So first, I made a mistake, and the #ifdef is only supposed to be around the call to r535_gsp_retain_logging().
Second, are you saying you want this?
/**
* r535_gsp_libos_debugfs_fini - retrain debugfs buffers if necessary
*
* bla bla some text about why we're doing this
*/
static void r535_gsp_libos_debugfs_fini(struct nvkm_gsp __maybe_unused *gsp)
{
#ifdef CONFIG_DEBUG_FS
r535_gsp_retain_logging(gsp);
#endif
}
