Re: [PATCH 3/3] drm/xe/pf: Expose SR-IOV policy settings over debugfs

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

On Mon, Apr 22, 2024 at 10:05:45PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 22.04.2024 21:39, Rodrigo Vivi wrote:
> > On Thu, Apr 18, 2024 at 10:34:42PM +0200, Michal Wajdeczko wrote:
> >> We already have functions to configure SR-IOV policies.
> >> Allow to tweak those policy settings over debugfs.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c | 50 +++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> >> index 8909bb950a8b..3a83af7aa039 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> >> @@ -17,6 +17,7 @@
> >>  #include "xe_gt_sriov_pf_control.h"
> >>  #include "xe_gt_sriov_pf_debugfs.h"
> >>  #include "xe_gt_sriov_pf_helpers.h"
> >> +#include "xe_gt_sriov_pf_policy.h"
> >>  #include "xe_pm.h"
> >>  
> >>  /*
> >> @@ -76,6 +77,54 @@ static const struct drm_info_list pf_info[] = {
> >>  	},
> >>  };
> >>  
> >> +/*
> >> + *      /sys/kernel/debug/dri/0/
> >> + *      ├── gt0
> >> + *      │   ├── pf
> >> + *      │   │   ├── reset_engine
> >> + *      │   │   ├── sample_period
> >> + *      │   │   ├── sched_if_idle
> >> + */
> >> +
> >> +#define DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(POLICY, TYPE, FORMAT)		\
> >> +										\
> >> +static int POLICY##_set(void *data, u64 val)					\
> >> +{										\
> >> +	struct xe_gt *gt = extract_gt(data);					\
> >> +	struct xe_device *xe = gt_to_xe(gt);					\
> >> +	int err;								\
> >> +										\
> >> +	xe_pm_runtime_get(xe);							\
> >> +	err = xe_gt_sriov_pf_policy_set_##POLICY(gt, val);			\
> >> +	xe_pm_runtime_put(xe);							\
> > 
> > I like that this takes care of the runtime_pm in a 'global' way without duplication...
> > 
> >> +										\
> >> +	return err;								\
> >> +}										\
> >> +										\
> >> +static int POLICY##_get(void *data, u64 *val)					\
> >> +{										\
> >> +	struct xe_gt *gt = extract_gt(data);					\
> >> +										\
> >> +	*val = xe_gt_sriov_pf_policy_get_##POLICY(gt);				\
> >> +	return 0;								\
> >> +}										\
> >> +										\
> >> +DEFINE_DEBUGFS_ATTRIBUTE(POLICY##_fops, POLICY##_get, POLICY##_set, FORMAT)
> >> +
> >> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(reset_engine, bool, "%llu\n");
> >> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(sched_if_idle, bool, "%llu\n");
> >> +DEFINE_SRIOV_GT_POLICY_DEBUGFS_ATTRIBUTE(sample_period, u32, "%llu\n");
> > 
> > ... but, is there any other way we could accomplish this without this style
> > that hides function... I hate when I have to understand anything in i915-guc
> > code because you cannot navigate or grep for the functions that are declared
> > with magic macros like this.
> 
> but such template macros are quite widely used (at least for sysfs)
> 
> $ cgrep ssize_t.*\\#.*show.*\\\\ | wc -l
> 189
> 
> and IMO it's better to have one template than replicate very similar
> code N times with just one line difference
> 
> besides this are static functions, no need to grep anywhere else

good point indeed. it is not something we will be manually calling from
somewhere else, but never seeing the declaration anywhere like in that
i915 guc code.

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

> 
> > 
> > but this is my only complain and no big suggestions. I would like to hear
> > from the xe maintainers here.
> > 
> > for the debugfs entries and rpm handling and everything I like that.
> > 
> >> +
> >> +static void pf_add_policy_attrs(struct xe_gt *gt, struct dentry *parent)
> >> +{
> >> +	xe_gt_assert(gt, gt == extract_gt(parent));
> >> +	xe_gt_assert(gt, PFID == extract_vfid(parent));
> >> +
> >> +	debugfs_create_file_unsafe("reset_engine", 0644, parent, parent, &reset_engine_fops);
> >> +	debugfs_create_file_unsafe("sched_if_idle", 0644, parent, parent, &sched_if_idle_fops);
> >> +	debugfs_create_file_unsafe("sample_period_ms", 0644, parent, parent, &sample_period_fops);
> >> +}
> >> +
> >>  /*
> >>   *      /sys/kernel/debug/dri/0/
> >>   *      ├── gt0
> >> @@ -261,6 +310,7 @@ void xe_gt_sriov_pf_debugfs_register(struct xe_gt *gt, struct dentry *root)
> >>  	pfdentry->d_inode->i_private = gt;
> >>  
> >>  	drm_debugfs_create_files(pf_info, ARRAY_SIZE(pf_info), pfdentry, minor);
> >> +	pf_add_policy_attrs(gt, pfdentry);
> >>  	pf_add_config_attrs(gt, pfdentry, PFID);
> >>  
> >>  	for (n = 1; n <= totalvfs; n++) {
> >> -- 
> >> 2.43.0
> >>