Re: [PATCH v3 12/12] drm/xe/sriov: Add debugfs to set EQ and PT for scheduler groups

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

please,

	drm/xe/pf:

On 12/11/2025 2:57 AM, Daniele Ceraolo Spurio wrote:
> Debugfs files are added to allow a user to provide a comma-separated list
> of values to assign to each group for each VF.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
> v2: drop files for individual groups, check input length (Michal)
> v3: add an extra space when to separate the dumped values (Michal)
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c | 128 +++++++++++++++++++-
>  1 file changed, 124 insertions(+), 4 deletions(-)
> 
> 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 c09a89c69fad..7bf17649affb 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> @@ -163,10 +163,18 @@ static void pf_add_policy_attrs(struct xe_gt *gt, struct dentry *parent)
>   *          :   ├── tile0
>   *              :   ├── gt0
>   *                  :   ├── sched_groups_mode
> + *                      ├── sched_groups_exec_quantums_ms
> + *                      ├── sched_groups_preempt_timeout_us
>   *                      ├── sched_groups
>   *                      :   ├── group0
>   *                          :
> - *                          └── groupN
> + *          :               └── groupN
> + *          ├── vf1
> + *          :   ├── tile0
> + *              :   ├── gt0
> + *                  :   ├── sched_groups_exec_quantums_ms
> + *                      ├── sched_groups_preempt_timeout_us
> + *                      :
>   */
>  
>  static const char *sched_group_mode_to_string(enum xe_sriov_sched_group_modes mode)
> @@ -258,6 +266,109 @@ static const struct file_operations sched_groups_fops = {
>  	.release = single_release,
>  };
>  
> +static int sched_groups_config_show(struct seq_file *m, void *data,
> +				    void (*get)(struct xe_gt *, unsigned int, u32 *, u32))
> +{
> +	struct drm_printer p = drm_seq_file_printer(m);
> +	unsigned int vfid = extract_vfid(m->private);
> +	struct xe_gt *gt = extract_gt(m->private);
> +	u32 values[GUC_MAX_SCHED_GROUPS];
> +	bool first = true;
> +	u8 group;
> +
> +	get(gt, vfid, values, ARRAY_SIZE(values));
> +
> +	for (group = 0; group < ARRAY_SIZE(values); group++) {
> +		drm_printf(&p, "%s%u", first ? "" : ", ", values[group]);

unfortunately, we can't add space between values as then currently used
function to parse input does not correctly recognized such input as array

> +
> +		first = false;
> +	}
> +
> +	drm_puts(&p, "\n");
> +
> +	return 0;
> +}
> +
> +static ssize_t sched_groups_config_write(struct file *file, const char __user *ubuf,
> +					 size_t size, loff_t *pos,
> +					 int (*set)(struct xe_gt *, unsigned int, u32 *, u32))
> +{
> +	struct dentry *parent = file_inode(file)->i_private;
> +	unsigned int vfid = extract_vfid(parent);
> +	struct xe_gt *gt = extract_gt(parent);
> +	u32 values[GUC_MAX_SCHED_GROUPS];
> +	int *input;
> +	u32 count;
> +	int ret;
> +	int i;
> +
> +	if (*pos)
> +		return -ESPIPE;
> +
> +	if (!size)
> +		return -ENODATA;
> +
> +	ret = parse_int_array_user(ubuf, min(size, GUC_MAX_SCHED_GROUPS * sizeof(u32)), &input);

while this is core function its functionality isn't perfect

* it doesn't complain if we provide malformed list "1, 2"
* it accepts ranges that we don't want "1-2"

while we can use it for now, IMO we should prepare something more robust
that we might use later in preparation to sysfs promotion

> +	if (ret)
> +		return ret;
> +
> +	count = input[0];
> +	if (count > GUC_MAX_SCHED_GROUPS) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		if (input[i + 1] < 0 || input[i + 1] > S32_MAX) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		values[i] = input[i + 1];
> +	}
> +
> +	xe_pm_runtime_get(gt_to_xe(gt));
> +	ret = set(gt, vfid, values, count);
> +	xe_pm_runtime_put(gt_to_xe(gt));
> +
> +out:
> +	kfree(input);
> +	return ret < 0 ? ret : size;
> +}
> +
> +#define DEFINE_SRIOV_GT_GRP_CFG_DEBUGFS_ATTRIBUTE(CONFIG)			\
> +static int sched_groups_##CONFIG##_show(struct seq_file *m, void *data)		\
> +{										\
> +	return sched_groups_config_show(m, data,				\
> +					xe_gt_sriov_pf_config_get_groups_##CONFIG); \
> +}										\
> +										\
> +static int sched_groups_##CONFIG##_open(struct inode *inode, struct file *file)	\
> +{										\
> +	return single_open(file, sched_groups_##CONFIG##_show,			\
> +			   inode->i_private);					\
> +}										\
> +										\
> +static ssize_t sched_groups_##CONFIG##_write(struct file *file,			\
> +					      const char __user *ubuf,		\
> +					      size_t size, loff_t *pos)		\
> +{										\
> +	return sched_groups_config_write(file, ubuf, size, pos,			\
> +					 xe_gt_sriov_pf_config_set_groups_##CONFIG); \
> +}										\
> +										\
> +static const struct file_operations sched_groups_##CONFIG##_fops = {		\
> +	.owner = THIS_MODULE,							\
> +	.open = sched_groups_##CONFIG##_open,					\
> +	.read = seq_read,							\
> +	.llseek = seq_lseek,							\
> +	.write = sched_groups_##CONFIG##_write,					\
> +	.release = single_release,						\
> +}
> +
> +DEFINE_SRIOV_GT_GRP_CFG_DEBUGFS_ATTRIBUTE(exec_quantums);
> +DEFINE_SRIOV_GT_GRP_CFG_DEBUGFS_ATTRIBUTE(preempt_timeouts);
> +
>  static ssize_t sched_group_engines_read(struct file *file, char __user *buf,
>  					size_t count, loff_t *ppos)
>  {
> @@ -303,13 +414,13 @@ static const struct file_operations sched_group_engines_fops = {
>  	.llseek = default_llseek,
>  };
>  
> -static void pf_add_sched_groups(struct xe_gt *gt, struct dentry *parent)
> +static void pf_add_sched_groups(struct xe_gt *gt, struct dentry *parent, unsigned int vfid)

nit: maybe we can use this signature from the beginning ?

>  {
>  	struct dentry *groups;
>  	u8 group;
>  
>  	xe_gt_assert(gt, gt == extract_gt(parent));
> -	xe_gt_assert(gt, PFID == extract_vfid(parent));
> +	xe_gt_assert(gt, vfid == extract_vfid(parent));

same here

>  
>  	/*
>  	 * TODO: we currently call this function before we initialize scheduler
> @@ -326,6 +437,14 @@ static void pf_add_sched_groups(struct xe_gt *gt, struct dentry *parent)
>  	if (!xe_sriov_gt_pf_policy_has_sched_groups_support(gt))
>  		return;
>  
> +	debugfs_create_file("sched_groups_exec_quantums_ms", 0644, parent, parent,
> +			    &sched_groups_exec_quantums_fops);
> +	debugfs_create_file("sched_groups_preempt_timeouts_us", 0644, parent, parent,
> +			    &sched_groups_preempt_timeouts_fops);
> +
> +	if (vfid != PFID)
> +		return;
> +
>  	debugfs_create_file("sched_groups_mode", 0644, parent, parent, &sched_groups_fops);
>  
>  	groups = debugfs_create_dir("sched_groups", parent);
> @@ -703,6 +822,7 @@ static void pf_populate_gt(struct xe_gt *gt, struct dentry *dent, unsigned int v
>  
>  	if (vfid) {
>  		pf_add_config_attrs(gt, dent, vfid);
> +		pf_add_sched_groups(gt, dent, vfid);
>  
>  		debugfs_create_file("control", 0600, dent, NULL, &control_ops);
>  
> @@ -716,7 +836,7 @@ static void pf_populate_gt(struct xe_gt *gt, struct dentry *dent, unsigned int v
>  	} else {
>  		pf_add_config_attrs(gt, dent, PFID);
>  		pf_add_policy_attrs(gt, dent);
> -		pf_add_sched_groups(gt, dent);
> +		pf_add_sched_groups(gt, dent, PFID);
>  
>  		drm_debugfs_create_files(pf_info, ARRAY_SIZE(pf_info), dent, minor);
>  	}

rest looks fine, so with nits and show() function fixed,

	Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>