Re: [PATCH 2/2] drm/xe/debugfs: Improve .show() helper for GT-based attributes

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 19, 2025 at 05:27:05PM -0400, Rodrigo Vivi wrote:
> On Fri, Sep 19, 2025 at 06:04:30PM +0200, Michal Wajdeczko wrote:
> > Like we did for tile-based attributes, introduce separate show()
> > helper that implicitly takes an RPM reference prior to the call
> > to the actual print() function. This translates into some savings.
> > 
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > Here is a report from the scripts/bloat-o-meter:
> > 
> >   add/remove: 2/2 grow/shrink: 0/14 up/down: 114/-581 (-467)
> >   Function                                     old     new   delta
> >   xe_gt_debugfs_show_with_rpm                    -      98     +98
> >   __pfx_xe_gt_debugfs_show_with_rpm              -      16     +16
> >   __pfx_powergate_info                          16       -     -16
> >   register_save_restore                        317     288     -29
> >   vecs_default_lrc                              73      42     -31
> >   vcs_default_lrc                               73      42     -31
> >   rcs_default_lrc                               70      39     -31
> >   ccs_default_lrc                               73      42     -31
> >   bcs_default_lrc                               73      42     -31
> >   hw_engines                                   179     145     -34
> >   hwconfig                                      70      33     -37
> >   workarounds                                   66      26     -40
> >   tunings                                       66      26     -40
> >   topology                                      66      26     -40
> >   steering                                      66      26     -40
> >   pat                                           66      26     -40
> >   mocs                                          66      26     -40
> >   powergate_info                                70       -     -70
> >   Total: Before=2870434, After=2869967, chg -0.02%
> > ---
> >  drivers/gpu/drm/xe/xe_gt_debugfs.c | 116 +++++++++++------------------
> >  drivers/gpu/drm/xe/xe_gt_debugfs.h |   1 +
> >  2 files changed, 46 insertions(+), 71 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > index e4eba91cb83d..b9176d4398e1 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > @@ -35,6 +35,11 @@
> >  #include "xe_uc_debugfs.h"
> >  #include "xe_wa.h"
> >  
> > +static struct xe_gt *node_to_gt(struct drm_info_node *node)
> > +{
> > +	return node->dent->d_parent->d_inode->i_private;
> > +}
> > +
> >  /**
> >   * xe_gt_debugfs_simple_show - A show callback for struct drm_info_list
> >   * @m: the &seq_file
> > @@ -77,8 +82,7 @@ int xe_gt_debugfs_simple_show(struct seq_file *m, void *data)
> >  {
> >  	struct drm_printer p = drm_seq_file_printer(m);
> >  	struct drm_info_node *node = m->private;
> > -	struct dentry *parent = node->dent->d_parent;
> > -	struct xe_gt *gt = parent->d_inode->i_private;
> > +	struct xe_gt *gt = node_to_gt(node);
> >  	int (*print)(struct xe_gt *, struct drm_printer *) = node->info_ent->data;
> >  
> >  	if (WARN_ON(!print))
> > @@ -87,15 +91,36 @@ int xe_gt_debugfs_simple_show(struct seq_file *m, void *data)
> >  	return print(gt, &p);
> >  }
> >  
> > +/**
> > + * xe_gt_debugfs_show_with_rpm - A show callback for struct drm_info_list
> > + * @m: the &seq_file
> > + * @data: data used by the drm debugfs helpers
> > + *
> > + * Similar to xe_gt_debugfs_simple_show() but implicitly takes a RPM ref.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int xe_gt_debugfs_show_with_rpm(struct seq_file *m, void *data)
> > +{
> > +	struct drm_info_node *node = m->private;
> > +	struct xe_gt *gt = node_to_gt(node);
> > +	struct xe_device *xe = gt_to_xe(gt);
> > +	int ret;
> > +
> > +	xe_pm_runtime_get(xe);
> > +	ret = xe_gt_debugfs_simple_show(m, data);
> > +	xe_pm_runtime_put(xe);
> > +
> > +	return ret;
> > +}
> > +
> >  static int hw_engines(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	struct xe_device *xe = gt_to_xe(gt);
> >  	struct xe_hw_engine *hwe;
> >  	enum xe_hw_engine_id id;
> >  	unsigned int fw_ref;
> >  	int ret = 0;
> >  
> > -	xe_pm_runtime_get(xe);
> >  	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >  	if (!xe_force_wake_ref_has_domain(fw_ref, XE_FORCEWAKE_ALL)) {
> >  		ret = -ETIMEDOUT;
> > @@ -107,37 +132,19 @@ static int hw_engines(struct xe_gt *gt, struct drm_printer *p)
> >  
> >  fw_put:
> >  	xe_force_wake_put(gt_to_fw(gt), fw_ref);
> > -	xe_pm_runtime_put(xe);
> > -
> > -	return ret;
> > -}
> > -
> > -static int powergate_info(struct xe_gt *gt, struct drm_printer *p)
> 
> I believe this should be removed in a separate patch.
> It confused me when the number of rpm removals and the number of
> function changed below didn't match. 
> 
> > -{
> > -	int ret;
> > -
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> > -	ret = xe_gt_idle_pg_print(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> >  
> >  	return ret;
> >  }
> >  
> >  static int topology(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_gt_topology_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int steering(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_gt_mcr_steering_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> > @@ -146,8 +153,6 @@ static int register_save_restore(struct xe_gt *gt, struct drm_printer *p)
> >  	struct xe_hw_engine *hwe;
> >  	enum xe_hw_engine_id id;
> >  
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> > -
> >  	xe_reg_sr_dump(&gt->reg_sr, p);
> >  	drm_printf(p, "\n");
> >  
> > @@ -165,98 +170,66 @@ static int register_save_restore(struct xe_gt *gt, struct drm_printer *p)
> >  	for_each_hw_engine(hwe, gt, id)
> >  		xe_reg_whitelist_dump(&hwe->reg_whitelist, p);
> >  
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int workarounds(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_wa_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int tunings(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_tuning_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int pat(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_pat_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int mocs(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_mocs_dump(gt, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int rcs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_RENDER);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int ccs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_COMPUTE);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int bcs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_COPY);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int vcs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_VIDEO_DECODE);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int vecs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_VIDEO_ENHANCE);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> >  static int hwconfig(struct xe_gt *gt, struct drm_printer *p)
> >  {
> > -	xe_pm_runtime_get(gt_to_xe(gt));
> >  	xe_guc_hwconfig_dump(&gt->uc.guc, p);
> > -	xe_pm_runtime_put(gt_to_xe(gt));
> > -
> >  	return 0;
> >  }
> >  
> > @@ -266,25 +239,26 @@ static int hwconfig(struct xe_gt *gt, struct drm_printer *p)
> >   * - without access to the PF specific data
> >   */
> >  static const struct drm_info_list vf_safe_debugfs_list[] = {
> > -	{"topology", .show = xe_gt_debugfs_simple_show, .data = topology},
> > -	{"register-save-restore", .show = xe_gt_debugfs_simple_show, .data = register_save_restore},
> > -	{"workarounds", .show = xe_gt_debugfs_simple_show, .data = workarounds},
> > -	{"tunings", .show = xe_gt_debugfs_simple_show, .data = tunings},
> > -	{"default_lrc_rcs", .show = xe_gt_debugfs_simple_show, .data = rcs_default_lrc},
> > -	{"default_lrc_ccs", .show = xe_gt_debugfs_simple_show, .data = ccs_default_lrc},
> > -	{"default_lrc_bcs", .show = xe_gt_debugfs_simple_show, .data = bcs_default_lrc},
> > -	{"default_lrc_vcs", .show = xe_gt_debugfs_simple_show, .data = vcs_default_lrc},
> > -	{"default_lrc_vecs", .show = xe_gt_debugfs_simple_show, .data = vecs_default_lrc},
> > -	{"hwconfig", .show = xe_gt_debugfs_simple_show, .data = hwconfig},
> > +	{ "topology", .show = xe_gt_debugfs_show_with_rpm, .data = topology },
> > +	{ "register-save-restore",
> > +		.show = xe_gt_debugfs_show_with_rpm, .data = register_save_restore },
> > +	{ "workarounds", .show = xe_gt_debugfs_show_with_rpm, .data = workarounds },
> > +	{ "tunings", .show = xe_gt_debugfs_show_with_rpm, .data = tunings },
> > +	{ "default_lrc_rcs", .show = xe_gt_debugfs_show_with_rpm, .data = rcs_default_lrc },
> > +	{ "default_lrc_ccs", .show = xe_gt_debugfs_show_with_rpm, .data = ccs_default_lrc },
> > +	{ "default_lrc_bcs", .show = xe_gt_debugfs_show_with_rpm, .data = bcs_default_lrc },
> > +	{ "default_lrc_vcs", .show = xe_gt_debugfs_show_with_rpm, .data = vcs_default_lrc },
> > +	{ "default_lrc_vecs", .show = xe_gt_debugfs_show_with_rpm, .data = vecs_default_lrc },
> > +	{ "hwconfig", .show = xe_gt_debugfs_show_with_rpm, .data = hwconfig },
> >  };
> >  
> >  /* everything else should be added here */
> >  static const struct drm_info_list pf_only_debugfs_list[] = {
> > -	{"hw_engines", .show = xe_gt_debugfs_simple_show, .data = hw_engines},
> > -	{"mocs", .show = xe_gt_debugfs_simple_show, .data = mocs},
> > -	{"pat", .show = xe_gt_debugfs_simple_show, .data = pat},
> > -	{"powergate_info", .show = xe_gt_debugfs_simple_show, .data = powergate_info},
> > -	{"steering", .show = xe_gt_debugfs_simple_show, .data = steering},
> > +	{ "hw_engines", .show = xe_gt_debugfs_show_with_rpm, .data = hw_engines },
> > +	{ "mocs", .show = xe_gt_debugfs_show_with_rpm, .data = mocs },
> > +	{ "pat", .show = xe_gt_debugfs_show_with_rpm, .data = pat },
> > +	{ "powergate_info", .show = xe_gt_debugfs_show_with_rpm, .data = xe_gt_idle_pg_print },

please ignore my last email, I had missed this entry here. And thanks for pointing out it offline.

I wonder if we could do something similar to the void return functions as well so we
keep some kind of symmetry...

anyway this patch is correct:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > +	{ "steering", .show = xe_gt_debugfs_show_with_rpm, .data = steering },
> >  };
> >  
> >  static ssize_t write_to_gt_call(const char __user *userbuf, size_t count, loff_t *ppos,
> > diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.h b/drivers/gpu/drm/xe/xe_gt_debugfs.h
> > index 05a6cc93c78c..32ee3264051b 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_debugfs.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.h
> > @@ -11,5 +11,6 @@ struct xe_gt;
> >  
> >  void xe_gt_debugfs_register(struct xe_gt *gt);
> >  int xe_gt_debugfs_simple_show(struct seq_file *m, void *data);
> > +int xe_gt_debugfs_show_with_rpm(struct seq_file *m, void *data);
> >  
> >  #endif
> > -- 
> > 2.47.1
> >