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

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

On 12/2/2025 7:03 PM, Daniele Ceraolo Spurio wrote:
> 
> 
> On 12/2/2025 7:52 AM, Michal Wajdeczko wrote:
>>
>> 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
> 
> ok
> 
>>
>> and if we check for EGS support earlier, see below,
>> then maybe this could be just an assert?
> 
> sure
> 
>>
>>> +        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
> 
> We actually don't. xe_sriov_init_late() (which is where the EGS init is called from) happens after xe_debugfs_register. xe_sriov_init() happens too early, so that's also not a good choice.
> I thought about moving xe_sriov_init_late to an earlier point, but I didn't want to mess with the general SRIOV flows. Thoughts?

add dent at xe_gt (like we have on xe_tile)

and move PF GT debugfs initialization from xe_gt_debugfs_register to xe_gt_sriov_pf_init() ?

but will take a closer look if this is safe ;)

> 
>>
>>>             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
> 
> ok
> 
>>
>>> +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
> 
> ok
> 
> Daniele
> 
>>
>>>   {
>>>       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);
>>>   
>