Re: [PATCH v2 06/11] drm/xe/sriov: Add debugfs to enable scheduler groups

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 12/8/2025 3:38 PM, Michal Wajdeczko wrote:
>
> On 12/7/2025 12:04 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.
>>
>> v2: don't print anything if the feature is unsupported (Michal), add
>>      TODO for reworking init order to know if there are valid groups
>>      when we register debugfs, check for basic feature support.
> btw, recently in Xe we started to follow core kernel rule and put the
> change log under the --- line

ok

>
>> 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 | 126 ++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c  |  19 +--
>>   drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.h  |   1 +
>>   3 files changed, 139 insertions(+), 7 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 5123ff1fb116..1be23809e624 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_debugfs.c
>> @@ -156,6 +156,131 @@ 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";
> maybe we should be consistent and use either:
>
> 	"none" / XE_SRIOV_SCHED_GROUPS_NONE
> or
> 	"disabled" / XE_SRIOV_SCHED_GROUPS_DISABLED

I am ok with switching both to disabled. Or maybe we should just go with 
"all_gt_engines", to indicate that all engines in one GT are in the same 
group? Because AFAIU from the GuC POV you always have groups, the 
question is if more than 1 group is actually active (and it also counts 
as an actual mode instead of just being the disabled state)

>> +	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_multi_group_modes(gt))
>> +		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, " ");
> nit: drm_puts() ?

I'd prefer not to add newlines here.

>
>> +
>> +		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_multi_group_modes(gt))
>> +		return -ENODEV;
> maybe we should drop this condition?
>
> if we don't have multi-groups, we will still display:
>
> 	[disabled]
>
> so for consistency we should allow:
>
> 	echo disabled > sched_groups_mode

Sure, that makes things easier.

>
>> +
>> +	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;
>> +
>> +	guard(xe_pm_runtime)(gt_to_xe(gt));
>> +	ret = xe_gt_sriov_pf_policy_set_sched_groups_mode(gt, m);
>> +
>> +	return (ret < 0) ? ret : size;
> we can drop ( ) here

ok

>
>> +}
>> +
>> +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));
>> +
>> +	/*
>> +	 * TODO: we currently call this function before we initialize scheduler
>> +	 * groups, so at this point in time we don't know if there are any
>> +	 * valid groups on the GT and we can't selectively register the debugfs
>> +	 * only if there are any. Therefore, we always register the debugfs
>> +	 * files if we're on a platform that has support for groups.
>> +	 * We should rework the flow so that debugfs is registered after the
>> +	 * policy init, so that we check if there are valid groups before
>> +	 * adding the debugfs files.
>> +	 */
>> +	if (!xe_sriov_gt_pf_policy_has_sched_groups_support(gt))
>> +		return;
>> +
>> +	debugfs_create_file("sched_groups_mode", 0644, parent, parent, &sched_groups_fops);
>> +}
>> +
>>   /*
>>    *      /sys/kernel/debug/dri/BDF/
>>    *      ├── sriov
>> @@ -531,6 +656,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);
>>   
>>   		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 6a682d788b02..2cafacac5d8e 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_policy.c
>> @@ -451,19 +451,24 @@ static void pf_sched_group_media_slices(struct xe_gt *gt, u32 **masks, u32 *num_
>>   	*num_masks = GUC_MAX_ENGINE_CLASSES * groups;
>>   }
>>   
>> -static void pf_init_sched_groups(struct xe_gt *gt)
> missing kernel doc

will add.

>
>> +bool xe_sriov_gt_pf_policy_has_sched_groups_support(struct xe_gt *gt)
>>   {
>> -	int m;
>> -
>> -	xe_gt_assert(gt, IS_SRIOV_PF(gt_to_xe(gt)));
>> -
>>   	/*
>>   	 * The GuC supports scheduler groups from v70.53.0, but a fix for it has
>>   	 * been merged in v70.55.1, so we require the latter. The feature is
>>   	 * also only enabled on BMG and newer FW.
>>   	 */
>> -	if (GUC_FIRMWARE_VER(&gt->uc.guc) < MAKE_GUC_VER(70, 55, 1) ||
>> -	    gt_to_xe(gt)->info.platform < XE_BATTLEMAGE)
>> +	return GUC_FIRMWARE_VER(&gt->uc.guc) >= MAKE_GUC_VER(70, 55, 1) &&
>> +	       gt_to_xe(gt)->info.platform >= XE_BATTLEMAGE;
> and maybe we can introduce this function in patch 2/11 to avoid this diff?

will do.

Daniele

>
>> +}
>> +> +static void pf_init_sched_groups(struct xe_gt *gt)
>> +{
>> +	int m;
>> +
>> +	xe_gt_assert(gt, IS_SRIOV_PF(gt_to_xe(gt)));
>> +
>> +	if (!xe_sriov_gt_pf_policy_has_sched_groups_support(gt))
>>   		return;
>>   
>>   	/*
>> 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 ceaf797ca21b..f5ea44dcaf82 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,7 @@ 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_sched_groups_support(struct xe_gt *gt);
>>   bool xe_sriov_gt_pf_policy_has_multi_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);