Quoting Kuogee Hsieh (2022-04-14 14:03:43)yes, how about change to ST_MAINLINK_READY?
Two stages are required to setup up main link to be ready to transmitReviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
video stream.
Stage 1: dp_hpd_plug_handle() perform link training to set up main link
stage 2: user space framework (msm_dp_display_enable()) to enable pixel
clock and transfer main link to video ready state.
At current implementation, when dongle unplugged dp_hdp_unplug_handle()
has to wait until stage 2 completed before it can send link down uevent
to user space framework to disable pixel clock followed by tearing down
main link. This introduce unnecessary latency if dongle unplugged happen
after stage 1 and before stage 2. It also has possibility leave main link
stay at ready state after dongle unplugged if framework does not response
to link down uevent notification. This will prevent next dongle plug in
from working. This scenario could possibly happen when dongle unplug while
system in the middle of suspending.
This patch allow unplug handle to tear down main link and notify
framework link down immediately if dongle unplugged happen after
stage 1 and before stage 2. With this approach, dp driver is much
more resilient to any different scenarios. Also redundant both
dp_connect_pending_timeout() and dp_disconnect_pending_timeout()
are removed to reduce logic complexity.
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
Some questions below but doesn't seem like it will hold up this patch.
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.cI take it that ST_CONNECT_PENDING is sort of like "userspace hasn't
index 01453db..f5bd8f5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -615,24 +598,21 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
if (dp->link->sink_count == 0) {
dp_display_host_phy_exit(dp);
}
+ dp_display_notify_disconnect(&dp->pdev->dev);
mutex_unlock(&dp->event_mutex);
return 0;
- }
-
- if (state == ST_DISCONNECT_PENDING) {
+ } else if (state == ST_DISCONNECT_PENDING) {
mutex_unlock(&dp->event_mutex);
return 0;
- }
-
- if (state == ST_CONNECT_PENDING) {
- /* wait until CONNECTED */
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 1); /* delay = 1 */
+ } else if (state == ST_CONNECT_PENDING) {
handled the uevent yet and modeset hasn't been called but the link is
setup and now we want to tear it down". The state name may want to be
changed to something else.
+ dp_ctrl_off_link(dp->ctrl);Nitpick: No braces needed for single line if clauses.
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
+ dp_display_notify_disconnect(&dp->pdev->dev);
mutex_unlock(&dp->event_mutex);
return 0;
}
- dp->hpd_state = ST_DISCONNECT_PENDING;
-
/* disable HPD plug interrupts */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
@@ -640,10 +620,13 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
* We don't need separate work for disconnect as
* connect/attention interrupts are disabled
*/
- dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
+ dp_display_notify_disconnect(&dp->pdev->dev);
- /* start sentinel checking in case of missing uevent */
- dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ if (state == ST_DISPLAY_OFF) {
+ dp->hpd_state = ST_DISCONNECTED;
+ } else {
+ dp->hpd_state = ST_DISCONNECT_PENDING;
+ }
DRM_DEBUG_DP("hpd_state=%d\n", state);Is this to guard against userspace doing an atomic commit when the
/* signal the disconnect event early to ensure proper teardown */
@@ -1529,8 +1480,11 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
- /* stop sentinel checking */
- dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
+ state = dp_display->hpd_state;
+ if (state != ST_DISPLAY_OFF && state != ST_CONNECT_PENDING) {
display isn't connected? Is that even possible?
+ mutex_unlock(&dp_display->event_mutex);
+ return rc;
+ }
rc = dp_display_set_mode(dp, &dp_display->dp_mode);
if (rc) {