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

[Stephen Boyd <swboyd@chromium.org>]

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