[PATCH v2 0/2] drm/msm/dp: fix DP subconnector handling

[PATCH v2 0/2] drm/msm/dp: fix DP subconnector handling

[Dmitry Baryshkov]

Fix two failovers in the DP subconnector's patch. I didn't notice that I
had another patch adding the property in my tree and later Abel pointed
out that we shouldn't use subconnector type for eDP panels.

Fixes since v1:
 - Add Abel's patch.

Abel Vesa (1):
  drm/msm/dp: don't touch DP subconnector property in eDP case

Dmitry Baryshkov (1):
  drm/msm/dp: attach the DP subconnector property

 drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++++-----
 drivers/gpu/drm/msm/dp/dp_drm.c     |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] drm/msm/dp: don't touch DP subconnector property in eDP case

[Dmitry Baryshkov]

From: Abel Vesa <abel.vesa@linaro.org>

In case of the eDP connection there is no subconnetor and as such no
subconnector property. Put drm_dp_set_subconnector_property() calls
under the !is_edp condition.

Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e329e03e068d..1b88fb52726f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -365,9 +365,11 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 	/* reset video pattern flag on disconnect */
 	if (!hpd) {
 		dp->panel->video_test = false;
-		drm_dp_set_subconnector_property(dp->dp_display.connector,
-						 connector_status_disconnected,
-						 dp->panel->dpcd, dp->panel->downstream_ports);
+		if (!dp->dp_display.is_edp)
+			drm_dp_set_subconnector_property(dp->dp_display.connector,
+							 connector_status_disconnected,
+							 dp->panel->dpcd,
+							 dp->panel->downstream_ports);
 	}
 
 	dp->dp_display.is_connected = hpd;
@@ -396,8 +398,11 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
 
 	dp_link_process_request(dp->link);
 
-	drm_dp_set_subconnector_property(dp->dp_display.connector, connector_status_connected,
-					 dp->panel->dpcd, dp->panel->downstream_ports);
+	if (!dp->dp_display.is_edp)
+		drm_dp_set_subconnector_property(dp->dp_display.connector,
+						 connector_status_connected,
+						 dp->panel->dpcd,
+						 dp->panel->downstream_ports);
 
 	edid = dp->panel->edid;
 
-- 
2.42.0

Re: [PATCH v2 1/2] drm/msm/dp: don't touch DP subconnector property in eDP case

[Abhinav Kumar]

On 10/25/2023 2:23 AM, Dmitry Baryshkov wrote:
> From: Abel Vesa <abel.vesa@linaro.org>
> 
> In case of the eDP connection there is no subconnetor and as such no
> subconnector property. Put drm_dp_set_subconnector_property() calls
> under the !is_edp condition.
> 
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 

We still need to unify the calls to drm_dp_set_subconnector_property() 
for the hpd connect/disconnect places preferably in 
dp_display_send_hpd_notification().

That way, we would have had to make this change only in one location.

If you want to pursue that as a separate patch, I am fine as well.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH v2 1/2] drm/msm/dp: don't touch DP subconnector property in eDP case

[Dmitry Baryshkov]

On Sat, 28 Oct 2023 at 00:02, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 10/25/2023 2:23 AM, Dmitry Baryshkov wrote:
> > From: Abel Vesa <abel.vesa@linaro.org>
> >
> > In case of the eDP connection there is no subconnetor and as such no
> > subconnector property. Put drm_dp_set_subconnector_property() calls
> > under the !is_edp condition.
> >
> > Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
>
> We still need to unify the calls to drm_dp_set_subconnector_property()
> for the hpd connect/disconnect places preferably in
> dp_display_send_hpd_notification().
>
> That way, we would have had to make this change only in one location.

Good point, I'd like to take another look at the HPD handling in the
DP driver after we land the pending pm_runtime changes. As a part of
that I'll check the drm_dp_set_subconnector_property() calls.

> If you want to pursue that as a separate patch, I am fine as well.
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/msm/dp: don't touch DP subconnector property in eDP case

[Jessica Zhang]

