Re: [PATCH 0/5] 0 MHz is not a valid current frequency

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 10/13/2021 7:28 PM, Alex Deucher wrote:
> On Wed, Oct 13, 2021 at 12:14 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>>
>>
>>
>> On 10/13/2021 8:40 AM, Luben Tuikov wrote:
>>> Some ASIC support low-power functionality for the whole ASIC or just
>>> an IP block. When in such low-power mode, some sysfs interfaces would
>>> report a frequency of 0, e.g.,
>>>
>>> $cat /sys/class/drm/card0/device/pp_dpm_sclk
>>> 0: 500Mhz
>>> 1: 0Mhz *
>>> 2: 2200Mhz
>>> $_
>>>
>>> An operating frequency of 0 MHz doesn't make sense, and this interface
>>> is designed to report only operating clock frequencies, i.e. non-zero,
>>> and possibly the current one.
>>>
>>> When in this low-power state, round to the smallest
>>> operating frequency, for this interface, as follows,
>>>
>>
>> Would rather avoid this -
>>
>> 1) It is manipulating FW reported value. If at all there is an uncaught
>> issue in FW reporting of frequency values, that is masked here.
>> 2) Otherwise, if 0MHz is described as GFX power gated case, this
>> provides a convenient interface to check if GFX is power gated.
>>
>> If seeing a '0' is not pleasing, consider changing to something like
>>          "NA" - not available (frequency cannot be fetched at the moment).
>>
> 
> I don't think this interface is really supposed to be the way to get
> the current clock, although some use it that way.  It's supposed to
> show the DPM states and which are active.  conflating the interface
> with other information confuses things in my opinion.  Why is the
> current clock less than the minimum clock?  Whether or not an IP is
> turned off or in deep sleep or not is independent of DPM states.  When
> the IP is active, it will never go below the minimum DPM level.  If we
> want to query deep sleep or gfxoff we can use the amdgpu_pm_info
> debugfs interface or add some other new interface.
> 

The idea of DPM level is deprecated with fine grained clock which 
provides only min and max. For fine grained clock, we fetch the current 
clock frequency and show it as an artificial DPM level between min/max. 
That itself should have confused users but it is not which means the 
users use the * to fetch the current frequency and not really bothered 
about the DPM level per se.

Also, some ASICs define 'min' as as the least possible freq (that 
happens during a throttle) and not the DPM level 0 min in the 
traditional sense (that is defined as idle frequency which doesn't come 
into min/max levels). It's usually from the idle frequency that GFX gets 
power gated. Showing a * against min in such cases would be confusing 
because that could be misinterpreted as a throttle scenario.

Thanks,
Lijo

> Alex
> 
> 
>> Thanks,
>> Lijo
>>
>>> $cat /sys/class/drm/card0/device/pp_dpm_sclk
>>> 0: 500Mhz *
>>> 1: 2200Mhz
>>> $_
>>>
>>> Luben Tuikov (5):
>>>     drm/amd/pm: Slight function rename
>>>     drm/amd/pm: Rename cur_value to curr_value
>>>     drm/amd/pm: Rename freq_values --> freq_value
>>>     dpm/amd/pm: Sienna: 0 MHz is not a current clock frequency
>>>     dpm/amd/pm: Navi10: 0 MHz is not a current clock frequency
>>>
>>>    .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   | 60 +++++++++------
>>>    .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c   | 73 ++++++++++++-------
>>>    2 files changed, 86 insertions(+), 47 deletions(-)
>>>