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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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(&gt->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
> >