Re: [PATCH v2 2/2] ACPI: CPPC: Add ospm_nominal_perf support

On 13/05/26 21:13, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c
>>>> b/drivers/cpufreq/cppc_cpufreq.c
>>>> index 7e7f9dfb7a24..d06cba963550 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -715,6 +715,16 @@ static int cppc_cpufreq_cpu_init(struct
>>>> cpufreq_policy *policy)
>>>>               goto out;
>>>>       }
>>>>
>>>> +     /*
>>>> +      * Initialize OSPM Nominal Performance to inform firmware of
>>>> +      * OSPM's nominal level. Performance above this value = boost;
>>>> +      * below = throttle. Uses platform nominal by default.
>>>> +      */
>>>> +     ret = cppc_set_ospm_nominal_perf(cpu, caps->nominal_perf);
>>>> +     if (ret && ret != -EOPNOTSUPP)
>>>> +             pr_debug("Failed to set ospm_nominal_perf for CPU%d:
>>>> %d\n",
>>>> +                      cpu, ret);
>>>> +
>>>
>>> IIUC, if (ospm_nominal_perf == nominal_perf), the firmware should
>>> not behave differently. Is this really useful ?
>>>
>>
>> Right, it's a no-op from the firmware's side. The init was only so that
>> sysfs would show a value (platform nominal) before any userspace write.
>> Will drop it in v3 and return 0 from sysfs until userspace writes a
>> value.
>>
>>
>>> ------------
>>>
>>> Also this seems like there will need some synchronization
>>> mechanism to keep-up with the boost state.
>>>
>>> If the ospm_nominal_perf is lowered and boost is disabled,
>>> a freq. update should happen. IMO it looks like this could
>>> be handled with (another) freq_qos_request.
>>>
>>> This new freq_qos_request, if we name it ospm_nominal_freq_req,
>>> should only be taken into account if boost is disabled.
>>> Otherwise, if boost is enabled, ospm_nominal_freq_req
>>> should be ignored.
>>>
>>
>> Agreed, will add the new freq_qos_request in a follow-up patch.
>>
>>> ------------
>>>
>>> Also, the function seems to set the ospm_nominal_freq for
>>> a single CPU when the policy might be common for multiple
>>> CPUs right ?
>>
>> In v3, after dropping the change from cppc_cpufreq_cpu_init,
>> the problem won't come in this specific instance.
>>
>>
>>>
>>> The issues this field raises seems similar to the auto_sel
>>> ones. I.e. :
>>>
>>> - concurrency accesses + need for a scratch value
>>>
>>> - what should happen when unloading the driver
>>>
>>> - the value can be set for single CPUs but we might
>>> want to have the same value for the whole policy
>>>
>>> Maybe a common solution should be found.
>>> (I m not suggesting anything right now unfortunately).
>>>
>>
>> One way to address this is to move the sysfs from per-CPU acpi_cppc to
>> a per-policy node under cpufreq (ospm_nominal_perf_freq, kHz).
>
> Yes right, would it make sense to also move the "ospm_nominal_perf"
> to the cpu_data ?
>

Yes, moved to struct cppc_cpudata in v3.

>
>> In the sysfs callback, we can convert kHz to perf and write the register
>> on every CPU in policy->cpus.
>> Concurrency is already covered by policy->rwsem at the cpufreq layer.
>> This is similar to how we were handling min/max_perf in earlier version.
>
> Yes right. Something I don't really understand is that similarly to
> auto_sel,
> the sysfs rwsem protects the state of the cpu_data->auto_sel field,
> but we don't really have any guarantee that another driver won't modify
> the firmware state right ? (maybe it's just not worth ?)
>

Yes, the cached value is updated only by the cpufreq sysfs store path.
A direct caller of cppc_set_ospm_nominal_perf() would bypass it.
cpufreq is the only caller today, so it doesn't surface in practice.
Same applies to auto_sel, and as you noted, it's probably not worth
chasing until we have more callers.

Thank you,
Sumit Gupta


>
>> Does this approach make sense?
>
> Yes right thanks !
>
>
>>
>> Thank you,
>> Sumit Gupta
>>
>> ....
>>
>>