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

[S, Shirish]

On 10/6/2022 10:51 PM, Leo Li wrote:
>
>
>
> 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.

Done.

Patch here: https://patchwork.freedesktop.org/patch/506242/

Regards,

Shirish S

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