Re: [PATCH 05/10] drm/xe/sriov: Add debugfs to enable scheduler groups

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

On 11/27/2025 2:45 AM, Daniele Ceraolo Spurio wrote:
> Reading the debugfs file lists the available configurations by name.
> Writing the name of a configuration to the file will enable it.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c | 116 ++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c  |  10 +-
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h  |   2 +
>  3 files changed, 123 insertions(+), 5 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 0fd863609848..2953ef21a5ad 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
> @@ -155,6 +155,121 @@ static void pf_add_policy_attrs(struct xe_gt *gt, struct dentry *parent)
>  	debugfs_create_file_unsafe("sample_period_ms", 0644, parent, parent, &sample_period_fops);
>  }
>  
> +/*
> + *      /sys/kernel/debug/dri/BDF/
> + *      ├── sriov
> + *      :   ├── pf
> + *          :   ├── tile0
> + *              :   ├── gt0
> + *                  :   ├── sched_groups_mode
> + */
> +
> +static const char *sched_group_mode_to_string(enum xe_sriov_sched_group_modes mode)
> +{
> +	switch (mode) {
> +	case XE_SRIOV_SCHED_GROUPS_NONE:
> +		return "disabled";
> +	case XE_SRIOV_SCHED_GROUPS_MEDIA_SLICES:
> +		return "media_slices";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +static int sched_groups_info(struct seq_file *m, void *data)
> +{
> +	struct drm_printer p = drm_seq_file_printer(m);
> +	struct xe_gt *gt = extract_gt(m->private);
> +	u32 current_mode = gt->sriov.pf.policy.guc.sched_groups.current_mode;
> +	int mode = 0;
> +
> +	if (!xe_sriov_gt_pf_policy_has_valid_sched_group_modes(gt)) {
> +		drm_printf(&p, "no groups available\n");

since this will be used by the file read operation and user expects

	"the available configurations by name."

then IMO we should just return empty string

and if we check for EGS support earlier, see below,
then maybe this could be just an assert?

> +		return 0;
> +	}
> +
> +	for (mode = 0; mode < XE_SRIOV_SCHED_GROUPS_MODES_COUNT; mode++) {
> +		if (!xe_sriov_gt_pf_policy_has_sched_group_mode(gt, mode))
> +			continue;
> +
> +		if (mode)
> +			drm_printf(&p, " ");
> +
> +		if (mode == current_mode)
> +			drm_printf(&p, "[");
> +
> +		drm_printf(&p, "%s", sched_group_mode_to_string(mode));
> +
> +		if (mode == current_mode)
> +			drm_printf(&p, "]");
> +	}
> +
> +	drm_printf(&p, "\n");
> +
> +	return 0;
> +}
> +
> +static int sched_groups_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, sched_groups_info, inode->i_private);
> +}
> +
> +static ssize_t sched_groups_write(struct file *file, const char __user *ubuf,
> +				  size_t size, loff_t *pos)
> +{
> +	struct xe_gt *gt = extract_gt(file_inode(file)->i_private);
> +	char name[32];
> +	int ret;
> +	int m;
> +
> +	if (*pos)
> +		return -ESPIPE;
> +
> +	if (!size)
> +		return -ENODATA;
> +
> +	if (!xe_sriov_gt_pf_policy_has_valid_sched_group_modes(gt))
> +		return -ENODEV;

maybe not needed - see below

> +
> +	if (size > sizeof(name) - 1)
> +		return -EINVAL;
> +
> +	ret = simple_write_to_buffer(name, sizeof(name) - 1, pos, ubuf, size);
> +	if (ret < 0)
> +		return ret;
> +	name[ret] = '\0';
> +
> +	for (m = 0; m < XE_SRIOV_SCHED_GROUPS_MODES_COUNT; m++)
> +		if (sysfs_streq(name, sched_group_mode_to_string(m)))
> +			break;
> +
> +	if (m == XE_SRIOV_SCHED_GROUPS_MODES_COUNT)
> +		return -EINVAL;
> +
> +	xe_pm_runtime_get(gt_to_xe(gt));

	guard(xe_pm_runtime)(xe);

> +	ret = xe_gt_sriov_pf_policy_set_sched_groups_mode(gt, m);
> +	xe_pm_runtime_put(gt_to_xe(gt));
> +
> +	return (ret < 0) ? ret : size;
> +}
> +
> +static const struct file_operations sched_groups_fops = {
> +	.owner = THIS_MODULE,
> +	.open = sched_groups_open,
> +	.read = seq_read,
> +	.write = sched_groups_write,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static void pf_add_sched_groups(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("sched_groups_mode", 0644, parent, parent, &sched_groups_fops);
> +}
> +
>  /*
>   *      /sys/kernel/debug/dri/BDF/
>   *      ├── sriov
> @@ -528,6 +643,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);

at this point we should know whether we support EGS or not,
so we should create EGS files only if EGS is supported

>  
>  		drm_debugfs_create_files(pf_info, ARRAY_SIZE(pf_info), dent, minor);
>  	}
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c
> index c7f1ea8eb9c5..3c5fc1b5f281 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c
> @@ -507,12 +507,12 @@ pf_policy_has_sched_group_modes(struct xe_gt *gt, unsigned long mask)
>  	return gt->sriov.pf.policy.guc.sched_groups.supported_modes & mask;
>  }
>  
> -static bool pf_policy_has_valid_sched_group_modes(struct xe_gt *gt)
> +bool xe_sriov_gt_pf_policy_has_valid_sched_group_modes(struct xe_gt *gt)
>  {
>  	return pf_policy_has_sched_group_modes(gt, ~BIT(XE_SRIOV_SCHED_GROUPS_NONE));
>  }
>  
> -static bool pf_policy_has_sched_group_mode(struct xe_gt *gt, u32 mode)

public function needs kernel-doc

> +bool xe_sriov_gt_pf_policy_has_sched_group_mode(struct xe_gt *gt, u32 mode)

if in the single series like this one, we know that we will need some function,
I guess it is ok to define it as public on the first use, even if it was initially
used only locally - this will make smaller diff on next patch like this one

>  {
>  	return pf_policy_has_sched_group_modes(gt, BIT(mode));
>  }
> @@ -553,7 +553,7 @@ static int pf_provision_sched_groups(struct xe_gt *gt, u32 mode)
>  	xe_gt_assert(gt, IS_SRIOV_PF(gt_to_xe(gt)));
>  	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
>  
> -	if (!pf_policy_has_sched_group_mode(gt, mode))
> +	if (!xe_sriov_gt_pf_policy_has_sched_group_mode(gt, mode))
>  		return -EINVAL;
>  
>  	/* already in the desired mode */
> @@ -588,7 +588,7 @@ static int pf_reprovision_sched_groups(struct xe_gt *gt)
>  	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
>  
>  	/* We only have something to provision if we have possible groups */
> -	if (!pf_policy_has_valid_sched_group_modes(gt))
> +	if (!xe_sriov_gt_pf_policy_has_valid_sched_group_modes(gt))
>  		return 0;
>  
>  	return __pf_provision_sched_groups(gt, gt->sriov.pf.policy.guc.sched_groups.current_mode);
> @@ -615,7 +615,7 @@ int xe_gt_sriov_pf_policy_set_sched_groups_mode(struct xe_gt *gt, u32 value)
>  {
>  	int err;
>  
> -	if (!(pf_policy_has_valid_sched_group_modes(gt)))
> +	if (!(xe_sriov_gt_pf_policy_has_valid_sched_group_modes(gt)))
>  		return -ENODEV;
>  
>  	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h b/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h
> index 89aa3af6cc7d..13550cff7c00 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h
> @@ -17,6 +17,8 @@ int xe_gt_sriov_pf_policy_set_reset_engine(struct xe_gt *gt, bool enable);
>  bool xe_gt_sriov_pf_policy_get_reset_engine(struct xe_gt *gt);
>  int xe_gt_sriov_pf_policy_set_sample_period(struct xe_gt *gt, u32 value);
>  u32 xe_gt_sriov_pf_policy_get_sample_period(struct xe_gt *gt);
> +bool xe_sriov_gt_pf_policy_has_valid_sched_group_modes(struct xe_gt *gt);
> +bool xe_sriov_gt_pf_policy_has_sched_group_mode(struct xe_gt *gt, u32 mode);
>  int xe_gt_sriov_pf_policy_set_sched_groups_mode(struct xe_gt *gt, u32 value);
>  bool xe_gt_sriov_pf_policy_sched_groups_enabled(struct xe_gt *gt);
>