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

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

On 12/9/2025 7:07 AM, Michal Wajdeczko wrote:
>
> On 12/9/2025 1:36 AM, Daniele Ceraolo Spurio wrote:
>>
>> 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)
> I would rather try to keep debugfs as close as possible to driver POV and GuC ABI, not as exposing GuC POV
> so IMO in case of no groups configured, we should show them as empty == not configured, not fully configured
>
> but we may want to change the approach once we promote EGS to sysfs
> where for consistency we should show "default group" with all engines, where EGS is N/A or disabled

My idea came from the fact that even with EGS off in sysfs we 
technically still have:

group0: rcs0, ccs0, bcs0, bcs9
group1: vcs0, vcs2, vecs0, vecs1

So the default state is 2 groups. But as you mentioned we can re-discuss 
when moving to sysfs.

>
>>>> +    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.
> it's not about newlines
>
> /**
>   * drm_puts - print a const string to a &drm_printer stream
>   * @p: the &drm printer
>   * @str: const string
>   *
>   * Allow &drm_printer types that have a constant string
>   * option to use it.
>   */

Sorry, I just assumed this would add a newline like the normal puts to 
stdout. Will switch to using it.

Daniele

>>>> +
>>>> +        if (mode == current_mode)
>>>> +            drm_printf(&p, "[");
>>>> +
>>>> +        drm_printf(&p, "%s", sched_group_mode_to_string(mode));
> only here you need to use drm_printf, all else could be drm_puts
>
>>>> +
>>>> +        if (mode == current_mode)
>>>> +            drm_printf(&p, "]");
>>>> +    }
>>>> +
>>>> +    drm_printf(&p, "\n");
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +