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

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

[Shirish S]

[Why]
psr feature continues to be enabled for non capable links.

[How]
disable the feature on links that are not capable of the same.

Signed-off-by: Shirish S <shirish.s@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")
+		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")
+		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;
-- 
2.17.1

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

[Harry Wentland]

+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? 

> [How]
> disable the feature on links that are not capable of the same.
> 
> Signed-off-by: Shirish S <shirish.s@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.

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

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

[S, Shirish]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]


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.

>
>> [How]
>> disable the feature on links that are not capable of the same.
>>
>> Signed-off-by: Shirish S<shirish.s@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.

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;

[-- Attachment #2: Type: text/html, Size: 3976 bytes --]

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

[S, Shirish]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

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.
>
>>> [How]
>>> disable the feature on links that are not capable of the same.
>>>
>>> Signed-off-by: Shirish S<shirish.s@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.
>
> 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;

[-- Attachment #2: Type: text/html, Size: 4241 bytes --]

Re: [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@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;

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

[S, Shirish]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 7869 bytes --]

Re: [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@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;

Re: [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@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;