On 10/25/2023 2:23 AM, Dmitry Baryshkov wrote:
> From: Abel Vesa <abel.vesa@linaro.org>
> 
> In case of the eDP connection there is no subconnetor and as such no
> subconnector property. Put drm_dp_set_subconnector_property() calls
> under the !is_edp condition.
> 
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Tested-by: Jessica Zhang <quic_jesszhan@quicinc.com> # SC7280

> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e329e03e068d..1b88fb52726f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -365,9 +365,11 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
>   	/* reset video pattern flag on disconnect */
>   	if (!hpd) {
>   		dp->panel->video_test = false;
> -		drm_dp_set_subconnector_property(dp->dp_display.connector,
> -						 connector_status_disconnected,
> -						 dp->panel->dpcd, dp->panel->downstream_ports);
> +		if (!dp->dp_display.is_edp)
> +			drm_dp_set_subconnector_property(dp->dp_display.connector,
> +							 connector_status_disconnected,
> +							 dp->panel->dpcd,
> +							 dp->panel->downstream_ports);
>   	}
>   
>   	dp->dp_display.is_connected = hpd;
> @@ -396,8 +398,11 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>   
>   	dp_link_process_request(dp->link);
>   
> -	drm_dp_set_subconnector_property(dp->dp_display.connector, connector_status_connected,
> -					 dp->panel->dpcd, dp->panel->downstream_ports);
> +	if (!dp->dp_display.is_edp)
> +		drm_dp_set_subconnector_property(dp->dp_display.connector,
> +						 connector_status_connected,
> +						 dp->panel->dpcd,
> +						 dp->panel->downstream_ports);
>   
>   	edid = dp->panel->edid;
>   
> -- 
> 2.42.0
> 

Re: [PATCH v2 1/2] drm/msm/dp: don't touch DP subconnector property in eDP case

[Johan Hovold]

On Wed, Oct 25, 2023 at 12:23:09PM +0300, Dmitry Baryshkov wrote:
> From: Abel Vesa <abel.vesa@linaro.org>
> 
> In case of the eDP connection there is no subconnetor and as such no
> subconnector property. Put drm_dp_set_subconnector_property() calls
> under the !is_edp condition.
> 
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

[PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Dmitry Baryshkov]

While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
support setting the DP subconnector type") I had the patch [1] in my
tree. I haven't noticed that it was a dependency for the commit in
question. Mea culpa.

Since the patch has not landed yet (and even was not reviewed)
and since one of the bridges erroneously uses USB connector type instead
of DP, attach the property directly from the MSM DP driver.

This fixes the following oops on DP HPD event:

 drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
 dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
 dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
 hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
 kthread (kernel/kthread.c:388)
 ret_from_fork (arch/arm64/kernel/entry.S:858)

[1] https://patchwork.freedesktop.org/patch/555530/

Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 40e7344180e3..e3bdd7dd4cdc 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -345,6 +345,9 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
 	if (IS_ERR(connector))
 		return connector;
 
+	if (!dp_display->is_edp)
+		drm_connector_attach_dp_subconnector_property(connector);
+
 	drm_connector_attach_encoder(connector, encoder);
 
 	return connector;
-- 
2.42.0

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Jessica Zhang]

On 10/25/2023 2:23 AM, Dmitry Baryshkov wrote:
> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
> support setting the DP subconnector type") I had the patch [1] in my
> tree. I haven't noticed that it was a dependency for the commit in
> question. Mea culpa.
> 
> Since the patch has not landed yet (and even was not reviewed)
> and since one of the bridges erroneously uses USB connector type instead
> of DP, attach the property directly from the MSM DP driver.
> 
> This fixes the following oops on DP HPD event:
> 
>   drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>   dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>   dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>   hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>   kthread (kernel/kthread.c:388)
>   ret_from_fork (arch/arm64/kernel/entry.S:858)
> 
> [1] https://patchwork.freedesktop.org/patch/555530/

Tested-by: Jessica Zhang <quic_jesszhan@quicinc.com> # SC7280

