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

[Dong, Zhanjun]

Please see my comments inline below.

Regards,
Zhanjun Dong

On 2025-06-28 9:06 a.m., Michal Wajdeczko wrote:
> 
> 
> On 23.04.2025 23:58, 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     | 52 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_log.h     |  1 +
>>   3 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c b/drivers/gpu/drm/xe/xe_guc_debugfs.c
>> index f33013f8a0f3..a441c5775ca3 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);
>> @@ -116,6 +122,7 @@ static const struct drm_info_list vf_safe_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 },
>>   	{ "guc_pc", .show = guc_debugfs_show, .data = guc_pc },
>>   };
>> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
>> index 38039c411387..e362e407f6d5 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_log_lfd_abi.h"
>>   #include "regs/xe_guc_regs.h"
>>   #include "xe_bo.h"
>>   #include "xe_devcoredump.h"
>> @@ -19,6 +20,14 @@
>>   #include "xe_mmio.h"
>>   #include "xe_module.h"
>>   
>> +static const struct guc_logfile_header default_guc_logfile_header = {
>> +	.magic = LFD_DRIVER_KEY_STREAMING,
>> +	.version.dw0 = FIELD_PREP_CONST(GUC_LOGFILE_FMT_VER_MINOR_VERSION,
>> +					GUC_LOG_FILE_FORMAT_VERSION_MINOR) |
>> +		       FIELD_PREP_CONST(GUC_LOGFILE_FMT_VER_MAJOR_VERSION,
>> +					GUC_LOG_FILE_FORMAT_VERSION_MAJOR)
>> +};
>> +
>>   static struct xe_guc *
>>   log_to_guc(struct xe_guc_log *log)
>>   {
>> @@ -216,6 +225,35 @@ 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,
>> +			      struct xe_guc_log *log)
> 
> 'log' - unused ?
to be removed>
>> +{
>> +	char *lfd_buf;
>> +	int index;
> 
> size_t or drop it completely
> 
>> +
>> +	if (!snapshot || !snapshot->size)
>> +		return;
>> +
>> +	/* Alloc lfd buffer in fixed size */
> 
> s/lfd/LFD to be consistent with your comment below
> 
>> +	lfd_buf = kzalloc(GUC_LOG_CHUNK_SIZE, GFP_KERNEL);
>> +	if (!lfd_buf) {
>> +		index = -ENOMEM;
>> +		goto lfd_out;
> 
> just exit here?
Although for ENOMEM there is no need for free memory, we still need to 
print \n>
>> +	}
>> +
>> +	/* Fill LFD file header */
>> +	memcpy(lfd_buf, &default_guc_logfile_header, sizeof(default_guc_logfile_header));
> 
> hmm, why exactly we need to copy this ?
> can't we just print directly from default_guc_logfile_header ?
Yes we can>
> and what's the goal of printing just empty lfd buffer?
> add some explanation in the commit message
This patch is to add the debugfs entry, detailed LFD segements to be 
filled in next patch. Will add more details.>
>> +	index = offsetof(struct guc_logfile_header, lfd_stream);
>> +	xe_print_blob_ascii85(p, "[LOG].data", 0, lfd_buf, 0, (size_t)index);
>> +
>> +lfd_out:
>> +	drm_printf(p, "\n");
>> +	if (index < 0)
>> +		drm_printf(p, "ERROR: Out of memory\n");
> 
> shouldn't this be in some common format, like:
> 
> 	"[LOG].error = %pe", ERR_PTR(-ENOMEM)
Good point, will do>
>> +	kfree(lfd_buf);
>> +}
>> +
>>   /**
>>    * xe_guc_log_print_dmesg - dump a copy of the GuC log to dmesg
>>    * @log: GuC log structure
>> @@ -251,6 +289,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
>> + * @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, log);
>> +	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 5b896f5fafaf..37ff4d11e6cf 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);
>