Dear Christian,
Am 22.06.20 um 19:41 schrieb Christian König:
> Am 22.06.20 um 19:25 schrieb Paul Menzel:
>> Am 22.06.20 um 15:39 schrieb Christian König:
>>> Am 19.06.20 um 20:50 schrieb Paul Menzel:
>>>> Currently, besides there is no explicit message, that DPM is disabled.
>>>> The user would need to know, that the missing success line indicates
>>>> that.
>>>>
>>>> [drm] amdgpu: dpm initialized
>>>>
>>>> So, add an explicit message, and make it log level warning, as
>>>> disabling
>>>> dpm is not the default, and device performance will most likely suffer.
>>>>
>>>> Resolves:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1173&data=02%7C01%7Cchristian.koenig%40amd.com%7C72d0b71d439e46d6253f08d816d150c6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637284435558396492&sdata=5EXY1o1zwXJRzN9fqpUg%2BQNJGB3zAlWKnGWsdFXRcjA%3D&reserved=0
>>
>> That URL is not mine. ;-)
>>
>>>> Cc: amd-gfx at lists.freedesktop.org
>>>> Signed-off-by: Paul Menzel <pmenzel at molgen.mpg.de>
>>>> ---
>>>> v2: Use new print helpers, and inform user about effects.
>>>>
>>>> drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 4 +++-
>>>> drivers/gpu/drm/amd/amdgpu/si_dpm.c | 4 +++-
>>>> 2 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> index f054ded902f2..c601587c6d59 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c
>>>> @@ -3014,8 +3014,10 @@ static int kv_dpm_sw_init(void *handle)
>>>> adev->pm.current_mclk = adev->clock.default_mclk;
>>>> adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>>> - if (amdgpu_dpm == 0)
>>>> + if (amdgpu_dpm == 0) {
>>>> + drm_warn(adev, "amdgpu: dpm disabled per parameter. Your
>>>> graphics device will run with lower clocks impacting graphics
>>>> performance.\n");
>>>
>>> I'm not very keen about this. When an user specifies that DPM
>>> shouldn't be used the driver doesn't need to inform the user about
>>> this once more.
>>>
>>> In other words shooting in your own foot is supposed to hurt.
>>
>> Maybe. The other point of view is, how does having the clarity in the
>> logs hurt?
>
> Well, you are spamming the logs with a warning about an intentional
> behavior.
>
>> For example, if the user added the parameter intentionally, maybe they
>> made a typo, and it’s actually not applied. Or there is a bug in the
>> parameter handling. Having explicit log messages is good in my opinion.
>>
>> Secondly, the parameter could have been left there unintentionally.
>> Having the message in the logs, makes the user aware of that.
>
> And exactly for this reason the kernel command line is printed as the
> second line of the logs.
No, I disagree. Having the parameters listed, and knowing the actual
effects are two totally different things. It’s user interface design 101
to give clear feedback. I think, you are looking through the glasses of
a developer, and not of a non-developer.
> Duplicating this in each driver is not only overkill, but also very
> error prone.
What drivers do you mean, and what is error prone?
> Sorry, but this is absolutely don't think that this is a good idea.
Despite other subsystems actually “spamming” the logs for parameters
given on the command line, here is my motivation.
Unfortunately, there is a serious issue with first(?) generation Ryzen
CPUs, and maybe even external AMD graphics cards [1]. In comment 29 it
is suggested to use `amdgpu.dpm=0` [1], and further it’s said, it’s
disabling power management. Reading the comments further [2], the user
gets the impression turning off power management will cause the device
to be operated at the highest performance [3].
Trying this option, I was surprised of the degraded performance, and
only in #radeon at irc.freenode.net my question was answered, saying that
it’s actually depending on the system firmware how clocks are set up,
and it’s common to use the *lowest* clocks. So the warning message,
would have helped me a lot.
So, especially with these option used by “normal” users, having clear
feedback on specified option, especially those decreasing performance,
is very important.
Kind regards,
Paul
>>>> return 0;
>>>> + }
>>>> INIT_WORK(&adev->pm.dpm.thermal.work,
>>>> amdgpu_dpm_thermal_work_handler);
>>>> mutex_lock(&adev->pm.mutex);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> index f7edc1d50df4..1f35d5a36300 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
>>>> @@ -7687,8 +7687,10 @@ static int si_dpm_sw_init(void *handle)
>>>> adev->pm.current_mclk = adev->clock.default_mclk;
>>>> adev->pm.int_thermal_type = THERMAL_TYPE_NONE;
>>>> - if (amdgpu_dpm == 0)
>>>> + if (amdgpu_dpm == 0) {
>>>> + drm_warn(adev, "amdgpu: dpm disabled per parameter. Your
>>>> graphics device will run with lower clocks impacting graphics
>>>> performance.\n");
>>>> return 0;
>>>> + }
>>>> ret = si_dpm_init_microcode(adev);
>>>> if (ret)
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=206903#c29
[2]: https://bugzilla.kernel.org/show_bug.cgi?id=206903#c33