> 
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 40e7344180e3..e3bdd7dd4cdc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -345,6 +345,9 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
>   	if (IS_ERR(connector))
>   		return connector;
>   
> +	if (!dp_display->is_edp)
> +		drm_connector_attach_dp_subconnector_property(connector);
> +
>   	drm_connector_attach_encoder(connector, encoder);
>   
>   	return connector;
> -- 
> 2.42.0
> 

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Johan Hovold]

On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
> support setting the DP subconnector type") I had the patch [1] in my
> tree. I haven't noticed that it was a dependency for the commit in
> question. Mea culpa.

This also broke boot on the Lenovo ThinkPad X13s.

Would be nice to get this fixed ASAP so that further people don't have
to debug this known regression.
 
> Since the patch has not landed yet (and even was not reviewed)
> and since one of the bridges erroneously uses USB connector type instead
> of DP, attach the property directly from the MSM DP driver.
> 
> This fixes the following oops on DP HPD event:
> 
>  drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>  dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>  dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>  hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>  kthread (kernel/kthread.c:388)
>  ret_from_fork (arch/arm64/kernel/entry.S:858)

This only says where the oops happened, it doesn't necessarily in itself
indicate an oops at all or that in this case it's a NULL pointer
dereference.

On the X13s I'm seeing the NULL deref in a different path during boot,
and when this happens after a deferred probe (due to the panel lookup
mess) it hangs the machine, which makes it a bit of a pain to debug:

   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000060
   ...
   CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
   Hardware name: Qualcomm QRD, BIOS 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
   ...
   Call trace:
    drm_object_property_set_value+0x0/0x88 [drm]
    dp_display_process_hpd_high+0xa0/0x14c [msm]
    dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
    dp_bridge_atomic_enable+0x184/0x21c [msm]
    edp_bridge_atomic_enable+0x60/0x94 [msm]
    drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
    drm_atomic_helper_commit_modeset_enables+0x194/0x26c [drm_kms_helper]
    msm_atomic_commit_tail+0x204/0x804 [msm]
    commit_tail+0xa4/0x18c [drm_kms_helper]
    drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
    drm_atomic_commit+0xa4/0x104 [drm]
    drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
    drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
    drm_client_modeset_commit+0x30/0x58 [drm]
    __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc [drm_kms_helper]
    drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
    fbcon_init+0x224/0x49c
    visual_init+0xb0/0x108
    do_bind_con_driver.isra.0+0x19c/0x38c
    do_take_over_console+0x140/0x1ec
    do_fbcon_takeover+0x6c/0xe4
    fbcon_fb_registered+0x180/0x1f0
    register_framebuffer+0x19c/0x228
    __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8 [drm_kms_helper]
    drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
    msm_fbdev_client_hotplug+0x84/0xcc [msm]
    drm_client_register+0x5c/0xa0 [drm]
    msm_fbdev_setup+0x94/0x148 [msm]
    msm_drm_bind+0x3d0/0x42c [msm]
    try_to_bring_up_aggregate_device+0x1ec/0x2f4
    __component_add+0xa8/0x194
    component_add+0x14/0x20
    dp_display_probe+0x278/0x41c [msm]

> [1] https://patchwork.freedesktop.org/patch/555530/
> 
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Abhinav Kumar]

On 11/15/2023 12:06 AM, Johan Hovold wrote:
> On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
>> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
>> support setting the DP subconnector type") I had the patch [1] in my
>> tree. I haven't noticed that it was a dependency for the commit in
>> question. Mea culpa.
> 
> This also broke boot on the Lenovo ThinkPad X13s.
> 
> Would be nice to get this fixed ASAP so that further people don't have
> to debug this known regression.
>   

I will queue this patch for -fixes rightaway.

