Re: [PATCH] drm/xe/guc_pc: Add ignore efficient frequency support

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 9/16/2025 10:52 PM, Anirban, Sk wrote:
> Hi,
>
> On 17-09-2025 10:59, Nilawar, Badal wrote:
>>
>>
>> On 16-09-2025 19:35, Sk Anirban wrote:
>>> When setting the minimum frequency, GuC automatically adjusts it to
>>> efficient levels, potentially overriding user requests.
>>> This patch adds the slpc_ignore_eff_freq sysfs interface to disable
>>> this behavior, allowing users to enforce their exact minimum frequency
>>> settings.
>>> Signed-off-by: Sk Anirban<sk.anirban@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_gt_freq.c      | 36 ++++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_guc_pc.c       | 49 
>>> ++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/xe/xe_guc_pc.h       |  1 +
>>>   drivers/gpu/drm/xe/xe_guc_pc_types.h |  2 ++
>>>   4 files changed, 86 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.c 
>>> b/drivers/gpu/drm/xe/xe_gt_freq.c
>>> index 4ff1b6b58d6b..fb16fa5c6a0f 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_freq.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt_freq.c
>>> @@ -227,6 +227,41 @@ static ssize_t max_freq_store(struct kobject 
>>> *kobj,
>>>   }
>>>   static struct kobj_attribute attr_max_freq = __ATTR_RW(max_freq);
>>>   +static ssize_t slpc_ignore_eff_freq_show(struct kobject *kobj,
>>> +                     struct kobj_attribute *attr, char *buf)
>>> +{
>>> +    struct device *dev = kobj_to_dev(kobj);
>>> +    struct xe_guc_pc *pc = dev_to_pc(dev);
>>> +
>>> +    return sysfs_emit(buf, "%u\n", pc->ignore_eff_freq);
>>> +}
>>> +
>>> +static ssize_t slpc_ignore_eff_freq_store(struct kobject *kobj,
>>> +                      struct kobj_attribute *attr,
>>> +                      const char *buff, size_t count)
>>> +{
>>> +    struct device *dev = kobj_to_dev(kobj);
>>> +    struct xe_guc_pc *pc = dev_to_pc(dev);
>>> +    u32 val;
>>> +    ssize_t ret;
>>> +
>>> +    ret = kstrtou32(buff, 0, &val);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (val > 1)
>>> +        return -EINVAL;
>>> +
>>> +    xe_pm_runtime_get(dev_to_xe(dev));
>>> +    ret = xe_guc_pc_set_ignore_eff_freq(pc, val);
>>> +    xe_pm_runtime_put(dev_to_xe(dev));
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return count;
>>> +}
>>> +static struct kobj_attribute attr_slpc_ignore_eff_freq = 
>>> __ATTR_RW(slpc_ignore_eff_freq);
>>> +
>>>   static ssize_t power_profile_show(struct kobject *kobj,
>>>                     struct kobj_attribute *attr,
>>>                     char *buff)
>>> @@ -263,6 +298,7 @@ static const struct attribute *freq_attrs[] = {
>>>       &attr_rpn_freq.attr,
>>>       &attr_min_freq.attr,
>>>       &attr_max_freq.attr,
>>> +    &attr_slpc_ignore_eff_freq.attr,
>>>       &attr_power_profile.attr,
>>>       NULL
>>>   };
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
>>> b/drivers/gpu/drm/xe/xe_guc_pc.c
>>> index 53fdf59524c4..0229a89e3ff8 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>> @@ -336,6 +336,8 @@ static void pc_set_cur_freq(struct xe_guc_pc 
>>> *pc, u32 freq)
>>>     static int pc_set_min_freq(struct xe_guc_pc *pc, u32 freq)
>>>   {
>>> +    int ret;
>>> +
>>>       /*
>>>        * Let's only check for the rpn-rp0 range. If max < min,
>>>        * min becomes a fixed request.
>>> @@ -347,8 +349,10 @@ static int pc_set_min_freq(struct xe_guc_pc 
>>> *pc, u32 freq)
>>>        * GuC policy is to elevate minimum frequency to the efficient 
>>> levels
>>>        * Our goal is to have the admin choices respected.
>>>        */
>>> -    pc_action_set_param(pc, SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
>>> -                freq < pc->rpe_freq);
>> This code already respects the user’s request by disabling efficient 
>> frequency when the requested frequency is lower than RPE. So, why is 
>> there a need for an additional sysfs entry to disable efficient 
>> frequency?
>>
>> Regards,
>> Badal
> Currently, the SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY parameter is only 
> set when pc_set_min_freq() is called—and even then, only if the 
> requested minimum frequency is below RPe. However, after the driver 
> loads or when the requested frequency is above RPe, there's no way to 
> manually enable this parameter. As a result, we see frequency 
> fluctuations during IGT testing.

Questioning my own reasoning on the need to do this. Do we have a 
scenario that is failing currently? As Badal mentioned, IGT tests can 
just set min to <RPe, that should automatically disable RPe. Am I 
missing something? The added complexity here just doesn't feel 
justified(unless we have a known failing case that requires it).

Thanks,

Vinay.

>
> Thanks,
> Anirban
>
>>> +    ret = pc_action_set_param(pc, 
>>> SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
>>> +                  freq < pc->rpe_freq || pc->ignore_eff_freq);
>>> +    if (ret)
>>> +        xe_gt_err(pc_to_gt(pc), "Failed to set efficient freq\n");
>>>         return pc_action_set_param(pc,
>>> SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>>> @@ -737,6 +741,45 @@ int xe_guc_pc_set_max_freq(struct xe_guc_pc 
>>> *pc, u32 freq)
>>>       return xe_guc_pc_set_max_freq_locked(pc, freq);
>>>   }
>>>   +int xe_guc_pc_set_ignore_eff_freq(struct xe_guc_pc *pc, bool val)
>>> +{
>>> +    int ret;
>>> +    u32 min_freq;
>>> +
>>> +    guard(mutex)(&pc->freq_lock);
>>> +
>>> +    if (!pc->freq_ready)
>>> +        return -EAGAIN;
>>> +
>>> +    if (val) {
>>> +        ret = xe_guc_pc_get_min_freq_locked(pc, &min_freq);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    ret = pc_action_set_param(pc, 
>>> SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY, val);
>>> +    if (ret) {
>>> +        xe_gt_err(pc_to_gt(pc), "Failed to set ignore efficient 
>>> freq(%d): %pe\n",
>>> +              val, ERR_PTR(ret));
>>> +        return ret;
>>> +    }
>>> +
>>> +    pc->ignore_eff_freq = val;
>>> +
>>> +    if (val) {
>>> +        ret = pc_action_set_param(pc,
>>> + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>>> +                      min_freq);
>>> +        if (ret) {
>>> +            xe_gt_err(pc_to_gt(pc), "Failed to set min freq to %u: 
>>> %pe\n",
>>> +                  min_freq, ERR_PTR(ret));
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   /**
>>>    * xe_guc_pc_c_status - get the current GT C state
>>>    * @pc: XE_GuC_PC instance
>>> @@ -1382,6 +1425,8 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>>>       if (err)
>>>           return err;
>>>   +    pc->ignore_eff_freq = false;
>>> +
>>>       bo = xe_managed_bo_create_pin_map(xe, tile, size,
>>>                         XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>>>                         XE_BO_FLAG_GGTT |
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.h 
>>> b/drivers/gpu/drm/xe/xe_guc_pc.h
>>> index 0e31396f103c..3a87d0da609d 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.h
>>> @@ -31,6 +31,7 @@ int xe_guc_pc_get_min_freq(struct xe_guc_pc *pc, 
>>> u32 *freq);
>>>   int xe_guc_pc_set_min_freq(struct xe_guc_pc *pc, u32 freq);
>>>   int xe_guc_pc_get_max_freq(struct xe_guc_pc *pc, u32 *freq);
>>>   int xe_guc_pc_set_max_freq(struct xe_guc_pc *pc, u32 freq);
>>> +int xe_guc_pc_set_ignore_eff_freq(struct xe_guc_pc *pc, bool val);
>>>   int xe_guc_pc_set_power_profile(struct xe_guc_pc *pc, const char 
>>> *buf);
>>>   void xe_guc_pc_get_power_profile(struct xe_guc_pc *pc, char 
>>> *profile);
>>>   diff --git a/drivers/gpu/drm/xe/xe_guc_pc_types.h 
>>> b/drivers/gpu/drm/xe/xe_guc_pc_types.h
>>> index 5e4ea53fbee6..c3b44598507b 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_pc_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc_types.h
>>> @@ -37,6 +37,8 @@ struct xe_guc_pc {
>>>       struct mutex freq_lock;
>>>       /** @freq_ready: Only handle freq changes, if they are really 
>>> ready */
>>>       bool freq_ready;
>>> +    /** @ignore_eff_freq: Ignore efficient frequency */
>>> +    bool ignore_eff_freq;
>>>       /** @power_profile: Base or power_saving profile */
>>>       u32 power_profile;
>>>   };
>