[PATCH v2 3/3] drm/amdgpu: Warn about disabled DPM

[Christian König]

Am 22.06.20 um 23:41 schrieb Paul Menzel:
> [SNIP]
>>> 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.

Well my job is to see this as a maintainer and keep the driver simple 
and clean so that we can easily maintain and extend it, but at the same 
time it should obviously give the best experience to end users.

So I'm certainly weighting this, but currently I don't see a good 
argument to add something like this.

See essentially you are requesting that an intended behavior should give 
a warning. That is like telling the user to not do something he wants.

If an user doesn't knew what he is doing, that's obviously a big problem 
as well. But I would say in this case we should improve the documentation.

Adding a warning would be justified for options like experimental 
features which could damage the hardware by going beyond the device 
capabilities for example.

>> Duplicating this in each driver is not only overkill, but also very 
>> error prone.
>
> What drivers do you mean, and what is error prone?

Individual hardware drivers in general.

>
>> 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.

Yeah ok, but that hardware falls back to the lowest possible performance 
setting as safety precaution when turn of power management is common 
knowledge.

In other words you are writing on the cup of coffee "Beware content is hot".

>
> So, especially with these option used by “normal” users, having clear 
> feedback on specified option, especially those decreasing performance, 
> is very important.

Well, what we could do is improve the documentation of the module option.

Regards,
Christian.

>
>
> Kind regards,
>
> Paul