<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 10/6/2022 4:33 AM, Leo Li wrote:<br>
</div>
<blockquote type="cite" cite="mid:38b74412-dc3a-37da-55dd-e2327fe59769@amd.com">
<br>
<br>
On 2022-10-03 11:26, S, Shirish wrote:
<br>
<blockquote type="cite">Ping!
<br>
<br>
Regards,
<br>
<br>
Shirish S
<br>
<br>
On 9/30/2022 7:17 PM, S, Shirish wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 9/30/2022 6:59 PM, Harry Wentland wrote:
<br>
<blockquote type="cite">+Leo
<br>
<br>
On 9/30/22 06:27, Shirish S wrote:
<br>
<blockquote type="cite">[Why]
<br>
psr feature continues to be enabled for non capable links.
<br>
<br>
</blockquote>
Do you have more info on what issues you're seeing with
this?
<br>
</blockquote>
<br>
Code wise without this change we end up setting
"vblank_disable_immediate" parameter to false for the failing
links also.
<br>
<br>
Issue wise there is a remote chance of this leading to
eDP/connected monitor not lighting up.
<br>
</blockquote>
</blockquote>
<br>
I'm surprised psr_settings.psr_feature_enabled can be 'true'
before
<br>
amdgpu_dm_set_psr_caps() runs. it should default to 'false', and
it's
<br>
set early on during amdgpu_dm_initialize_drm_device() before any
other
<br>
psr-related code runs.
<br>
<br>
In other words, I don't expect psr_settings.psr_feature_enabled to
be
<br>
'true' on early return of dm_set_psr_caps().
<br>
<br>
What are the sequence of events that causes an issue for you?
<br>
</blockquote>
<p>psr_feature_enabled is set to true by default in
<a class="moz-txt-link-freetext" href="https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L4264">https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L4264</a>
for DCN 3.0 onwards<br>
</p>
<p>(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.)<br>
</p>
<p>Hence, the variable is set to true while entering
amdgpu_dm_set_psr_caps().</p>
<br>
<blockquote type="cite" cite="mid:38b74412-dc3a-37da-55dd-e2327fe59769@amd.com">
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
<blockquote type="cite">
<blockquote type="cite">[How]
<br>
disable the feature on links that are not capable of the
same.
<br>
<br>
Signed-off-by: Shirish S<a class="moz-txt-link-rfc2396E" href="mailto:shirish.s@amd.com"><shirish.s@amd.com></a>
<br>
---
<br>
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c |
10 ++++++++--
<br>
1 file changed, 8 insertions(+), 2 deletions(-)
<br>
<br>
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
<br>
index 8ca10ab3dfc1..f73af028f312 100644
<br>
---
a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
<br>
+++
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
<br>
@@ -60,11 +60,17 @@ static bool link_supports_psrsu(struct
dc_link *link)
<br>
*/
<br>
void amdgpu_dm_set_psr_caps(struct dc_link *link)
<br>
{
<br>
- if (!(link->connector_signal &
SIGNAL_TYPE_EDP))
<br>
+ if (!(link->connector_signal &
SIGNAL_TYPE_EDP)) {
<br>
+ DRM_ERROR("Disabling PSR as connector is not
eDP\n")
<br>
</blockquote>
I don't think we should log an error here.
<br>
</blockquote>
<br>
My objective of logging an error was to inform user/developer
that this boot PSR enablement had issues.
<br>
</blockquote>
</blockquote>
<br>
It's not really an issue, PSR simply cannot be enabled on non-eDP
or
<br>
disconnected links. </blockquote>
<p>Agree, the idea here is to avoid decisions being taken presuming
psr_feature_enabled being set on such links, like disabling <a moz-do-not-send="true" href="https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L4330">vblank_disable_immediate
</a>etc.,</p>
<p>Regards,</p>
<p>Shirish S<br>
</p>
<blockquote type="cite" cite="mid:38b74412-dc3a-37da-55dd-e2327fe59769@amd.com">However,
it is concerning if we enter this function
<br>
with psr_feature_enabled == true.
<br>
<br>
Thanks,
<br>
Leo
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
Am fine with moving it to INFO or remove it, if you insist.
<br>
<br>
Thanks for your comments.
<br>
<br>
Regards,
<br>
<br>
Shirish S
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+
link->psr_settings.psr_feature_enabled = false;
<br>
return;
<br>
+ }
<br>
- if (link->type == dc_connection_none)
<br>
+ if (link->type == dc_connection_none) {
<br>
+ DRM_ERROR("Disabling PSR as eDP connection type
is invalid\n")
<br>
</blockquote>
Same here, this doesn't warrant an error log.
<br>
<br>
Harry
<br>
<br>
<blockquote type="cite">+
link->psr_settings.psr_feature_enabled = false;
<br>
return;
<br>
+ }
<br>
if (link->dpcd_caps.psr_info.psr_version == 0)
{
<br>
link->psr_settings.psr_version =
DC_PSR_VERSION_UNSUPPORTED;
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>