Re: [PATCH i-g-t v1] tools/xe_guc_logger: Add guc logger for Xe

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

Thanks for review.
Please see my inline comments below.

Regards,
Zhanjun Dong

On 2025-04-08 5:10 a.m., Peter Senna Tschudin wrote:
> Dear Zhanjun,
> 
> On 3/12/2025 10:30 PM, Zhanjun Dong wrote:
>> Add guc logger for Xe, support save guc log in LFD format.
> 
> I tested this patch and found the behavior of the tool a bit
> confusing. It behaves more like a test than a standalone tool.
> For example:
> 
> $ sudo ./build/tools/xe_guc_logger -i /sys/kernel/debug/dri/0000:03:00.0/gt0/uc/guc_log
> IGT-Version: 2.0-g4b22256d0 (x86_64) (Linux: 6.14.0-xe x86_64)
> Using IGT_SRANDOM=1744102201 for randomisation
> SUCCESS (0.371s)
> 
> $ sudo ./build/tools/xe_guc_logger --help
> Usage: xe_guc_logger [OPTIONS]
>    --list-subtests
>    --show-testlist
>    --run-subtest <pattern>
>    --dynamic-subtest <pattern>
>    --debug[=log-domain]
>    --interactive-debug[=domain]
>    --skip-crc-compare
>    --trace-on-oops
>    --hook [<events>:]<cmd>
>    --help-hook
>    --help-description
>    --describe
>    --device filters
>    --version
>    --help|-h
>    -i --inputfile=name   name of the guc log file, including the path
>    -o --outputfile=name  name of the output file, including the location, where logs will be stored
>    -v --verbosity=level   verbosity level of output
> 
> This makes it appear as if the logger is a test case rather
> than a tool. I suggest removing this test-like behavior. You
> can look at other examples under the tools/ directory, such
> as lsgpu and igt_facts, which are implemented as tools.
Right, the tool should not has the testcase help message, to be updated.

> 
> Additionally, the tool is hard to use without reading the
> source code. It would help to:
>   - Automatically locate the guc_log files in /sys/kernel/debug/
>   - Provide a clearer error message when the input file is
>     not specified
> 
> Right now, omitting the -i option results in a test-like
> assertion failure with no guidance:
> 
> $ sudo ./build/tools/xe_guc_logger
> IGT-Version: 2.0-g4b22256d0 (x86_64) (Linux: 6.14.0-xe x86_64)
> Using IGT_SRANDOM=1744102681 for randomisation
> (xe_guc_logger:2445) CRITICAL: Test assertion failure function load_guc_log, file ../tools/xe_guc_logger.c:267:
> (xe_guc_logger:2445) CRITICAL: Failed assertion: fd
> (xe_guc_logger:2445) CRITICAL: Last errno: 2, No such file or directory
> (xe_guc_logger:2445) CRITICAL: couldn't open the file: guc_log
> Stack trace:
>    #0 ../lib/igt_core.c:2065 __igt_fail_assert()
>    #1 ../tools/xe_guc_logger.c:166 main()
>    #2 ../sysdeps/nptl/libc_start_call_main.h:74 __libc_start_call_main()
>    #3 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
>    #4 [_start+0x25]
> Test xe_guc_logger failed.
> **** DEBUG ****
> (xe_guc_logger:2445) igt_core-INFO: IGT-Version: 2.0-g4b22256d0 (x86_64) (Linux: 6.14.0-xe x86_64)
> (xe_guc_logger:2445) igt_core-INFO: Using IGT_SRANDOM=1744102681 for randomisation
> (xe_guc_logger:2445) CRITICAL: Test assertion failure function load_guc_log, file ../tools/xe_guc_logger.c:267:
> (xe_guc_logger:2445) CRITICAL: Failed assertion: fd
> (xe_guc_logger:2445) CRITICAL: Last errno: 2, No such file or directory
> (xe_guc_logger:2445) CRITICAL: couldn't open the file: guc_log
> (xe_guc_logger:2445) igt_core-INFO: Stack trace:
> (xe_guc_logger:2445) igt_core-INFO:   #0 ../lib/igt_core.c:2065 __igt_fail_assert()
> (xe_guc_logger:2445) igt_core-INFO:   #1 ../tools/xe_guc_logger.c:166 main()
> (xe_guc_logger:2445) igt_core-INFO:   #2 ../sysdeps/nptl/libc_start_call_main.h:74 __libc_start_call_main()
> (xe_guc_logger:2445) igt_core-INFO:   #3 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
> (xe_guc_logger:2445) igt_core-INFO:   #4 [_start+0x25]
> ****  END  ****
> FAIL (0.046s)
> 
> Instead, please consider printing a helpful usage message,
> such as:
> 
> $ sudo ./build/tools/xe_guc_logger
> ERROR: Please use -i to specify the guc_log file
> (e.g. -i /sys/kernel/debug/dri/0000:03:00.0/gt0/uc/guc_log)
'-i' is designed to let user specifiy data source, it would be usedful 
for mulptile gpu  system. Thanks for take time to test it, I will 
consider better handling on this. >
> Lastly, it would be helpful to clarify the purpose of the
> output file. What does the tool provide that a simple cat
> on the guc_log file wouldn’t? A short explanation in the
> source code and the --help output would help users and
> developers understand this tool.
Good point, will add that later.

Next rev will have dependency with KMD/guc changes, this tool will not 
moving forward before those changes finished.>
> Thank you,
> 
> Peter>
>> Reference:
>> https://coredocs.intel.com/InterfaceDocs/sphinx/core/kmd_log_file_format.html?highlight=lfd
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>> ---
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>>
>>   tools/lfd.h           | 590 ++++++++++++++++++++++++++++++++++++++++
>>   tools/lfd_default.h   |  39 +++
>>   tools/meson.build     |   1 +
>>   tools/xe_guc_logger.c | 615 ++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 1245 insertions(+)
>>   create mode 100644 tools/lfd.h
>>   create mode 100644 tools/lfd_default.h
>>   create mode 100644 tools/xe_guc_logger.c

... Omitted ...