Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train

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);