Re: [PATCH v7 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 Tue, 2025-02-11 at 17:48 -0500, Dong, Zhanjun wrote:
> 
> 
> 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.
alan: yes i did - previously snapshot was one-level up of xe_hw_engine that included other things
alongside the matching node from guc-err-capture but with this patch i wanted to streamline this
this helper since it only needs access to the node and nothing else. 
> > +               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>

alan: Thanks for the RB and yes - we tested it with mesa tool also.

alan:snip