>> Since the patch has not landed yet (and even was not reviewed)
>> and since one of the bridges erroneously uses USB connector type instead
>> of DP, attach the property directly from the MSM DP driver.
>>
>> This fixes the following oops on DP HPD event:
>>
>>   drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>>   dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>>   dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>>   hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>>   kthread (kernel/kthread.c:388)
>>   ret_from_fork (arch/arm64/kernel/entry.S:858)
> 
> This only says where the oops happened, it doesn't necessarily in itself
> indicate an oops at all or that in this case it's a NULL pointer
> dereference.
> 
> On the X13s I'm seeing the NULL deref in a different path during boot,
> and when this happens after a deferred probe (due to the panel lookup
> mess) it hangs the machine, which makes it a bit of a pain to debug:
> 
>     Unable to handle kernel NULL pointer dereference at virtual address 0000000000000060
>     ...
>     CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
>     Hardware name: Qualcomm QRD, BIOS 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
>     ...
>     Call trace:
>      drm_object_property_set_value+0x0/0x88 [drm]
>      dp_display_process_hpd_high+0xa0/0x14c [msm]
>      dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
>      dp_bridge_atomic_enable+0x184/0x21c [msm]
>      edp_bridge_atomic_enable+0x60/0x94 [msm]
>      drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
>      drm_atomic_helper_commit_modeset_enables+0x194/0x26c [drm_kms_helper]
>      msm_atomic_commit_tail+0x204/0x804 [msm]
>      commit_tail+0xa4/0x18c [drm_kms_helper]
>      drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
>      drm_atomic_commit+0xa4/0x104 [drm]
>      drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
>      drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
>      drm_client_modeset_commit+0x30/0x58 [drm]
>      __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc [drm_kms_helper]
>      drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
>      fbcon_init+0x224/0x49c
>      visual_init+0xb0/0x108
>      do_bind_con_driver.isra.0+0x19c/0x38c
>      do_take_over_console+0x140/0x1ec
>      do_fbcon_takeover+0x6c/0xe4
>      fbcon_fb_registered+0x180/0x1f0
>      register_framebuffer+0x19c/0x228
>      __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8 [drm_kms_helper]
>      drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
>      msm_fbdev_client_hotplug+0x84/0xcc [msm]
>      drm_client_register+0x5c/0xa0 [drm]
>      msm_fbdev_setup+0x94/0x148 [msm]
>      msm_drm_bind+0x3d0/0x42c [msm]
>      try_to_bring_up_aggregate_device+0x1ec/0x2f4
>      __component_add+0xa8/0x194
>      component_add+0x14/0x20
>      dp_display_probe+0x278/0x41c [msm]
> 
>> [1] https://patchwork.freedesktop.org/patch/555530/
>>
>> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector type")
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> Tested-by: Johan Hovold <johan+linaro@kernel.org>
> 

Thanks !

> Johan

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Linux regression tracking (Thorsten Leemhuis)]

On 15.11.23 19:06, Abhinav Kumar wrote:
> On 11/15/2023 12:06 AM, Johan Hovold wrote:
>> On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
>>> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
>>> support setting the DP subconnector type") I had the patch [1] in my
>>> tree. I haven't noticed that it was a dependency for the commit in
>>> question. Mea culpa.
>>
>> This also broke boot on the Lenovo ThinkPad X13s.
>>
>> Would be nice to get this fixed ASAP so that further people don't have
>> to debug this known regression.
> 
> I will queue this patch for -fixes rightaway.

Thx. I noticed that this fix is still not in -next. I then investigated
and I found it was applied on Thursday last week here:
https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads

Makes me wonder: when will that patch go to a branch that is included in
-next? And when will it move on towards mainline?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

