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

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

[Abhinav Kumar]

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.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 628b0e248db6..dba43167de66 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -28,6 +28,10 @@
 #include "dp_audio.h"
 #include "dp_debug.h"
 
+static bool psr_enabled = false;
+module_param(psr_enabled, bool, 0);
+MODULE_PARM_DESC(psr_enabled, "enable PSR for eDP and DP displays");
+
 #define HPD_STRING_SIZE 30
 
 enum {
@@ -407,7 +411,7 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
 
 	edid = dp->panel->edid;
 
-	dp->dp_display.psr_supported = dp->panel->psr_cap.version;
+	dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
 
 	dp->audio_supported = drm_detect_monitor_audio(edid);
 	dp_panel_handle_sink_request(dp->panel);
-- 
2.40.1

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

[Abhinav Kumar]

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.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 628b0e248db6..dba43167de66 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -28,6 +28,10 @@
 #include "dp_audio.h"
 #include "dp_debug.h"
 
+static bool psr_enabled = false;
+module_param(psr_enabled, bool, 0);
+MODULE_PARM_DESC(psr_enabled, "enable PSR for eDP and DP displays");
+
 #define HPD_STRING_SIZE 30
 
 enum {
@@ -407,7 +411,7 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
 
 	edid = dp->panel->edid;
 
-	dp->dp_display.psr_supported = dp->panel->psr_cap.version;
+	dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
 
 	dp->audio_supported = drm_detect_monitor_audio(edid);
 	dp_panel_handle_sink_request(dp->panel);
-- 
2.40.1

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

[Dmitry Baryshkov]

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

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 628b0e248db6..dba43167de66 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -28,6 +28,10 @@
>   #include "dp_audio.h"
>   #include "dp_debug.h"
>   
> +static bool psr_enabled = false;
> +module_param(psr_enabled, bool, 0);
> +MODULE_PARM_DESC(psr_enabled, "enable PSR for eDP and DP displays");
> +
>   #define HPD_STRING_SIZE 30
>   
>   enum {
> @@ -407,7 +411,7 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>   
>   	edid = dp->panel->edid;
>   
> -	dp->dp_display.psr_supported = dp->panel->psr_cap.version;
> +	dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
>   
>   	dp->audio_supported = drm_detect_monitor_audio(edid);
>   	dp_panel_handle_sink_request(dp->panel);

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 628b0e248db6..dba43167de66 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -28,6 +28,10 @@
>   #include "dp_audio.h"
>   #include "dp_debug.h"
>   
> +static bool psr_enabled = false;
> +module_param(psr_enabled, bool, 0);
> +MODULE_PARM_DESC(psr_enabled, "enable PSR for eDP and DP displays");
> +
>   #define HPD_STRING_SIZE 30
>   
>   enum {
> @@ -407,7 +411,7 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>   
>   	edid = dp->panel->edid;
>   
> -	dp->dp_display.psr_supported = dp->panel->psr_cap.version;
> +	dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
>   
>   	dp->audio_supported = drm_detect_monitor_audio(edid);
>   	dp_panel_handle_sink_request(dp->panel);

-- 
With best wishes
Dmitry

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

[Johan Hovold]

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@kernel.org/

Johan

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

[Johan Hovold]

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@kernel.org/

Johan

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@kernel.org/
> 
> 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.

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@kernel.org/
> 
> 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.

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

[Rob Clark]

On Tue, May 23, 2023 at 12:23 PM Abhinav Kumar
<quic_abhinavk@quicinc.com> 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:
> >>>    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@kernel.org/
> >
> > 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.

Tbf, I'd go further in the modparam direction, ie. add a default
disabled modparam, but then _also_ enable the modparam in CI and add
the failing tests to xfails.  I'd rather have xfails in CI than skips.

Acked-by: Rob Clark <robdclark@gmail.com>

BR,
-R

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

[Rob Clark]

On Tue, May 23, 2023 at 12:23 PM Abhinav Kumar
<quic_abhinavk@quicinc.com> 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:
> >>>    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@kernel.org/
> >
> > 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.

Tbf, I'd go further in the modparam direction, ie. add a default
disabled modparam, but then _also_ enable the modparam in CI and add
the failing tests to xfails.  I'd rather have xfails in CI than skips.

Acked-by: Rob Clark <robdclark@gmail.com>

BR,
-R

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@kernel.org/
> 
> 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

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@kernel.org/
> 
> 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

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

[Dmitry Baryshkov]

On 24/05/2023 09:59, Johan Hovold wrote:
> 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@kernel.org/
>>
>> 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.

Originally this issue was reported by Doug, and at [1] he reported that 
an issue is fixed for him. So, for me it looks like we have hardware 
where VT works and hardware where it doesn't.

Doug, can you please confirm whether you can reproduce the PSR+VT issue 
on 6.4-rc (without extra patches) or if the issue is fixed for you?

[1] 
https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/

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

We have had a history of using modparam with i915 and IIRC amdgpu / 
radeon drivers to allow users to easily check whether new feature works 
for their hardware. My current understanding is that PSR+VT works for on 
some laptops and doesn't on some other laptops, which makes it a valid case.

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 24/05/2023 09:59, Johan Hovold wrote:
> 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@kernel.org/
>>
>> 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.

Originally this issue was reported by Doug, and at [1] he reported that 
an issue is fixed for him. So, for me it looks like we have hardware 
where VT works and hardware where it doesn't.

Doug, can you please confirm whether you can reproduce the PSR+VT issue 
on 6.4-rc (without extra patches) or if the issue is fixed for you?

[1] 
https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/

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

We have had a history of using modparam with i915 and IIRC amdgpu / 
radeon drivers to allow users to easily check whether new feature works 
for their hardware. My current understanding is that PSR+VT works for on 
some laptops and doesn't on some other laptops, which makes it a valid case.

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

-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Wed, May 24, 2023 at 11:06:03AM +0300, Dmitry Baryshkov wrote:
> On 24/05/2023 09:59, Johan Hovold wrote:

> > 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".
> 
> We have had a history of using modparam with i915 and IIRC amdgpu / 
> radeon drivers to allow users to easily check whether new feature works 
> for their hardware. My current understanding is that PSR+VT works for on 
> some laptops and doesn't on some other laptops, which makes it a valid case.

But here it does not seem to be the hardware that's the issue, but
rather that the implementation is incorrect or incomplete.

Johan

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

[Johan Hovold]

On Wed, May 24, 2023 at 11:06:03AM +0300, Dmitry Baryshkov wrote:
> On 24/05/2023 09:59, Johan Hovold wrote:

> > 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".
> 
> We have had a history of using modparam with i915 and IIRC amdgpu / 
> radeon drivers to allow users to easily check whether new feature works 
> for their hardware. My current understanding is that PSR+VT works for on 
> some laptops and doesn't on some other laptops, which makes it a valid case.

But here it does not seem to be the hardware that's the issue, but
rather that the implementation is incorrect or incomplete.

Johan

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

[Doug Anderson]

Hi,

On Wed, May 24, 2023 at 1:06 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 24/05/2023 09:59, Johan Hovold wrote:
> > 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@kernel.org/
> >>
> >> 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.
>
> Originally this issue was reported by Doug, and at [1] he reported that
> an issue is fixed for him. So, for me it looks like we have hardware
> where VT works and hardware where it doesn't.

As I understand it, the problem was originally reported by Bjorn over
IRC. When he reported the problem on VT2, Stephen Boyd confirmed that
he could see the same problem on our hardware when using VT2. I
confirmed I could reproduce and also tested the fix. I don't remember
if Bjorn ever tested the fix. I think many of us assumed that it was
the same issue so a fix for one person would also fix the other.


> Doug, can you please confirm whether you can reproduce the PSR+VT issue
> on 6.4-rc (without extra patches) or if the issue is fixed for you?
>
> [1]
> https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/

Ugh. Unfortunately, it's not easy for me to test this particular
feature directly on upstream Linux. :( I typically run with a ChromeOS
root filesystem, which works pretty well with mainline. One place
where it doesn't work with mainline is VT2. The VT2 feature for
Chromebooks is implemented with "frecon" and takes advantage of a
downstream patch that we've carried in the ChromeOS kernel trees for
years allowing handoff of who the DRM "master" is.

For testing the fix previously, I confirmed that I could reproduce the
problem on our downstream kernel (which had the PSR patches picked)
and that the fixes worked for me in that context.

Ah, but it shouldn't be too hard to pick that one patch. Let's see if
that works. I'll pick ("CHROMIUM: drm: Add drm_master_relax debugfs
file (non-root set/drop master ioctls)"). Indeed, it works!

OK, so with the top of Linus's tree (v6.4-rc3-17-g9d646009f65d) plus
the CHROMIUM patch to enable my VT2, I can confirm that I don't see
the issue. I can switch to VT2 and typing works fine. I will say that
on herobrine the backlight is all messed up, but I assume that's an
unrelated issue.

-Doug



> > 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".
>
> We have had a history of using modparam with i915 and IIRC amdgpu /
> radeon drivers to allow users to easily check whether new feature works
> for their hardware. My current understanding is that PSR+VT works for on
> some laptops and doesn't on some other laptops, which makes it a valid case.
>
> >
> > 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
>
> --
> With best wishes
> Dmitry
>

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

[Doug Anderson]

Hi,

On Wed, May 24, 2023 at 1:06 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 24/05/2023 09:59, Johan Hovold wrote:
> > 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@kernel.org/
> >>
> >> 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.
>
> Originally this issue was reported by Doug, and at [1] he reported that
> an issue is fixed for him. So, for me it looks like we have hardware
> where VT works and hardware where it doesn't.

As I understand it, the problem was originally reported by Bjorn over
IRC. When he reported the problem on VT2, Stephen Boyd confirmed that
he could see the same problem on our hardware when using VT2. I
confirmed I could reproduce and also tested the fix. I don't remember
if Bjorn ever tested the fix. I think many of us assumed that it was
the same issue so a fix for one person would also fix the other.


> Doug, can you please confirm whether you can reproduce the PSR+VT issue
> on 6.4-rc (without extra patches) or if the issue is fixed for you?
>
> [1]
> https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/

Ugh. Unfortunately, it's not easy for me to test this particular
feature directly on upstream Linux. :( I typically run with a ChromeOS
root filesystem, which works pretty well with mainline. One place
where it doesn't work with mainline is VT2. The VT2 feature for
Chromebooks is implemented with "frecon" and takes advantage of a
downstream patch that we've carried in the ChromeOS kernel trees for
years allowing handoff of who the DRM "master" is.

For testing the fix previously, I confirmed that I could reproduce the
problem on our downstream kernel (which had the PSR patches picked)
and that the fixes worked for me in that context.

Ah, but it shouldn't be too hard to pick that one patch. Let's see if
that works. I'll pick ("CHROMIUM: drm: Add drm_master_relax debugfs
file (non-root set/drop master ioctls)"). Indeed, it works!

OK, so with the top of Linus's tree (v6.4-rc3-17-g9d646009f65d) plus
the CHROMIUM patch to enable my VT2, I can confirm that I don't see
the issue. I can switch to VT2 and typing works fine. I will say that
on herobrine the backlight is all messed up, but I assume that's an
unrelated issue.

-Doug



> > 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".
>
> We have had a history of using modparam with i915 and IIRC amdgpu /
> radeon drivers to allow users to easily check whether new feature works
> for their hardware. My current understanding is that PSR+VT works for on
> some laptops and doesn't on some other laptops, which makes it a valid case.
>
> >
> > 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
>
> --
> With best wishes
> Dmitry
>

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

[Johan Hovold]

On Wed, May 24, 2023 at 10:13:33AM -0700, Doug Anderson wrote:
> On Wed, May 24, 2023 at 1:06 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:

> > Originally this issue was reported by Doug, and at [1] he reported that
> > an issue is fixed for him. So, for me it looks like we have hardware
> > where VT works and hardware where it doesn't.
> 
> As I understand it, the problem was originally reported by Bjorn over
> IRC. When he reported the problem on VT2, Stephen Boyd confirmed that
> he could see the same problem on our hardware when using VT2. I
> confirmed I could reproduce and also tested the fix. I don't remember
> if Bjorn ever tested the fix. I think many of us assumed that it was
> the same issue so a fix for one person would also fix the other.

Yeah, that sounds reasonable even if there apparently are some
differences between mainline and what you run in ChromeOS here.

> > Doug, can you please confirm whether you can reproduce the PSR+VT issue
> > on 6.4-rc (without extra patches) or if the issue is fixed for you?
> >
> > [1]
> > https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/
> 
> Ugh. Unfortunately, it's not easy for me to test this particular
> feature directly on upstream Linux. :( I typically run with a ChromeOS
> root filesystem, which works pretty well with mainline. One place
> where it doesn't work with mainline is VT2. The VT2 feature for
> Chromebooks is implemented with "frecon" and takes advantage of a
> downstream patch that we've carried in the ChromeOS kernel trees for
> years allowing handoff of who the DRM "master" is.
> 
> For testing the fix previously, I confirmed that I could reproduce the
> problem on our downstream kernel (which had the PSR patches picked)
> and that the fixes worked for me in that context.
> 
> Ah, but it shouldn't be too hard to pick that one patch. Let's see if
> that works. I'll pick ("CHROMIUM: drm: Add drm_master_relax debugfs
> file (non-root set/drop master ioctls)"). Indeed, it works!
> 
> OK, so with the top of Linus's tree (v6.4-rc3-17-g9d646009f65d) plus
> the CHROMIUM patch to enable my VT2, I can confirm that I don't see
> the issue. I can switch to VT2 and typing works fine. I will say that
> on herobrine the backlight is all messed up, but I assume that's an
> unrelated issue.

Interesting, as VTs are still broken in with rc4 on the X13s (and
sc8280xp CRD).

Does anyone have any ideas of why things break on mainline with these
machines? Any patches or tests I can try?

Dmitry, do you have an X13s now that you can reproduce this on?

Johan

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

[Johan Hovold]

On Wed, May 24, 2023 at 10:13:33AM -0700, Doug Anderson wrote:
> On Wed, May 24, 2023 at 1:06 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:

> > Originally this issue was reported by Doug, and at [1] he reported that
> > an issue is fixed for him. So, for me it looks like we have hardware
> > where VT works and hardware where it doesn't.
> 
> As I understand it, the problem was originally reported by Bjorn over
> IRC. When he reported the problem on VT2, Stephen Boyd confirmed that
> he could see the same problem on our hardware when using VT2. I
> confirmed I could reproduce and also tested the fix. I don't remember
> if Bjorn ever tested the fix. I think many of us assumed that it was
> the same issue so a fix for one person would also fix the other.

Yeah, that sounds reasonable even if there apparently are some
differences between mainline and what you run in ChromeOS here.

> > Doug, can you please confirm whether you can reproduce the PSR+VT issue
> > on 6.4-rc (without extra patches) or if the issue is fixed for you?
> >
> > [1]
> > https://lore.kernel.org/dri-devel/CAD=FV=VSHmQPtsQfWjviEZeErms-VEOTmfozejASUC9zsMjAbA@mail.gmail.com/
> 
> Ugh. Unfortunately, it's not easy for me to test this particular
> feature directly on upstream Linux. :( I typically run with a ChromeOS
> root filesystem, which works pretty well with mainline. One place
> where it doesn't work with mainline is VT2. The VT2 feature for
> Chromebooks is implemented with "frecon" and takes advantage of a
> downstream patch that we've carried in the ChromeOS kernel trees for
> years allowing handoff of who the DRM "master" is.
> 
> For testing the fix previously, I confirmed that I could reproduce the
> problem on our downstream kernel (which had the PSR patches picked)
> and that the fixes worked for me in that context.
> 
> Ah, but it shouldn't be too hard to pick that one patch. Let's see if
> that works. I'll pick ("CHROMIUM: drm: Add drm_master_relax debugfs
> file (non-root set/drop master ioctls)"). Indeed, it works!
> 
> OK, so with the top of Linus's tree (v6.4-rc3-17-g9d646009f65d) plus
> the CHROMIUM patch to enable my VT2, I can confirm that I don't see
> the issue. I can switch to VT2 and typing works fine. I will say that
> on herobrine the backlight is all messed up, but I assume that's an
> unrelated issue.

Interesting, as VTs are still broken in with rc4 on the X13s (and
sc8280xp CRD).

Does anyone have any ideas of why things break on mainline with these
machines? Any patches or tests I can try?

Dmitry, do you have an X13s now that you can reproduce this on?

Johan