[PATCH 2/2] accel/ivpu: Show NPU frequency in sysfs

[Falkowski, Maciej]

On 4/11/2025 5:49 PM, Jeff Hugo wrote:

> On 4/8/2025 8:55 AM, Falkowski, Maciej wrote:
>> On 4/4/2025 5:13 PM, Jeff Hugo wrote:
>>
>>> On 4/1/2025 9:59 AM, Maciej Falkowski wrote:
>>>> From: Andrzej Kacprowski <Andrzej.Kacprowski at intel.com>
>>>>
>>>> Add sysfs files that show maximum and current
>>>> frequency of the NPU's data processing unit.
>>>> New sysfs entries:
>>>> - npu_max_frequency_mhz
>>>
>>> Don't you have an ioctl to get this, which is fixed in patch 1 of 
>>> the series?  Why duplicate that with a sysfs?
>>
>> We added this to keep it consistent with sysfs entry for current 
>> frequency.
>> This is more for a convenience but ioctl is still required as 
>> user-mode driver requires this.
>> Do you think it is justified having this consideration?
>
> The duplication is not great, but I understand the rational.
>
> Also, I think your last message was HTML formatted instead of plain 
> text.  I suggest you check your mail client settings.
Yes, it was set to automatic so I had not seen it before this occurence, 
thanks for pointing this out.
>
>>
>>>
>>>> - npu_current_frequency_mhz
>>>
>>> Do you have userspace code that consumes these?
>>
>> We do have a support for two device monitors in a userspace - 
>> resources[1] and  btop[2].
>> I develop myself a btop support and have already frequency included, 
>> I will also include it in resources.
>>
>> [1]: 
>> https://github.com/nokyan/resources/blob/main/src/utils/npu/intel.rs
>> <https://github.com/nokyan/resources/blob/main/src/utils/npu/ 
>> intel.rs>[2]: https://github.com/m-falkowski/btop
>
> It would be good to document this.  Without something in 
> Documentation, at least have this in the commit text.
I think it will be the best if we prepare appropriate documentation just 
like yours.
I will post kerneldoc fixes and doc in a separate series if that is not 
a problem.
Thank you for checking this.

Best regards,
Maciej
>
> -Jeff