[PATCH v6 3/6] drm/xe/guc: Add new debugfs entry for lfd format output

[Dong, Zhanjun]

See my comments inline below.

Regards,
Zhanjun Dong

On 2025-07-29 3:19 p.m., John Harrison wrote:
> On 7/21/2025 6:35 PM, Zhanjun Dong wrote:
>> Add new debugfs entry "guc_log_lfd", prepared for output guc log
>> in LFD(Log Format Descriptors) format.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_debugfs.c |  7 +++++++
>>   drivers/gpu/drm/xe/xe_guc_log.c     | 32 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_log.h     |  1 +
>>   3 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c b/drivers/gpu/drm/xe/ 
>> xe_guc_debugfs.c
>> index 0b102ab46c4d..6cbc4240a3bd 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_debugfs.c
>> @@ -85,6 +85,12 @@ static int guc_log(struct xe_guc *guc, struct 
>> drm_printer *p)
>>       return 0;
>>   }
>> +static int guc_log_lfd(struct xe_guc *guc, struct drm_printer *p)
>> +{
>> +    xe_guc_log_print_lfd(&guc->log, p);
>> +    return 0;
>> +}
>> +
>>   static int guc_log_dmesg(struct xe_guc *guc, struct drm_printer *p)
>>   {
>>       xe_guc_log_print_dmesg(&guc->log);
>> @@ -121,6 +127,7 @@ static const struct drm_info_list 
>> slpc_debugfs_list[] = {
>>   /* everything else should be added here */
>>   static const struct drm_info_list pf_only_debugfs_list[] = {
>>       { "guc_log", .show = guc_debugfs_show, .data = guc_log },
>> +    { "guc_log_lfd", .show = guc_debugfs_show, .data = guc_log_lfd },
>>       { "guc_log_dmesg", .show = guc_debugfs_show, .data = 
>> guc_log_dmesg },
>>   };
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/ 
>> xe_guc_log.c
>> index c01ccb35dc75..d2256773aaf3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
>> @@ -9,6 +9,7 @@
>>   #include <drm/drm_managed.h>
>> +#include "abi/guc_lfd_abi.h"
>>   #include "regs/xe_guc_regs.h"
>>   #include "xe_bo.h"
>>   #include "xe_devcoredump.h"
>> @@ -19,6 +20,12 @@
>>   #include "xe_mmio.h"
>>   #include "xe_module.h"
>> +static const struct guc_lfd_file_header default_guc_lfd_file_header = {
>> +    .magic = GUC_LFD_DRIVER_KEY_STREAMING,
>> +    .version.dw0 = FIELD_PREP_CONST(GUC_LFD_VERSION_MINOR, 
>> GUC_LFD_FORMAT_VERSION_MINOR) |
>> +               FIELD_PREP_CONST(GUC_LFD_VERSION_MAJOR, 
>> GUC_LFD_FORMAT_VERSION_MAJOR)
>> +};
> For the sake of two words, I'm not seeing that it is worth having this 
> as a static global structure. You are going to need to either copy it to 
> a local structure that you then fill in the 'lfd_stream' field of or 
> only output the first part of the structure. Either option is messy.
To be removed

> 
>> +
>>   static struct xe_guc *
>>   log_to_guc(struct xe_guc_log *log)
>>   {
>> @@ -216,6 +223,17 @@ void xe_guc_log_snapshot_print(struct 
>> xe_guc_log_snapshot *snapshot, struct drm_
>>       }
>>   }
>> +static void
>> +xe_guc_log_snapshot_print_lfd(struct xe_guc_log_snapshot *snapshot, 
>> struct drm_printer *p)
>> +{
>> +    if (!snapshot || !snapshot->size)
>> +        return;
>> +
>> +    /* Output LFD file header */
>> +    xe_print_blob_ascii85(p, "[LOG].data", 0, 
>> &default_guc_lfd_file_header, 0,
>> +                  offsetof(struct guc_lfd_file_header, lfd_stream));
> This is not an LFD format output. And a quick glance I am not seeing it 
> being replaced in any of the later patches.
> 
> Also, all you are generating at this point is a header with no actual 
> LFD blocks. That would not be a valid LFD stream. You should move this 
> patch to much later in the series (if not the end) so you only start 
> generating a file when it is a valid file.

From
https://docs.kernel.org/process/submitting-patches.html
"
When dividing your change into a series of patches, take special care to 
ensure that the kernel builds and runs properly after each patch in the 
series.
"
LFD feature code is called by debugfs, if debugfs is the number 2 patch 
in series, build #1 will get "xxx not referenced" build error.
Might discuss offline.

> 
>> +}
>> +
>>   /**
>>    * xe_guc_log_print_dmesg - dump a copy of the GuC log to dmesg
>>    * @log: GuC log structure
>> @@ -251,6 +269,20 @@ void xe_guc_log_print(struct xe_guc_log *log, 
>> struct drm_printer *p)
>>       xe_guc_log_snapshot_free(snapshot);
>>   }
>> +/**
>> + * xe_guc_log_print_lfd - dump a copy of the GuC log to some useful 
>> location
> Is this a cut/paste comment from somewhere else?
To be changed to:
        xe_guc_log_print_lfd - dump a copy of the GuC log in LFD format>
> John.
> 
>> + * @log: GuC log structure
>> + * @p: the printer object to output to
>> + */
>> +void xe_guc_log_print_lfd(struct xe_guc_log *log, struct drm_printer *p)
>> +{
>> +    struct xe_guc_log_snapshot *snapshot;
>> +
>> +    snapshot = xe_guc_log_snapshot_capture(log, false);
>> +    xe_guc_log_snapshot_print_lfd(snapshot, p);
>> +    xe_guc_log_snapshot_free(snapshot);
>> +}
>> +
>>   int xe_guc_log_init(struct xe_guc_log *log)
>>   {
>>       struct xe_device *xe = log_to_xe(log);
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.h b/drivers/gpu/drm/xe/ 
>> xe_guc_log.h
>> index f1e2b0be90a9..0f6299886010 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_log.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_log.h
>> @@ -40,6 +40,7 @@ struct xe_device;
>>   int xe_guc_log_init(struct xe_guc_log *log);
>>   void xe_guc_log_print(struct xe_guc_log *log, struct drm_printer *p);
>> +void xe_guc_log_print_lfd(struct xe_guc_log *log, struct drm_printer 
>> *p);
>>   void xe_guc_log_print_dmesg(struct xe_guc_log *log);
>>   struct xe_guc_log_snapshot *xe_guc_log_snapshot_capture(struct 
>> xe_guc_log *log, bool atomic);
>>   void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, 
>> struct drm_printer *p);
>