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

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

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 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
>