Connection state is set incorrectly happen at either failure of link train or cable plugged in while suspended. This patch fixes these problems. This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org --- drivers/gpu/drm/msm/dp/dp_display.c | 52 ++++++++++++++--------------- drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++ 2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 431dff9de797..898c6cc1643a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -45,7 +45,7 @@ enum { ST_CONNECT_PENDING, ST_CONNECTED, ST_DISCONNECT_PENDING, - ST_SUSPEND_PENDING, + ST_DISPLAY_OFF, ST_SUSPENDED, };
@@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) }
dp_add_event(dp, EV_USER_NOTIFICATION, true, 0); - - end: return rc; } @@ -489,7 +487,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) mutex_lock(&dp->event_mutex);
state = atomic_read(&dp->hpd_state); - if (state == ST_SUSPEND_PENDING) { + if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) { mutex_unlock(&dp->event_mutex); return 0; } @@ -511,14 +509,14 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) hpd->hpd_high = 1;
ret = dp_display_usbpd_configure_cb(&dp->pdev->dev); - if (ret) { /* failed */ + if (ret) { /* link train failed */ hpd->hpd_high = 0; atomic_set(&dp->hpd_state, ST_DISCONNECTED); + } else { + /* start sentinel checking in case of missing uevent */ + dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout); }
- /* start sanity checking */ - dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout); - mutex_unlock(&dp->event_mutex);
/* uevent will complete connection part */ @@ -563,10 +561,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) mutex_lock(&dp->event_mutex);
state = atomic_read(&dp->hpd_state); - if (state == ST_SUSPEND_PENDING) { - mutex_unlock(&dp->event_mutex); - return 0; - }
if (state == ST_DISCONNECT_PENDING || state == ST_DISCONNECTED) { mutex_unlock(&dp->event_mutex); @@ -594,7 +588,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) */ dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
- /* start sanity checking */ + /* start sentinel checking in case of missing uevent */ dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
/* signal the disconnect event early to ensure proper teardown */ @@ -634,7 +628,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
/* irq_hpd can happen at either connected or disconnected state */ state = atomic_read(&dp->hpd_state); - if (state == ST_SUSPEND_PENDING) { + if (state == ST_DISPLAY_OFF) { mutex_unlock(&dp->event_mutex); return 0; } @@ -1067,7 +1061,7 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id) }
if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) { - /* delete connect pending event first */ + /* delete sentinel connect pending checking */ dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT); dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0); } @@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)
dp = container_of(dp_display, struct dp_display_private, dp_display);
+ /* start from dis connection state */ + atomic_set(&dp->hpd_state, ST_DISCONNECTED); + dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
status = dp_catalog_hpd_get_state_status(dp->catalog);
- if (status) { + if (status) dp->dp_display.is_connected = true; - } else { + else dp->dp_display.is_connected = false; - /* make sure next resume host_init be called */ - dp->core_initialized = false; - }
return 0; } @@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev) if (dp_display->power_on == true) dp_display_disable(dp, 0);
+ /* host_init will be called at pm_resume */ + dp->core_initialized = false; + atomic_set(&dp->hpd_state, ST_SUSPENDED);
return 0; @@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
+ /* delete sentinel checking */ + dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT); + rc = dp_display_set_mode(dp, &dp_display->dp_mode); if (rc) { DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc); @@ -1368,9 +1368,8 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) dp_display_unprepare(dp); }
- dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT); - - if (state == ST_SUSPEND_PENDING) + /* manual kick off plug event to train link */ + if (state == ST_DISPLAY_OFF) dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0);
/* completed connection */ @@ -1402,20 +1401,21 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
+ /* delete sentinel checking */ + dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT); + dp_display_disable(dp_display, 0);
rc = dp_display_unprepare(dp); if (rc) DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
- dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT); - state = atomic_read(&dp_display->hpd_state); if (state == ST_DISCONNECT_PENDING) { /* completed disconnection */ atomic_set(&dp_display->hpd_state, ST_DISCONNECTED); } else { - atomic_set(&dp_display->hpd_state, ST_SUSPEND_PENDING); + atomic_set(&dp_display->hpd_state, ST_DISPLAY_OFF); }
mutex_unlock(&dp_display->event_mutex); diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 18cec4fc5e0b..1b7a20dc2d8e 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -196,6 +196,11 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, &panel->aux->ddc); if (!dp_panel->edid) { DRM_ERROR("panel edid read failed\n"); + /* check edid read fail is due to unplug */ + if (!dp_catalog_hpd_get_state_status(panel->catalog)) { + rc = -ETIMEDOUT; + goto end; + }
/* fail safe edid */ mutex_lock(&connector->dev->mode_config.mutex);
base-commit: 6fa6fd6a3e2cb76f7d54220daa1266a94c13bc4d
Quoting Kuogee Hsieh (2020-10-02 15:09:19)
Connection state is set incorrectly happen at either failure of link train or cable plugged in while suspended. This patch fixes these problems. This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Any Fixes: tag?
drivers/gpu/drm/msm/dp/dp_display.c | 52 ++++++++++++++--------------- drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++ 2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 431dff9de797..898c6cc1643a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) }
dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
end: return rc; }
Not sure we need this hunk
@@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)
dp = container_of(dp_display, struct dp_display_private, dp_display);
/* start from dis connection state */
disconnection? Or disconnected state?
atomic_set(&dp->hpd_state, ST_DISCONNECTED);dp_display_host_init(dp); dp_catalog_ctrl_hpd_config(dp->catalog); status = dp_catalog_hpd_get_state_status(dp->catalog);
if (status) {
if (status) dp->dp_display.is_connected = true;
} else {
else dp->dp_display.is_connected = false;
/* make sure next resume host_init be called */dp->core_initialized = false;} return 0;} @@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev) if (dp_display->power_on == true) dp_display_disable(dp, 0);
/* host_init will be called at pm_resume */dp->core_initialized = false;atomic_set(&dp->hpd_state, ST_SUSPENDED); return 0;@@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
/* delete sentinel checking */
Stop sentinel checking?
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);rc = dp_display_set_mode(dp, &dp_display->dp_mode); if (rc) { DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);@@ -1368,9 +1368,8 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) dp_display_unprepare(dp); }
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);if (state == ST_SUSPEND_PENDING)
/* manual kick off plug event to train link */if (state == ST_DISPLAY_OFF) dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0); /* completed connection */@@ -1402,20 +1401,21 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
/* delete sentinel checking */
Stop sentinel checking?
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);dp_display_disable(dp_display, 0); rc = dp_display_unprepare(dp); if (rc) DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);state = atomic_read(&dp_display->hpd_state); if (state == ST_DISCONNECT_PENDING) {
I don't understand the atomic nature of this hpd_state variable. Why is it an atomic variable? Is taking a spinlock bad? What is to prevent the atomic read here to not be interrupted and then this if condition check be invalid because the variable has been updated somewhere else?
/* completed disconnection */ atomic_set(&dp_display->hpd_state, ST_DISCONNECTED); } else {
atomic_set(&dp_display->hpd_state, ST_SUSPEND_PENDING);
atomic_set(&dp_display->hpd_state, ST_DISPLAY_OFF);
On 2020-10-02 19:12, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2020-10-02 15:09:19)
Connection state is set incorrectly happen at either failure of link train or cable plugged in while suspended. This patch fixes these problems. This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Any Fixes: tag?
drivers/gpu/drm/msm/dp/dp_display.c | 52 ++++++++++++++--------------- drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++ 2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 431dff9de797..898c6cc1643a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp) }
dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
end: return rc; }
Not sure we need this hunk
@@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)
dp = container_of(dp_display, struct dp_display_private,dp_display);
/* start from dis connection state */disconnection? Or disconnected state?
atomic_set(&dp->hpd_state, ST_DISCONNECTED);dp_display_host_init(dp); dp_catalog_ctrl_hpd_config(dp->catalog); status = dp_catalog_hpd_get_state_status(dp->catalog);
if (status) {
if (status) dp->dp_display.is_connected = true;
} else {
else dp->dp_display.is_connected = false;
/* make sure next resume host_init be called */dp->core_initialized = false;} return 0;} @@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev) if (dp_display->power_on == true) dp_display_disable(dp, 0);
/* host_init will be called at pm_resume */dp->core_initialized = false;atomic_set(&dp->hpd_state, ST_SUSPENDED); return 0;@@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
/* delete sentinel checking */Stop sentinel checking?
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);rc = dp_display_set_mode(dp, &dp_display->dp_mode); if (rc) { DRM_ERROR("Failed to perform a mode set, rc=%d\n",rc); @@ -1368,9 +1368,8 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) dp_display_unprepare(dp); }
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);if (state == ST_SUSPEND_PENDING)
/* manual kick off plug event to train link */if (state == ST_DISPLAY_OFF) dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0); /* completed connection */@@ -1402,20 +1401,21 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
/* delete sentinel checking */Stop sentinel checking?
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);dp_display_disable(dp_display, 0); rc = dp_display_unprepare(dp); if (rc) DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);state = atomic_read(&dp_display->hpd_state); if (state == ST_DISCONNECT_PENDING) {I don't understand the atomic nature of this hpd_state variable. Why is it an atomic variable? Is taking a spinlock bad? What is to prevent the atomic read here to not be interrupted and then this if condition check be invalid because the variable has been updated somewhere else?
hpd_state variable updated by multiple threads. however it was protected by mutex. in theory, it should also work as u32. since it was declared as atomic from beginning and it does not cause any negative effects, can we keep it as it is?
/* completed disconnection */ atomic_set(&dp_display->hpd_state, ST_DISCONNECTED); } else {
atomic_set(&dp_display->hpd_state,ST_SUSPEND_PENDING);
atomic_set(&dp_display->hpd_state, ST_DISPLAY_OFF);
Quoting khsieh@codeaurora.org (2020-10-05 11:02:10)
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);dp_display_disable(dp_display, 0); rc = dp_display_unprepare(dp); if (rc) DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);state = atomic_read(&dp_display->hpd_state); if (state == ST_DISCONNECT_PENDING) {I don't understand the atomic nature of this hpd_state variable. Why is it an atomic variable? Is taking a spinlock bad? What is to prevent the atomic read here to not be interrupted and then this if condition check be invalid because the variable has been updated somewhere else?
hpd_state variable updated by multiple threads. however it was protected by mutex. in theory, it should also work as u32. since it was declared as atomic from beginning and it does not cause any negative effects, can we keep it as it is?
It does cause negative effects by generating worse code for something that is already protected from concurrency by a mutex. Can we make it an enum and name the enum and then add a comment indicating that the 'event_mutex' lock protects this variable?
dri-devel@lists.freedesktop.org
-
khsieh＠codeaurora.org -
Kuogee Hsieh
-
Stephen Boyd