[PATCH v3 2/4] drm/xe/guc: Add new debugfs entry for lfd format output

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Zhanjun Dong
Sent: Thursday, April 10, 2025 8:59 AM
To: intel-xe at lists.freedesktop.org
Cc: Dong, Zhanjun <zhanjun.dong at intel.com>
Subject: [PATCH v3 2/4] drm/xe/guc: Add new debugfs entry for lfd format output
> 
> 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>

Some non-blockers below, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

> ---
>  drivers/gpu/drm/xe/xe_guc_debugfs.c | 14 ++++++++++++
>  drivers/gpu/drm/xe/xe_guc_log.c     | 34 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc_log.h     |  1 +
>  3 files changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> index c569ff456e74..6449c9a69b8a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> @@ -48,6 +48,19 @@ static int guc_log(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static int guc_log_lfd(struct seq_file *m, void *data)
> +{
> +	struct xe_guc *guc = node_to_guc(m->private);
> +	struct xe_device *xe = guc_to_xe(guc);
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	xe_pm_runtime_get(xe);
> +	xe_guc_log_print_lfd(&guc->log, &p);
> +	xe_pm_runtime_put(xe);
> +
> +	return 0;
> +}
> +
>  static int guc_log_dmesg(struct seq_file *m, void *data)
>  {
>  	struct xe_guc *guc = node_to_guc(m->private);
> @@ -89,6 +102,7 @@ static int guc_pc(struct seq_file *m, void *data)
>  static const struct drm_info_list debugfs_list[] = {
>  	{"guc_info", guc_info, 0},
>  	{"guc_log", guc_log, 0},
> +	{"guc_log_lfd", guc_log_lfd, 0},
>  	{"guc_log_dmesg", guc_log_dmesg, 0},
>  	{"guc_ctb", guc_ctb, 0},
>  	{"guc_pc", guc_pc, 0},
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index 38039c411387..df849a0ee7e5 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> @@ -216,6 +216,26 @@ 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)
> +{
> +	size_t remain;
> +	int i;
> +
> +	if (!snapshot)
> +		return;
> +
> +	remain = snapshot->size;
> +	for (i = 0; i < snapshot->num_chunks; i++) {
> +		size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
> +
> +		/* To be add: Output snapshot in LFD format */

Non-blocking:
Debatably, one of the following two things should be done:

1) Squash this patch and the next patch together.

2) Swap the order of this patch and the next patch, such that patch
2 adds all the functionality needed for this function to operate, and
patch 3 implements static int guc_log_lfd and void xe_guc_log_print_lfd.

> +
> +		remain -= size;
> +	}
> +}
> +
>  /**
>   * xe_guc_log_print_dmesg - dump a copy of the GuC log to dmesg
>   * @log: GuC log structure
> @@ -251,6 +271,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

Non-blocking:
It's a bit weird that specifying the log is being printed "to some useful location"
is the standard for this file, when this doesn't appear to be the case outside of
xe_guc_log.c.  I wonder if it's because we have xe_guc_log_print_dmesg that
has to specify it's printing to dmesg?

-Jonathan Cavitt

> + * @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);
> -- 
> 2.34.1
> 
>