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

[Johan Hovold]

On Tue, May 23, 2023 at 12:23:04PM -0700, Abhinav Kumar wrote:
> 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:

> >>> ---[ 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/
> 
> 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.

Yes, there may still be some time left to fix it, but it's pretty damn
annoying to find that an issue reported two months ago still is not
fixed at 6.4-rc3. (I even waited to make the switch to 6.4 so that I
would not have to spend time on this.)

I didn't see any mail from Bjorn saying that the series that claimed to
fix the VT issue actually did fix the VT issue. There's only the comment
above from Dmitry suggesting that disabling this feature is the only way
to get a working terminal back.

Regressions happen and sometimes there are corner cases that are harder
to find, but this is a breakage of a fundamental feature that was
reported before the code was even merged into mainline.

> 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.

Modparams are things of the past should not be used to enable broken
features so that some vendor can tick of their internal lists of
features that have been "mainlined".

You can carry that single patch out-of-tree to enable this if you need
it for some particular use case where you don't care about VTs.

But hopefully you can just get this sorted quickly. If not, the revert I
posted is the way to go rather than adding random module parameters.

Johan