Re: [PATCH 4/4] drm/xe/guc: Expose raw access to GuC log over debugfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/14/2024 14:15, Michal Wajdeczko wrote:
On 14.05.2024 22:31, Matthew Brost wrote:
On Tue, May 14, 2024 at 11:13:14AM -0700, John Harrison wrote:
On 5/14/2024 07:58, Michal Wajdeczko wrote:
On 13.05.2024 18:53, John Harrison wrote:
On 5/12/2024 08:36, Michal Wajdeczko wrote:
We already provide the content of the GuC log in debugsfs, but it
is in a text format where each log dword is printed as hexadecimal
number, which does not scale well with large GuC log buffers.

To allow more efficient access to the GuC log, which could benefit
our CI systems, expose raw binary log data.  In addition to less
overhead in preparing text based GuC log file, the new GuC log file
in binary format is also almost 3x smaller.

Any existing script that expects the GuC log buffer in text format
can use command like below to convert from new binary format:

      hexdump -e '4/4 "0x%08x " "\n"'

but this shouldn't be the case as most decoders expect GuC log data
in binary format.
I strongly disagree with this.

Efficiency and file size is not an issue when accessing the GuC log via
debugfs on actual hardware.
to some extend it is as CI team used to refuse to collect GuC logs after
each executed test just because of it's size
I've never heard that argument. I've heard many different arguments but not
one about file size. The default GuC log size is pretty tiny. So size really
is not an issue.

It is an issue when dumping via dmesg but
you definitely should not be dumping binary data to dmesg. Whereas,
not following here - this is debugfs specific, not a dmesg printer
Except that it is preferable to have common code for both if at all
possible.

dumping in binary data is much more dangerous and liable to corruption
because some tool along the way tries to convert to ASCII, or truncates
at the first zero, etc. We request GuC logs be sent by end users,
customer bug reports, etc. all doing things that we have no control over.
hmm, how "cp gt0/uc/guc_log_raw FILE" could end with a corrupted file ?
Because someone then tries to email it, or attach it or copy it via Windows
or any number of other ways in which a file can get munged.

Converting the hexdump back to binary is trivial for those tools which
require it. If you follow the acquisition and decoding instructions on
the wiki page then it is all done for you automatically.
I'm afraid I don't know where this wiki page is, but I do know that hex
conversion dance is not needed for me to get decoded GuC log the way I
used to do
Look for the 'GuC Debug Logs' page on the developer wiki. It's pretty easy
to find.

These patches are trying to solve a problem which does not exist and are
going to make working with GuC logs harder and more error prone.
it at least solves the problem of currently super inefficient way of
generating the GuC log in text format.

it also opens other opportunities to develop tools that could monitor or
capture GuC log independently on  top of what driver is able to offer
today (on i915 there was guc-log-relay, but it was broken for long time,
not sure what are the plans for Xe)

also still not sure how it can be more error prone.
As already explained, the plan is move to LFD - an extensible, streamable,
logging format. Any non-trivial effort that is not helping to move to LFD is
not worth the effort.

On the other hand, there are many other issues with GuC logs that it
would be useful to solves - including extra meta data, reliable output
via dmesg, continuous streaming, pre-sizing the debugfs file to not have
to generate it ~12 times for a single read, etc.
this series actually solves last issue but in a bit different way (we
even don't need to generate full GuC log dump at all if we would like to
capture only part of the log if we know where to look)
No, it doesn't solve it. Your comment below suggests it will be read in 4KB
chunks. Which means your 16MB buffer now requires 4096 separate reads! And
you only doing partial reads of the section you think you need is never
going to be reliable on live system. Not sure why you would want to anyway.
It is just making things much more complex. You now need an intelligent user
land program to read the log out and decode at least the header section to
know what data section to read. You can't just dump the whole thing with
'cat' or 'dd'.

Briefly have read this thread but seconding John's opinion that anything
which breaks GuC log collection via a simple command like cat is a no
hexdump or cp is also a simple command and likely we can assume that
either user will know what command to use or will just type the command
that we say.

