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

[Leo Li]

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?


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