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