go. Also anything that can allow windows to mangle the output would be
less than idle too. Lastly, GuC log collection is not a critical path
operation so it likely does not need to optimized for speed.
but please remember that this patch does not change anything to the
existing debugfs files, the guc_log stays as is, this new raw access is
defined as another guc_log_raw file that would allow develop other use
cases, beyond what is possible with naive text snapshots, like live
monitor for errors, all implemented above kernel driver
Which is another issue. We now have two interfaces to the same thing. When should someone use one or the other? Which interface should we be telling the CI people to use? It is unnecessary duplication and a source of confusion and therefore a way to cause even more problems.

John.



Matt

for reliable output via dmesg - see my proposal at [1]

[1] https://patchwork.freedesktop.org/series/133613/
Hmm. Actually, is this interface allowing the filesystem layers to issue
multiple read calls to read the buffer out in small chunks? That is also
going to break things. If the GuC is still writing to the log as the
user is reading from it, there is the opportunity for each chunk to not
follow on from the previous chunk because the data has just been
overwritten. This is already a problem at the moment that causes issues
when decoding the logs, even with an almost atomic copy of the log into
a temporary buffer before reading it out. Doing the read in separate
chunks is only going to make that problem even worse.
current solution, that converts data into hex numbers, reads log buffer
in chunks of 128 dwords, how proposed here solution that reads in 4K
chunks could be "even worse" ?
See above, 4KB chunks means 4096 separate reads for a 16M buffer. And each
one of those reads is a full round trip to user land and back. If you want
to get at all close to an atomic read of the log then it needs to be done as
a single call that copies the log into a locally allocated kernel buffer and
then allows user land to read out from that buffer rather than from the live
log. Which can be trivially done with the current method (at the expense of
a large memory allocation) but would be much more difficult with random
access reader like this as you would need to say the copied buffer around
until the reads have all been done. Which would presumably mean adding
open/close handlers to allocate and free that memory.

and in case of some smart tool, that would understands the layout of the
GuC log buffer, we can even fully eliminate problem of reading stale
data, so why not to choose a more scalable solution ?
You cannot eliminate the problem of stale data. You read the header, you
read the data it was pointing to, you re-read the header and find that the
GuC has moved on. That is an infinite loop of continuously updating
pointers.

John.

John.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
---
Cc: linux-fsdevel@xxxxxxxxxxxxxxx
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
---
    drivers/gpu/drm/xe/xe_guc_debugfs.c | 26 ++++++++++++++++++++++++++
    1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c
b/drivers/gpu/drm/xe/xe_guc_debugfs.c
index d3822cbea273..53fea952344d 100644
--- a/drivers/gpu/drm/xe/xe_guc_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_guc_debugfs.c
@@ -8,6 +8,7 @@
    #include <drm/drm_debugfs.h>
    #include <drm/drm_managed.h>
    +#include "xe_bo.h"
    #include "xe_device.h"
    #include "xe_gt.h"
    #include "xe_guc.h"
@@ -52,6 +53,29 @@ static const struct drm_info_list debugfs_list[] = {
        {"guc_log", guc_log, 0},
    };
    +static ssize_t guc_log_read(struct file *file, char __user *buf,
size_t count, loff_t *pos)
+{
+    struct dentry *dent = file_dentry(file);
+    struct dentry *uc_dent = dent->d_parent;
+    struct dentry *gt_dent = uc_dent->d_parent;
+    struct xe_gt *gt = gt_dent->d_inode->i_private;
+    struct xe_guc_log *log = &gt->uc.guc.log;
+    struct xe_device *xe = gt_to_xe(gt);
+    ssize_t ret;
+
+    xe_pm_runtime_get(xe);
+    ret = xe_map_read_from(xe, buf, count, pos, &log->bo->vmap,
log->bo->size);
+    xe_pm_runtime_put(xe);
+
+    return ret;
+}
+
+static const struct file_operations guc_log_ops = {
+    .owner        = THIS_MODULE,
+    .read        = guc_log_read,
+    .llseek        = default_llseek,
+};
+
    void xe_guc_debugfs_register(struct xe_guc *guc, struct dentry *parent)
    {
        struct drm_minor *minor = guc_to_xe(guc)->drm.primary;
@@ -72,4 +96,6 @@ void xe_guc_debugfs_register(struct xe_guc *guc,
struct dentry *parent)
        drm_debugfs_create_files(local,
                     ARRAY_SIZE(debugfs_list),
                     parent, minor);
+
+    debugfs_create_file("guc_log_raw", 0600, parent, NULL,
&guc_log_ops);
    }




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux