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

["S, Shirish" <sshankar@xxxxxxx>]

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