Re: [PATCH] drm/msm/dp: add module parameter for PSR

[Abhinav Kumar]

On 5/23/2023 8:24 AM, Johan Hovold wrote:
> On Fri, May 12, 2023 at 09:13:04PM +0300, Dmitry Baryshkov wrote:
> > On 28/04/2023 02:28, Abhinav Kumar wrote:
> > > On sc7280 where eDP is the primary display, PSR is causing
> > > IGT breakage even for basic test cases like kms_atomic and
> > > kms_atomic_transition. Most often the issue starts with below
> > > stack so providing that as reference
> > >
> > > Call trace:
> > >    dpu_encoder_assign_crtc+0x64/0x6c
> > >    dpu_crtc_enable+0x188/0x204
> > >    drm_atomic_helper_commit_modeset_enables+0xc0/0x274
> > >    msm_atomic_commit_tail+0x1a8/0x68c
> > >    commit_tail+0xb0/0x160
> > >    drm_atomic_helper_commit+0x11c/0x124
> > >    drm_atomic_commit+0xb0/0xdc
> > >    drm_atomic_connector_commit_dpms+0xf4/0x110
> > >    drm_mode_obj_set_property_ioctl+0x16c/0x3b0
> > >    drm_connector_property_set_ioctl+0x4c/0x74
> > >    drm_ioctl_kernel+0xec/0x15c
> > >    drm_ioctl+0x264/0x408
> > >    __arm64_sys_ioctl+0x9c/0xd4
> > >    invoke_syscall+0x4c/0x110
> > >    el0_svc_common+0x94/0xfc
> > >    do_el0_svc+0x3c/0xb0
> > >    el0_svc+0x2c/0x7c
> > >    el0t_64_sync_handler+0x48/0x114
> > >    el0t_64_sync+0x190/0x194
> > > ---[ end trace 0000000000000000 ]---
> > > [drm-dp] dp_ctrl_push_idle: PUSH_IDLE pattern timedout
> > >
> > > Other basic use-cases still seem to work fine hence add a
> > > a module parameter to allow toggling psr enable/disable till
> > > PSR related issues are hashed out with IGT.
> >
> > For the reference: Bjorn reported that he has issues with VT on a
> > PSR-enabled laptops. This patch fixes the issue for him
>
> Module parameters are almost never warranted, and it is definitely not
> the right way to handle a broken implementation.
>
> I've just sent a revert that unconditionally disables PSR support until
> the implementation has been fixed:
>
> 	https://lore.kernel.org/lkml/20230523151646.28366-1-johan+linaro@xxxxxxxxxx/
>
> Johan

I dont completely agree with this. Even the virtual terminal case was 
reported to be fixed by one user but not the other. So it was probably 
something missed out either in validation or reproduction steps of the 
user who reported it to be fixed OR the user who reported it not fixed. 
That needs to be investigated now.

We should have ideally gone with the modparam with the feature patches 
itself knowing that it gets enabled for all sinks if PSR is supported.

I had discussed with Rob that till we have some more confidence with the 
reported issues we would go with the modparam so as to not do the full 
revert.

In this particular case, the one line revert is not really a deal 
breaker. In some other implementations, it might not really be so 
trivial to revert the feature with a one line change.

So I would like to understand what is the concern with the mod param if 
the maintainers are onboard with it.