[PATCH] drm/amd/display: disable psr whenever applicable

[Leo Li]

On 2022-10-06 03:46, S, Shirish wrote:
> 
> On 10/6/2022 4:33 AM, Leo Li wrote:
>>
>>
>> On 2022-10-03 11:26, S, Shirish wrote:
>>> Ping!
>>>
>>> Regards,
>>>
>>> Shirish S
>>>
>>> On 9/30/2022 7:17 PM, S, Shirish wrote:
>>>>
>>>>
>>>> On 9/30/2022 6:59 PM, Harry Wentland wrote:
>>>>> +Leo
>>>>>
>>>>> On 9/30/22 06:27, Shirish S wrote:
>>>>>> [Why]
>>>>>> psr feature continues to be enabled for non capable links.
>>>>>>
>>>>> Do you have more info on what issues you're seeing with this?
>>>>
>>>> Code wise without this change we end up setting 
>>>> "vblank_disable_immediate" parameter to false for the failing links 
>>>> also.
>>>>
>>>> Issue wise there is a remote chance of this leading to eDP/connected 
>>>> monitor not lighting up.
>>
>> I'm surprised psr_settings.psr_feature_enabled can be 'true' before
>> amdgpu_dm_set_psr_caps() runs. it should default to 'false', and it's
>> set early on during amdgpu_dm_initialize_drm_device() before any other
>> psr-related code runs.
>>
>> In other words, I don't expect psr_settings.psr_feature_enabled to be
>> 'true' on early return of dm_set_psr_caps().
>>
>> What are the sequence of events that causes an issue for you?
> 
> psr_feature_enabled is set to true by default in 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L4264 for DCN 3.0 onwards
> 
> (Also, in ChromeOS wherein KMS driver is statically built in kernel, we 
> set PSR feature  as enabled as command-line argument via 
> amdgpu_dc_feature_mask.)
> 
> Hence, the variable is set to true while entering amdgpu_dm_set_psr_caps().

Hmm, that is a local variable in the function, not the same as
link->psr_settings.psr_feature_enabled. Unless I'm missing something, it
looks like link->psr_settings.psr_feature_enabled is never set to true.

More below,

> 
> 
>>
>>
>>>>
>>>>>> [How]
>>>>>> disable the feature on links that are not capable of the same.
>>>>>>
>>>>>> Signed-off-by: Shirish S<shirish.s at amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c | 10 
>>>>>> ++++++++--
>>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c 
>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
>>>>>> index 8ca10ab3dfc1..f73af028f312 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
>>>>>> @@ -60,11 +60,17 @@ static bool link_supports_psrsu(struct dc_link 
>>>>>> *link)
>>>>>>    */
>>>>>>   void amdgpu_dm_set_psr_caps(struct dc_link *link)
>>>>>>   {
>>>>>> -    if (!(link->connector_signal & SIGNAL_TYPE_EDP))
>>>>>> +    if (!(link->connector_signal & SIGNAL_TYPE_EDP)) {
>>>>>> +        DRM_ERROR("Disabling PSR as connector is not eDP\n")
>>>>> I don't think we should log an error here.
>>>>
>>>> My objective of logging an error was to inform user/developer that 
>>>> this boot PSR enablement had issues.
>>
>> It's not really an issue, PSR simply cannot be enabled on non-eDP or
>> disconnected links. 
> 
> Agree, the idea here is to avoid decisions being taken presuming 
> psr_feature_enabled being set on such links, like disabling 
> vblank_disable_immediate 
> <https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L4330>etc.,
> 
> Regards,
> 
> Shirish S
> 
>> However, it is concerning if we enter this function
>> with psr_feature_enabled == true.
>>
>> Thanks,
>> Leo
>>
>>>>
>>>> Am fine with moving it to INFO or remove it, if you insist.
>>>>
>>>> Thanks for your comments.
>>>>
>>>> Regards,
>>>>
>>>> Shirish S
>>>>
>>>>>> + link->psr_settings.psr_feature_enabled = false;

Never the less, explicitly setting to false rather than leaving it as
default sounds like a good idea to me.

But I don't see how this fixes an issue.

If this is a readability fix, I suggest changing commit title and
description to reflect that.

Thanks,
Leo

>>>>>>           return;
>>>>>> +    }
>>>>>>   -    if (link->type == dc_connection_none)
>>>>>> +    if (link->type == dc_connection_none) {
>>>>>> +        DRM_ERROR("Disabling PSR as eDP connection type is 
>>>>>> invalid\n")
>>>>> Same here, this doesn't warrant an error log.
>>>>>
>>>>> Harry
>>>>>
>>>>>> + link->psr_settings.psr_feature_enabled = false;
>>>>>>           return;
>>>>>> +    }
>>>>>>         if (link->dpcd_caps.psr_info.psr_version == 0) {
>>>>>>           link->psr_settings.psr_version = 
>>>>>> DC_PSR_VERSION_UNSUPPORTED;