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

[S, Shirish]

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


>
>
>>>
>>>>> [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;
>>>>>           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;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20221006/8f4defe7/attachment.htm>