>>> Since the patch has not landed yet (and even was not reviewed)
>>> and since one of the bridges erroneously uses USB connector type instead
>>> of DP, attach the property directly from the MSM DP driver.
>>>
>>> This fixes the following oops on DP HPD event:
>>>
>>>   drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>>>   dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>>>   dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>>>   hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>>>   kthread (kernel/kthread.c:388)
>>>   ret_from_fork (arch/arm64/kernel/entry.S:858)
>>
>> This only says where the oops happened, it doesn't necessarily in itself
>> indicate an oops at all or that in this case it's a NULL pointer
>> dereference.
>>
>> On the X13s I'm seeing the NULL deref in a different path during boot,
>> and when this happens after a deferred probe (due to the panel lookup
>> mess) it hangs the machine, which makes it a bit of a pain to debug:
>>
>>     Unable to handle kernel NULL pointer dereference at virtual
>> address 0000000000000060
>>     ...
>>     CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
>>     Hardware name: Qualcomm QRD, BIOS
>> 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
>>     ...
>>     Call trace:
>>      drm_object_property_set_value+0x0/0x88 [drm]
>>      dp_display_process_hpd_high+0xa0/0x14c [msm]
>>      dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
>>      dp_bridge_atomic_enable+0x184/0x21c [msm]
>>      edp_bridge_atomic_enable+0x60/0x94 [msm]
>>      drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
>>      drm_atomic_helper_commit_modeset_enables+0x194/0x26c
>> [drm_kms_helper]
>>      msm_atomic_commit_tail+0x204/0x804 [msm]
>>      commit_tail+0xa4/0x18c [drm_kms_helper]
>>      drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
>>      drm_atomic_commit+0xa4/0x104 [drm]
>>      drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
>>      drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
>>      drm_client_modeset_commit+0x30/0x58 [drm]
>>      __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
>> [drm_kms_helper]
>>      drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
>>      fbcon_init+0x224/0x49c
>>      visual_init+0xb0/0x108
>>      do_bind_con_driver.isra.0+0x19c/0x38c
>>      do_take_over_console+0x140/0x1ec
>>      do_fbcon_takeover+0x6c/0xe4
>>      fbcon_fb_registered+0x180/0x1f0
>>      register_framebuffer+0x19c/0x228
>>      __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
>> [drm_kms_helper]
>>      drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
>>      msm_fbdev_client_hotplug+0x84/0xcc [msm]
>>      drm_client_register+0x5c/0xa0 [drm]
>>      msm_fbdev_setup+0x94/0x148 [msm]
>>      msm_drm_bind+0x3d0/0x42c [msm]
>>      try_to_bring_up_aggregate_device+0x1ec/0x2f4
>>      __component_add+0xa8/0x194
>>      component_add+0x14/0x20
>>      dp_display_probe+0x278/0x41c [msm]
>>
>>> [1] https://patchwork.freedesktop.org/patch/555530/
>>>
>>> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
>>> type")
>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>
> 
> Thanks !
> 
>> Johan

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Abhinav Kumar]

On 11/21/2023 9:57 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 15.11.23 19:06, Abhinav Kumar wrote:
>> On 11/15/2023 12:06 AM, Johan Hovold wrote:
>>> On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
>>>> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
>>>> support setting the DP subconnector type") I had the patch [1] in my
>>>> tree. I haven't noticed that it was a dependency for the commit in
>>>> question. Mea culpa.
>>>
>>> This also broke boot on the Lenovo ThinkPad X13s.
>>>
>>> Would be nice to get this fixed ASAP so that further people don't have
>>> to debug this known regression.
>>
>> I will queue this patch for -fixes rightaway.
> 
> Thx. I noticed that this fix is still not in -next. I then investigated
> and I found it was applied on Thursday last week here:
> https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads
> 
> Makes me wonder: when will that patch go to a branch that is included in
> -next? And when will it move on towards mainline?
> 

This has been included in a pull request for 6.7-rc3 to the DRM tree and 
shall make it to -next from there.

> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
>>>> Since the patch has not landed yet (and even was not reviewed)
>>>> and since one of the bridges erroneously uses USB connector type instead
>>>> of DP, attach the property directly from the MSM DP driver.
>>>>
>>>> This fixes the following oops on DP HPD event:
>>>>
>>>>    drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>>>>    dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>>>>    dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>>>>    hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>>>>    kthread (kernel/kthread.c:388)
>>>>    ret_from_fork (arch/arm64/kernel/entry.S:858)
>>>
>>> This only says where the oops happened, it doesn't necessarily in itself
>>> indicate an oops at all or that in this case it's a NULL pointer
>>> dereference.
>>>
>>> On the X13s I'm seeing the NULL deref in a different path during boot,
>>> and when this happens after a deferred probe (due to the panel lookup
>>> mess) it hangs the machine, which makes it a bit of a pain to debug:
>>>
>>>      Unable to handle kernel NULL pointer dereference at virtual
>>> address 0000000000000060
>>>      ...
>>>      CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
>>>      Hardware name: Qualcomm QRD, BIOS
>>> 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
>>>      ...
>>>      Call trace:
>>>       drm_object_property_set_value+0x0/0x88 [drm]
>>>       dp_display_process_hpd_high+0xa0/0x14c [msm]
>>>       dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
>>>       dp_bridge_atomic_enable+0x184/0x21c [msm]
>>>       edp_bridge_atomic_enable+0x60/0x94 [msm]
>>>       drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
>>>       drm_atomic_helper_commit_modeset_enables+0x194/0x26c
>>> [drm_kms_helper]
>>>       msm_atomic_commit_tail+0x204/0x804 [msm]
>>>       commit_tail+0xa4/0x18c [drm_kms_helper]
>>>       drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
>>>       drm_atomic_commit+0xa4/0x104 [drm]
>>>       drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
>>>       drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
>>>       drm_client_modeset_commit+0x30/0x58 [drm]
>>>       __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
>>> [drm_kms_helper]
>>>       drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
>>>       fbcon_init+0x224/0x49c
>>>       visual_init+0xb0/0x108
>>>       do_bind_con_driver.isra.0+0x19c/0x38c
>>>       do_take_over_console+0x140/0x1ec
>>>       do_fbcon_takeover+0x6c/0xe4
>>>       fbcon_fb_registered+0x180/0x1f0
>>>       register_framebuffer+0x19c/0x228
>>>       __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
>>> [drm_kms_helper]
>>>       drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
>>>       msm_fbdev_client_hotplug+0x84/0xcc [msm]
>>>       drm_client_register+0x5c/0xa0 [drm]
>>>       msm_fbdev_setup+0x94/0x148 [msm]
>>>       msm_drm_bind+0x3d0/0x42c [msm]
>>>       try_to_bring_up_aggregate_device+0x1ec/0x2f4
>>>       __component_add+0xa8/0x194
>>>       component_add+0x14/0x20
>>>       dp_display_probe+0x278/0x41c [msm]
>>>
>>>> [1] https://patchwork.freedesktop.org/patch/555530/
>>>>
>>>> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
>>>> type")
>>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>>
>>
>> Thanks !
>>
>>> Johan

