lkml.org 
[lkml]   [2020]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train
From
Date
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);

\
 
 \ /
  Last update: 2020-10-03 04:13    [W:0.047 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site