Re: [PATCH v7 3/6] drm/xe/guc: Split engine state print between xe_hw_engine vs xe_guc_capture

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

On 2025-02-10 6:32 p.m., Alan Previn wrote:
> Relocate the xe_engine_snapshot_print function from xe_guc_capture.c
> into xe_hw_engine.c but split out the GuC-Err-Capture register printing
> portion out into a separate helper inside xe_guc_capture.c so that
> we can have a clear separation between printing the general engine info
> vs GuC-Err-Capture node's register list.
> 
>     v7: - Fix function name to respect "xe_hw_engine" name space. (Rodrigo)
>         - Remove additional newline in engine dump (Jose Souza) +
>           ensure changes didn't break mesa's aubinator tool (Rodrigo)
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_devcoredump.c |  2 +-
>   drivers/gpu/drm/xe/xe_guc_capture.c | 79 +++++++++++++----------------
>   drivers/gpu/drm/xe/xe_guc_capture.h |  4 +-
>   drivers/gpu/drm/xe/xe_hw_engine.c   | 29 ++++++++++-
>   drivers/gpu/drm/xe/xe_hw_engine.h   |  1 +
>   5 files changed, 67 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 006041997550..7a4610d2ea4f 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -128,7 +128,7 @@ static ssize_t __xe_devcoredump_read(char *buffer, size_t count,
>   	drm_puts(&p, "\n**** HW Engines ****\n");
>   	for (i = 0; i < XE_NUM_HW_ENGINES; i++)
>   		if (ss->hwe[i])
> -			xe_engine_snapshot_print(ss->hwe[i], &p);
> +			xe_hw_engine_snapshot_print(ss->hwe[i], &p);
>   
>   	drm_puts(&p, "\n**** VM state ****\n");
>   	xe_vm_snapshot_print(ss->vm, &p);
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index f118e8dd0ecb..76c20ff97864 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -917,9 +917,10 @@ guc_capture_init_node(struct xe_guc *guc, struct xe_guc_capture_snapshot *node)
>    * --------------------
>    *      --> xe_devcoredump_read->
>    *             L--> xxx_snapshot_print
> - *                    L--> xe_engine_snapshot_print
> - *                         Print register lists values saved at
> - *                         guc->capture->outlist
> + *                    L--> xe_hw_engine_print --> xe_hw_engine_snapshot_print
> + *                          L--> xe_guc_capture_snapshot_print
> + *                               Print register lists values saved in matching
> + *                               node from guc->capture->outlist
>    *
>    */
>   
> @@ -1655,22 +1656,16 @@ guc_capture_find_reg(struct gcap_reg_list_info *reginfo, u32 addr, u32 flags)
>   }
>   
>   static void
> -snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p,
> -			     u32 type, const struct __guc_mmio_reg_descr_group *list)
> +print_noderegs_by_list_order(struct xe_guc *guc, struct gcap_reg_list_info *reginfo,
> +			     const struct __guc_mmio_reg_descr_group *list, struct drm_printer *p)
>   {
> -	struct xe_gt *gt = snapshot->hwe->gt;
> -	struct xe_guc *guc = &gt->uc.guc;
> -	struct gcap_reg_list_info *reginfo = NULL;
> -	u32 i, last_value = 0;
> +	u32 last_value, i;
>   	bool is_ext, low32_ready = false;
>   
>   	if (!list || !list->list || list->num_regs == 0)
>   		return;
>   
> -	XE_WARN_ON(!snapshot->matched_node);
> -
>   	is_ext = list == guc->capture->extlists;
> -	reginfo = &snapshot->matched_node->reginfo[type];
>   
>   	/*
>   	 * loop through descriptor first and find the register in the node
> @@ -1740,8 +1735,8 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
>   
>   				group = FIELD_GET(GUC_REGSET_STEERING_GROUP, reg_desc->flags);
>   				instance = FIELD_GET(GUC_REGSET_STEERING_INSTANCE, reg_desc->flags);
> -				dss = xe_gt_mcr_steering_info_to_dss_id(gt, group, instance);
> -
> +				dss = xe_gt_mcr_steering_info_to_dss_id(guc_to_gt(guc), group,
> +									instance);
>   				drm_printf(p, "\t%s[%u]: 0x%08x\n", reg_desc->regname, dss, value);
>   			} else {
>   				drm_printf(p, "\t%s: 0x%08x\n", reg_desc->regname, value);
> @@ -1760,13 +1755,18 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
>   }
>   
>   /**
> - * xe_engine_snapshot_print - Print out a given Xe HW Engine snapshot.
> - * @snapshot: Xe HW Engine snapshot object.
> + * xe_guc_capture_snapshot_print - Print out a the contents of a provided Guc-Err-Capture node
> + * @guc : Target GuC for operation.
> + * @node: GuC Error Capture register dump node.
>    * @p: drm_printer where it will be printed out.
>    *
> - * This function prints out a given Xe HW Engine snapshot object.
> + * This function prints out a register dump of a GuC-Err-Capture node that was retrieved
> + * earlier either by GuC-FW reporting or by manual capture depending on how the
> + * caller (typically xe_hw_engine_snapshot) was invoked and used.
>    */
> -void xe_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p)
> +
> +void xe_guc_capture_snapshot_print(struct xe_guc *guc, struct xe_guc_capture_snapshot *node,
> +				   struct drm_printer *p)
>   {
>   	const char *grptype[GUC_STATE_CAPTURE_GROUP_TYPE_MAX] = {
>   		"full-capture",
> @@ -1774,45 +1774,36 @@ void xe_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm
>   	};
>   	int type;
>   	const struct __guc_mmio_reg_descr_group *list;
> -	enum guc_capture_list_class_type capture_class;
>   	struct xe_gt *gt;
>   
> -	if (!snapshot)
> +	if (!guc)
>   		return;
> -
> -	gt = snapshot->hwe->gt;
> -
> -	if (!snapshot->matched_node)
> +	gt = guc_to_gt(guc);
> +	if (!node) {
node was called snapshot befrore.
> +		xe_gt_warn(gt, "GuC Capture printing without node!\n");
>   		return;
> +	}
> +	if (!p) {
New printer pointer check, good.
> +		xe_gt_warn(gt, "GuC Capture printing without printer!\n");
> +		return;
> +	}
>   
> -	xe_gt_assert(gt, snapshot->hwe);
> -
> -	capture_class = xe_engine_class_to_guc_capture_class(snapshot->hwe->class);
> -
> -	drm_printf(p, "%s (physical), logical instance=%d\n",
> -		   snapshot->name ? snapshot->name : "",
> -		   snapshot->logical_instance);
>   	drm_printf(p, "\tCapture_source: %s\n",
> -		   snapshot->matched_node->source == XE_ENGINE_CAPTURE_SOURCE_GUC ?
> +		   node->source == XE_ENGINE_CAPTURE_SOURCE_GUC ?
>   		   "GuC" : "Manual");
> -	drm_printf(p, "\tCoverage: %s\n", grptype[snapshot->matched_node->is_partial]);
> -	drm_printf(p, "\tForcewake: domain 0x%x, ref %d\n",
> -		   snapshot->forcewake.domain, snapshot->forcewake.ref);
> -	drm_printf(p, "\tReserved: %s\n",
> -		   str_yes_no(snapshot->kernel_reserved));
> +	drm_printf(p, "\tCoverage: %s\n", grptype[node->is_partial]);
Yes, I see the printout order was changed:
vcs0 (physical), logical instance=0
	Capture_source: GuC
	Coverage: full-capture
	Forcewake: domain 0x8, ref 1
	Reserved: no
	FORCEWAKE_GT: 0x00000000
to:
vcs0 (physical), logical instance=0
	Forcewake: domain 0x8, ref 1
	Reserved: no
	Capture_source: GuC
	Coverage: full-capture
	FORCEWAKE_GT: 0x00000000
The xe_exec_capture igt test can handle this change, as long as it not 
cause other tools stop working, I'm fine.

Reviewed-by: Zhanjun Dong <zhanjun.dong@intel.com>

>   
>   	for (type = GUC_STATE_CAPTURE_TYPE_GLOBAL; type < GUC_STATE_CAPTURE_TYPE_MAX; type++) {
>   		list = xe_guc_capture_get_reg_desc_list(gt, GUC_CAPTURE_LIST_INDEX_PF, type,
> -							capture_class, false);
> -		snapshot_print_by_list_order(snapshot, p, type, list);
> +							node->eng_class, false);
> +		print_noderegs_by_list_order(guc, &node->reginfo[type], list, p);
>   	}
>   
> -	if (capture_class == GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE) {
> +	if (node->eng_class == GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE) {
> +		type = GUC_STATE_CAPTURE_TYPE_ENGINE_CLASS;
>   		list = xe_guc_capture_get_reg_desc_list(gt, GUC_CAPTURE_LIST_INDEX_PF,
> -							GUC_STATE_CAPTURE_TYPE_ENGINE_CLASS,
> -							capture_class, true);
> -		snapshot_print_by_list_order(snapshot, p, GUC_STATE_CAPTURE_TYPE_ENGINE_CLASS,
> -					     list);
> +							type, node->eng_class, true);
> +		print_noderegs_by_list_order(guc, &node->reginfo[type], list, p);
>   	}
>   
>   	drm_puts(p, "\n");
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.h b/drivers/gpu/drm/xe/xe_guc_capture.h
> index 8ac893c92f19..e67589ab4342 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.h
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.h
> @@ -15,7 +15,6 @@
>   struct xe_exec_queue;
>   struct xe_guc;
>   struct xe_hw_engine;
> -struct xe_hw_engine_snapshot;
>   
>   static inline enum guc_capture_list_class_type xe_guc_class_to_capture_class(u16 class)
>   {
> @@ -55,7 +54,8 @@ struct xe_guc_capture_snapshot *
>   xe_guc_capture_get_matching_and_lock(struct xe_exec_queue *q,
>   				     enum xe_guc_capture_snapshot_source srctype);
>   void xe_guc_capture_snapshot_store_manual_job(struct xe_guc *guc, struct xe_exec_queue *q);
> -void xe_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p);
> +void xe_guc_capture_snapshot_print(struct xe_guc *guc, struct xe_guc_capture_snapshot *node,
> +				   struct drm_printer *p);
>   void xe_engine_snapshot_capture_for_queue(struct xe_exec_queue *q);
>   void xe_guc_capture_steered_list_init(struct xe_guc *guc);
>   void xe_guc_capture_put_matched_nodes(struct xe_guc *guc, struct xe_guc_capture_snapshot *n);
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 02871d319471..c980a5c84a8b 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -907,6 +907,33 @@ void xe_hw_engine_snapshot_free(struct xe_hw_engine_snapshot *snapshot)
>   	kfree(snapshot);
>   }
>   
> +/**
> + * xe_hw_engine_snapshot_print - Print out a given Xe HW Engine snapshot.
> + * @snapshot: Xe HW Engine snapshot object.
> + * @p: drm_printer where it will be printed out.
> + *
> + * This function prints out a given Xe HW Engine snapshot object.
> + */
> +void xe_hw_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p)
> +{
> +	struct xe_gt *gt;
> +
> +	if (!snapshot)
> +		return;
> +
> +	gt = snapshot->hwe->gt;
> +
> +	drm_printf(p, "%s (physical), logical instance=%d\n",
> +		   snapshot->name ? snapshot->name : "",
> +		   snapshot->logical_instance);
> +	drm_printf(p, "\tForcewake: domain 0x%x, ref %d\n",
> +		   snapshot->forcewake.domain, snapshot->forcewake.ref);
> +	drm_printf(p, "\tReserved: %s\n",
> +		   str_yes_no(snapshot->kernel_reserved));
> +
> +	xe_guc_capture_snapshot_print(&gt->uc.guc, snapshot->matched_node, p);
> +}
> +
>   /**
>    * xe_hw_engine_print - Xe HW Engine Print.
>    * @hwe: Hardware Engine.
> @@ -919,7 +946,7 @@ void xe_hw_engine_print(struct xe_hw_engine *hwe, struct drm_printer *p)
>   	struct xe_hw_engine_snapshot *snapshot;
>   
>   	snapshot = xe_hw_engine_snapshot_capture(hwe, NULL);
> -	xe_engine_snapshot_print(snapshot, p);
> +	xe_hw_engine_snapshot_print(snapshot, p);
>   	xe_hw_engine_snapshot_free(snapshot);
>   }
>   
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> index 6b5f9fa2a594..069b32aa7423 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> @@ -58,6 +58,7 @@ u32 xe_hw_engine_mask_per_class(struct xe_gt *gt,
>   struct xe_hw_engine_snapshot *
>   xe_hw_engine_snapshot_capture(struct xe_hw_engine *hwe, struct xe_exec_queue *q);
>   void xe_hw_engine_snapshot_free(struct xe_hw_engine_snapshot *snapshot);
> +void xe_hw_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p);
>   void xe_hw_engine_print(struct xe_hw_engine *hwe, struct drm_printer *p);
>   void xe_hw_engine_setup_default_lrc_state(struct xe_hw_engine *hwe);
>