Re: [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property

[Linux regression tracking (Thorsten Leemhuis)]

On 21.11.23 19:50, Abhinav Kumar wrote:
> On 11/21/2023 9:57 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 15.11.23 19:06, Abhinav Kumar wrote:
>>> On 11/15/2023 12:06 AM, Johan Hovold wrote:
>>>> On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
>>>>> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
>>>>> support setting the DP subconnector type") I had the patch [1] in my
>>>>> tree. I haven't noticed that it was a dependency for the commit in
>>>>> question. Mea culpa.
>>>>
>>>> This also broke boot on the Lenovo ThinkPad X13s.
>>>>
>>>> Would be nice to get this fixed ASAP so that further people don't have
>>>> to debug this known regression.
>>>
>>> I will queue this patch for -fixes rightaway.
>>
>> Thx. I noticed that this fix is still not in -next. I then investigated
>> and I found it was applied on Thursday last week here:
>> https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads
>>
>> Makes me wonder: when will that patch go to a branch that is included in
>> -next? And when will it move on towards mainline?
> 
> This has been included in a pull request for 6.7-rc3 to the DRM tree and
> shall make it to -next from there.

Ahh, great, thx, I was slowly getting worried.

Ciao, Thorsten

>>>>> Since the patch has not landed yet (and even was not reviewed)
>>>>> and since one of the bridges erroneously uses USB connector type
>>>>> instead
>>>>> of DP, attach the property directly from the MSM DP driver.
>>>>>
>>>>> This fixes the following oops on DP HPD event:
>>>>>
>>>>>    drm_object_property_set_value
>>>>> (drivers/gpu/drm/drm_mode_object.c:288)
>>>>>    dp_display_process_hpd_high
>>>>> (drivers/gpu/drm/msm/dp/dp_display.c:402)
>>>>>    dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>>>>>    hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>>>>>    kthread (kernel/kthread.c:388)
>>>>>    ret_from_fork (arch/arm64/kernel/entry.S:858)
>>>>
>>>> This only says where the oops happened, it doesn't necessarily in
>>>> itself
>>>> indicate an oops at all or that in this case it's a NULL pointer
>>>> dereference.
>>>>
>>>> On the X13s I'm seeing the NULL deref in a different path during boot,
>>>> and when this happens after a deferred probe (due to the panel lookup
>>>> mess) it hangs the machine, which makes it a bit of a pain to debug:
>>>>
>>>>      Unable to handle kernel NULL pointer dereference at virtual
>>>> address 0000000000000060
>>>>      ...
>>>>      CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
>>>>      Hardware name: Qualcomm QRD, BIOS
>>>> 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
>>>>      ...
>>>>      Call trace:
>>>>       drm_object_property_set_value+0x0/0x88 [drm]
>>>>       dp_display_process_hpd_high+0xa0/0x14c [msm]
>>>>       dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
>>>>       dp_bridge_atomic_enable+0x184/0x21c [msm]
>>>>       edp_bridge_atomic_enable+0x60/0x94 [msm]
>>>>       drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
>>>>       drm_atomic_helper_commit_modeset_enables+0x194/0x26c
>>>> [drm_kms_helper]
>>>>       msm_atomic_commit_tail+0x204/0x804 [msm]
>>>>       commit_tail+0xa4/0x18c [drm_kms_helper]
>>>>       drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
>>>>       drm_atomic_commit+0xa4/0x104 [drm]
>>>>       drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
>>>>       drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
>>>>       drm_client_modeset_commit+0x30/0x58 [drm]
>>>>       __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
>>>> [drm_kms_helper]
>>>>       drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
>>>>       fbcon_init+0x224/0x49c
>>>>       visual_init+0xb0/0x108
>>>>       do_bind_con_driver.isra.0+0x19c/0x38c
>>>>       do_take_over_console+0x140/0x1ec
>>>>       do_fbcon_takeover+0x6c/0xe4
>>>>       fbcon_fb_registered+0x180/0x1f0
>>>>       register_framebuffer+0x19c/0x228
>>>>       __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
>>>> [drm_kms_helper]
>>>>       drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
>>>>       msm_fbdev_client_hotplug+0x84/0xcc [msm]
>>>>       drm_client_register+0x5c/0xa0 [drm]
>>>>       msm_fbdev_setup+0x94/0x148 [msm]
>>>>       msm_drm_bind+0x3d0/0x42c [msm]
>>>>       try_to_bring_up_aggregate_device+0x1ec/0x2f4
>>>>       __component_add+0xa8/0x194
>>>>       component_add+0x14/0x20
>>>>       dp_display_probe+0x278/0x41c [msm]
>>>>
>>>>> [1] https://patchwork.freedesktop.org/patch/555530/
>>>>>
>>>>> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
>>>>> type")
>>>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>
>>>> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
>>>> Tested-by: Johan Hovold <johan+linaro@kernel.org>
>>>>
>>>
>>> Thanks !
>>>
>>>> Johan
> 
> 

Re: [PATCH v2 0/2] drm/msm/dp: fix DP subconnector handling

[Abhinav Kumar]

On Wed, 25 Oct 2023 12:23:08 +0300, Dmitry Baryshkov wrote:
> Fix two failovers in the DP subconnector's patch. I didn't notice that I
> had another patch adding the property in my tree and later Abel pointed
> out that we shouldn't use subconnector type for eDP panels.
> 
> Fixes since v1:
>  - Add Abel's patch.
> 
> [...]

Applied, thanks!

[1/2] drm/msm/dp: don't touch DP subconnector property in eDP case
      https://gitlab.freedesktop.org/drm/msm/-/commit/ebfa85c504cb
[2/2] drm/msm/dp: attach the DP subconnector property
      https://gitlab.freedesktop.org/drm/msm/-/commit/21133266ca12

Best regards,
-- 
Abhinav Kumar <quic_abhinavk@quicinc.com>