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