Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
From: Kuogee Hsieh
Date: Thu Apr 14 2022 - 12:59:15 EST
On 4/13/2022 5:02 PM, Stephen Boyd wrote:
The subject is still misleading. It is fixing something. It may be
enhancing it as well but it is clearly fixing it first.
Quoting Kuogee Hsieh (2022-04-06 14:28:13)
dp_hpd_plug_handle() is responsible for setting up main link and send
uevent to notify user space framework to start video stream. Similarly,
dp_hdp_unplug_handle is responsible to send uevent to notify user space
framework to stop video stream and then tear down main link.
However there are rare cases, such as in the middle of system suspending,
that uevent could not be delivered to user space framework. Therefore
some kind of recover mechanism armed by timer need to be in place in the
case of user space framework does not respond to uevent.
This patch have both dp_conenct_pending_timeout and
dp_disconnect_pending_timeout are used to stop video stream and tear down
main link and eventually restore DP driver state to known default
DISCONNECTED state in the case of timer fired due to framework does not
respond to uevent so that DP driver can recover itself gracefully at next
dongle unplug followed by plugin event.
Changes in v2:
-- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect
I'd prefer this part to be a different patch. It can come after the fix
to ease backporting.
Also, is there any response to Dmitry's question yet? I haven't seen
anything.
Sorry, since our internal review does not like this approach.
I will upload new patch for review soon.
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,7 @@ struct dp_ctrl {
int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 178b774..a6200a5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
static int dp_display_usbpd_disconnect_cb(struct device *dev)
We shouldn't need to keep around an empty function.
{
+ return 0;
+}
+
+static void dp_display_notify_disconnect(struct device *dev)
+{
struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
-
- return 0;
}
static void dp_display_handle_video_request(struct dp_display_private *dp)
@@ -593,10 +596,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been setup but video is not ready yet
+ * only tear down main link
+ */
state = dp->hpd_state;
if (state == ST_CONNECT_PENDING) {
- dp->hpd_state = ST_CONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off_link(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);
@@ -645,6 +654,7 @@ 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;
}
@@ -661,19 +671,22 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
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);
/*
* We don't need separate work for disconnect as
* connect/attention interrupts are disabled
- */
- dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
+ */
This comment end is wrong. It should be unchanged.
+ 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 {
+ /* start sentinel checking in case of missing uevent */
+ dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ dp->hpd_state = ST_DISCONNECT_PENDING;
+ }
/* signal the disconnect event early to ensure proper teardown */
dp_display_handle_plugged_change(&dp->dp_display, false);
@@ -695,10 +708,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been set up and video is ready
+ * tear down main link, video stream and phy
+ */
state = dp->hpd_state;
if (state == ST_DISCONNECT_PENDING) {
- dp->hpd_state = ST_DISCONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);