check sink_count before update is_connected status
v4 cover check sink_count before update is_connected status
1) check sink_count before update is_connected status
2) initialize audio_comp when audio starts
3) check main link status before start aux read
4) dp_link_parse_sink_count() return immediately if aux read failed
Kuogee Hsieh (4):
drm/msm/dp: check sink_count before update is_connected status drm/msm/dp: initialize audio_comp when audio starts drm/msm/dp: check main link status before start aux read drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read
failed
drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++++++++++------------ drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_link.c | 20 ++++++++++++++----- 5 files changed, 48 insertions(+), 17 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v4 1/4 drm/msm/dp: check sink_count before update is_connected status
Link status is different from display connected status in the case of something like an Apple dongle where the type-c plug can be connected, and therefore the link is connected, but no sink is connected until an HDMI cable is plugged into the dongle. The sink_count of DPCD of dongle will increase to 1 once an HDMI cable is plugged into the dongle so that display connected status will become true. This checking also apply at pm_resume.
Changes in v4:
-- none
--- drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/gpu/drm/msm/dp/dp_display.c +8 -7
@@ -586,10 +586,8 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)mutex_lock (&dp->event_mutex); state = dp->hpd_state; -if (state ==ST_CONNECT_PENDING ) { -dp_display_enable (dp,0 ); +if (state ==ST_CONNECT_PENDING ) dp->hpd_state =ST_CONNECTED ; - }mutex_unlock (&dp->event_mutex);
@@ -669,10 +667,8 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 datamutex_lock (&dp->event_mutex); state = dp->hpd_state; -if (state ==ST_DISCONNECT_PENDING ) { -dp_display_disable (dp,0 ); +if (state ==ST_DISCONNECT_PENDING ) dp->hpd_state =ST_DISCONNECTED ; - }mutex_unlock (&dp->event_mutex);
@@ -1272,7 +1268,12 @@ static int dp_pm_resume(struct device *dev) status =dp_catalog_link_is_connected (dp->catalog); -if (status) +/* +* can not declared display is connected unless +* HDMI cable is plugged in and sink_count of +* dongle become 1 +*/ +if (status && dp->link->sink_count) dp->dp_display.is_connected = true;else dp->dp_display.is_connected = false;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 1/4] drm/msm/dp: check sink_count before update is_connected status
Quoting Kuogee Hsieh (2021-04-21 16:37:35)
Quoted 25 lines
Link status is different from display connected status in the case of something like an Apple dongle where the type-c plug can be connected, and therefore the link is connected, but no sink is connected until an HDMI cable is plugged into the dongle. The
sink_countof DPCD of dongle will increase to 1 once an HDMI cable is plugged into the dongle so that display connected status will become true. This checking also apply atpm_resume.Changes in v4:
-- noneFixes 94e58e2d06e3 ("drm/msm/dp: reset dp controller only at boot up and pm_resume")Reported-by Stephen Boyd <[email protected]>Reviewed-by Stephen Boyd <[email protected]>Tested-by Stephen Boyd <[email protected]>Signed-off-by Kuogee Hsieh <[email protected]>--- drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)drivers/gpu/drm/msm/dp/dp_display.c +0
-0
drivers/gpu/drm/msm/dp/dp_display.c static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data) · line 592
Quoted 8 lines
drivers/gpu/drm/msm/dp/dp_display.c +1
-3@@ -586,10 +586,8 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)mutex_lock (&dp->event_mutex); state = dp->hpd_state; -if (state ==ST_CONNECT_PENDING ) { -dp_display_enable (dp,0 ); +if (state ==ST_CONNECT_PENDING ) dp->hpd_state =ST_CONNECTED ; - }
This part has been there since commit 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets") so we should add that tag here too, like
Snapdragon Chipsets")
It would also be nice if this hunk was explained. It doesn't make sense to me that a timeout would enable the display so maybe that can be called out in the commit text about why we remove the call here.
PATCH v4 2/4 drm/msm/dp: initialize audio_comp when audio starts
In reply to Kuogee HsiehInitialize audio_comp when audio starts and wait for audio_comp at dp_display_disable(). This will take care of both dongle unplugged and display off (suspend) cases.
Changes in v2:
-- add dp_display_signal_audio_start()
Changes in v3:
-- restore dp_display_handle_plugged_change() at dp_hpd_unplug_handle().
Changes in v4:
-- none
--- drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 11 +++++++++-- drivers/gpu/drm/msm/dp/dp_display.h | 1 + 3 files changed, 11 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/dp/dp_audio.c +1 -0
@@ -527,6 +527,7 @@ int dp_audio_hw_params(struct device *dev,dp_audio_setup_acr (audio);dp_audio_safe_to_exit_level (audio);dp_audio_enable (audio, true); +dp_display_signal_audio_start (dp_display); dp_display->audio_enabled = true; end:
drivers/gpu/drm/msm/dp/dp_display.c +9 -2
@@ -178,6 +178,15 @@ static int dp_del_event(struct dp_display_private *dp_priv, u32 event)return 0 ; } +void dp_display_signal_audio_start (struct msm_dp *dp_display) +{ +struct dp_display_private *dp; + + dp = container_of(dp_display, struct dp_display_private, dp_display); + +reinit_completion (&dp->audio_comp); +} +void dp_display_signal_audio_complete (struct msm_dp *dp_display) {struct dp_display_private *dp;
@@ -649,7 +658,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)dp_add_event (dp,EV_DISCONNECT_PENDING_TIMEOUT ,0 ,DP_TIMEOUT_5_SECOND );/* signal the disconnect event early to ensure proper teardown */ -reinit_completion (&dp->audio_comp);dp_display_handle_plugged_change (g_dp_display, false); dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK |
@@ -894,7 +902,6 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)/* wait only if audio was enabled */ if (dp_display->audio_enabled) {/* signal the disconnect event */ -reinit_completion (&dp->audio_comp);dp_display_handle_plugged_change (dp_display, false);if (!wait_for_completion_timeout (&dp->audio_comp,HZ *5 ))
drivers/gpu/drm/msm/dp/dp_display.h +1 -0
@@ -34,6 +34,7 @@ int dp_display_get_modes(struct msm_dp *dp_display,int dp_display_request_irq (struct msm_dp *dp_display);bool dp_display_check_video_test (struct msm_dp *dp_display);int dp_display_get_test_bpp (struct msm_dp *dp_display); +void dp_display_signal_audio_start (struct msm_dp *dp_display);void dp_display_signal_audio_complete (struct msm_dp *dp_display);#endif /* _DP_DISPLAY_H_ */
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v4 2/4] drm/msm/dp: initialize audio_comp when audio starts
Quoting Kuogee Hsieh (2021-04-21 16:37:36)
Quoted 7 lines
Initialize
audio_compwhen audio starts and wait foraudio_compatdp_display_disable(). This will take care of both dongle unplugged and display off (suspend) cases.Changes in v2:
-- adddp_display_signal_audio_start()
Quoted 8 lines
Changes in v3:
-- restoredp_display_handle_plugged_change()atdp_hpd_unplug_handle().Changes in v4:
-- noneSigned-off-by Kuogee Hsieh <[email protected]>---
msm_dp_display_disable")
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v4 3/4 drm/msm/dp: check main link status before start aux read
In reply to Kuogee HsiehMaybe when the cable is disconnected the DP phy should be shutdown and some bit in the phy could effectively "cut off" the aux channel and then NAKs would start coming through here in the DP controller I/O register space. This patch have DP aux channel read/write to return NAK immediately if DP controller connection status is in unplugged state.
Changes in V4:
-- split this patch as stand alone patch
--- drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/gpu/drm/msm/dp/dp_aux.c +5 -0
@@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,mutex_lock (&aux->mutex); +if (!dp_catalog_link_is_connected (aux->catalog)) { + ret = -ETIMEDOUT ; + goto unlock_exit; + } + aux->native = msg->request & (DP_AUX_NATIVE_WRITE &DP_AUX_NATIVE_READ );/* Ignore address only message */
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/4] drm/msm/dp: check main link status before start aux read
Quoting Kuogee Hsieh (2021-04-21 16:37:37)
Quoted 20 lines
Maybe when the cable is disconnected the DP phy should be shutdown and some bit in the phy could effectively "cut off" the aux channel and then NAKs would start coming through here in the DP controller I/O register space. This patch have DP aux channel read/write to return NAK immediately if DP controller connection status is in unplugged state.
Changes in V4:
-- split this patch as stand alone patchSigned-off-by Kuogee Hsieh <[email protected]>--- drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/gpu/drm/msm/dp/dp_aux.c +0
-0@@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
drivers/gpu/drm/msm/dp/dp_aux.c static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, · line 353
Quoted 10 lines
drivers/gpu/drm/msm/dp/dp_aux.c +5
-0mutex_lock (&aux->mutex); +if (!dp_catalog_link_is_connected (aux->catalog)) { + ret = -ETIMEDOUT ; + goto unlock_exit; + } + aux->native = msg->request & (DP_AUX_NATIVE_WRITE &DP_AUX_NATIVE_READ );/* Ignore address only message */
Can the code check for aux timeouts? So instead of blindly completing 'aux->comp' we would do the transfer, and then dp_aux_cmd_fifo_tx() would check to see if the completion was completed from the irq handler because of a timeout or a nack, etc. I think the code is probably racy, given that dp_aux_isr() is called from irq context, and aux_error_num is set from the irq context and tested in non-irq context. This code needs a spinlock and then to check the isr bits to figure out if it should tell the upper layers that the address was wrong, or there was a nack or a timeout, etc.
I don't think we need to check the link to see if it is connected, just look at the irq bits to see if the response was bad and letting higher layers know that should quickly cut off the transactions.
PATCH v4 4/4 drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
In reply to Kuogee HsiehAdd checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if dp aux read/write failed at timeout due to cable unplugged.
Changes in V4:
-- split this patch as stand alone patch
--- drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++++--- drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-)
drivers/gpu/drm/msm/dp/dp_display.c +9 -3
@@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)return 0 ; } - ret =dp_display_usbpd_attention_cb (&dp->pdev->dev); -if (ret == -ECONNRESET ) {/* cable unplugged */ - dp->core_initialized = false; +/* +* dp core (ahb/aux clks) must be initialized before +* irq_hpd be handled +*/ +if (dp->core_initialized) { + ret =dp_display_usbpd_attention_cb (&dp->pdev->dev); +if (ret == -ECONNRESET ) {/* cable unplugged */ + dp->core_initialized = false; + } }mutex_unlock (&dp->event_mutex);
drivers/gpu/drm/msm/dp/dp_link.c +15 -5
@@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct dp_link *dp_link)return 0 ; } -static void dp_link_parse_sink_status_field (struct dp_link_private *link) +static int dp_link_parse_sink_status_field (struct dp_link_private *link) {int len =0 ; link->prev_sink_count = link->dp_link.sink_count; -dp_link_parse_sink_count (&link->dp_link); + len =dp_link_parse_sink_count (&link->dp_link); +if (len <0 ) { +DRM_ERROR ("DP parse sink count failed\n" ); +return len; + } len =drm_dp_dpcd_read_link_status (link->aux, link->link_status); -if (len <DP_LINK_STATUS_SIZE ) +if (len <DP_LINK_STATUS_SIZE ) {DRM_ERROR ("DP link status read failed\n" ); -dp_link_parse_request (link); +return len; + } + +return dp_link_parse_request (link); }/**
@@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link *dp_link)dp_link_reset_data (link); -dp_link_parse_sink_status_field (link); + ret =dp_link_parse_sink_status_field (link); +if (ret) { +return ret; + }if (link->request.test_requested ==DP_TEST_LINK_EDID_READ ) { dp_link->sink_request |=DP_TEST_LINK_EDID_READ ;
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 4/4] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Quoted 2 lines
Add checking aux read/write status at both
dp_link_parse_sink_count()anddp_link_parse_sink_status_filed()to avoid long timeout delay if
s/filed/field/
Quoted 6 lines
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4:
-- split this patch as stand alone patchSigned-off-by Kuogee Hsieh <[email protected]>
Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Re: [PATCH v4 4/4] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
Quoted 11 lines · Stephen Boyd
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Quoted 2 lines
Add checking aux read/write status at both
dp_link_parse_sink_count()anddp_link_parse_sink_status_filed()to avoid long timeout delay ifs/filed/field/
Quoted 5 lines
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4:
-- split this patch as stand alone patch
Quoted 7 lines
Quoted 1 line
Signed-off-by Kuogee Hsieh <[email protected]>Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Reviewed-by Stephen Boyd <[email protected]>Tested-by Stephen Boyd <[email protected]>
Is this something that we still need to pursue/merge?
There were changes requested for this and for the previous patch, but no new versions were posted.
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH v4 4/4] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
Quoted 17 lines · Dmitry Baryshkov
Quoted 16 lines · Stephen Boyd
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Quoted 2 lines
Add checking aux read/write status at both
dp_link_parse_sink_count()anddp_link_parse_sink_status_filed()to avoid long timeout delay ifs/filed/field/
Quoted 6 lines
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4:
-- split this patch as stand alone patchSigned-off-by Kuogee Hsieh <[email protected]>Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Quoted 7 lines
Quoted 2 lines
Reviewed-by Stephen Boyd <[email protected]>Tested-by Stephen Boyd <[email protected]>Is this something that we still need to pursue/merge?
There were changes requested for this and for the previous patch, but no new versions were posted.
It is my fault to miss this one.
The first two patches of this serial are merged.
I will rebase and re submit this one to V5.10.
Quoted 6 lines · Kuogee Hsieh
1) check
sink_countbefore updateis_connectedstatus
2) initializeaudio_compwhen audio starts
3) check main link status before start aux read
4)dp_link_parse_sink_count()return immediately if aux read failed
Quoted 6 lines
Kuogee Hsieh (4):
drm/msm/dp: checksink_countbefore updateis_connectedstatus drm/msm/dp: initializeaudio_compwhen audio starts drm/msm/dp: check main link status before start aux read drm/msm/dp:dp_link_parse_sink_count()return immediately if aux read
failed
I've picked up these two in msm-next for an upcoming -fixes pull req:
drm/msm/dp: initialize audio_comp when audio starts drm/msm/dp: check sink_count before update is_connected status
BR,
-R
Quoted 12 lines
drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++++++++++------------ drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_link.c | 20 ++++++++++++++----- 5 files changed, 48 insertions(+), 17 deletions(-)--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project