On Thu, 2025-01-30 at 17:42 -0500, Vivi, Rodrigo wrote:
> On Tue, Jan 28, 2025 at 10:36:49AM -0800, 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.
> >
> >
alan:snip
> > @@ -1774,45 +1773,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) {
> > + xe_gt_warn(gt, "GuC Capture printing without node!\n");
> > return;
> > + }
> > + if (!p) {
> > + 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");
>
> This looks like it is changing the order of the prints. So, please ensure that this
> is not breaking the decode user space tools.
alan: good catch Rodrigo, let me double check this against the mesa tool. Although I am wondering
why the MESA tool would have ever expected non-engine-specific registers (forcewake) to be slotted
after a couple of GuCFW specific tags (like "Capture source" and "Coverage") followed by
engine-register dumps. I suspect the location of the GuC-tags could move around.
Will double check with them.
>
> > - 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]);
> >
> > 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 26006d72904f..d615ebab6e42 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -905,6 +905,34 @@ void xe_hw_engine_snapshot_free(struct xe_hw_engine_snapshot *snapshot)
> > kfree(snapshot);
> > }
> >
> > +/**
> > + * xe_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_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));
> > + drm_puts(p, "\n");
> > +
> > + xe_guc_capture_snapshot_print(>->uc.guc, snapshot->matched_node, p);
> > +}
> > +
> > /**
> > * xe_hw_engine_print - Xe HW Engine Print.
> > * @hwe: Hardware Engine.
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 6b5f9fa2a594..fac2e9a421d9 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_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot, struct drm_printer *p);
>
> please respect the component namespace here
>
> > 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);
> >
> > --
> > 2.34.1
> >
