Re: [PATCH] drm/msm/dp: fix connect/disconnect handled at ir_hdp

[Stephen Boyd]

Quoting Kuogee Hsieh (2020-11-13 14:26:39)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 27e7e27b8b90..4e84f500b721 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -279,13 +279,25 @@ static void dp_display_send_hpd_event(struct msm_dp *dp_display)
>         drm_helper_hpd_irq_event(connector->dev);
>  }
>  
> -static int dp_display_send_hpd_notification(struct dp_display_private *dp,
> -                                           bool hpd)
> +
> +static void dp_display_set_encoder_mode(struct dp_display_private *dp)
>  {
> -       static bool encoder_mode_set;
>         struct msm_drm_private *priv = dp->dp_display.drm_dev->dev_private;
>         struct msm_kms *kms = priv->kms;
> +       static bool encoder_mode_set;

Can this be stored in the dp_display_private structure instead? No
singletons please.

> +
> +       if (!encoder_mode_set && dp->dp_display.encoder &&
> +                               kms->funcs->set_encoder_mode) {
> +               kms->funcs->set_encoder_mode(kms,
> +                               dp->dp_display.encoder, false);
>  
> +               encoder_mode_set = true;
> +       }
> +}
> +
> +static int dp_display_send_hpd_notification(struct dp_display_private *dp,
> +                                           bool hpd)
> +{
>         if ((hpd && dp->dp_display.is_connected) ||
>                         (!hpd && !dp->dp_display.is_connected)) {
>                 DRM_DEBUG_DP("HPD already %s\n", (hpd ? "on" : "off"));
> @@ -491,17 +487,29 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
>         if (!rc) {
>                 sink_request = dp->link->sink_request;
>                 if (sink_request & DS_PORT_STATUS_CHANGED) {
> -                       /* same as unplugged */
> -                       hpd->hpd_high = 0;
> -                       dp->hpd_state = ST_DISCONNECT_PENDING;
> -                       dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> -               }
> -
> -               rc = dp_display_handle_irq_hpd(dp);
> -
> -               if (!rc && (sink_request & DS_PORT_STATUS_CHANGED)) {
> -                       hpd->hpd_high = 1;
> -                       dp->hpd_state = ST_CONNECT_PENDING;
> +                       if (dp_display_is_sink_count_zero(dp)) {
> +                               DRM_DEBUG_DP("sink count is zero, nothing to do\n");
> +                               if (dp->hpd_state != ST_DISCONNECTED) {
> +                                       hpd->hpd_high = 0;
> +                                       dp->hpd_state = ST_DISCONNECT_PENDING;
> +                                       dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> +                               }
> +                               rc = -ENOTCONN;
> +                       } else {
> +                               if (dp->hpd_state == ST_DISCONNECTED) {
> +                                       hpd->hpd_high = 1;

This else and then if can be an else if, right?

> +                                       dp->hpd_state = ST_CONNECT_PENDING;
> +
> +                                       rc = dp_display_process_hpd_high(dp);
> +                                       if (rc) {
> +                                               hpd->hpd_high = 0;
> +                                               dp->hpd_state = ST_DISCONNECTED;
> +                                       }
> +                               }
> +                       }
> +               } else {
> +                       if (!dp_display_is_ds_bridge(dp->panel))
> +                               rc = dp_display_handle_irq_hpd(dp);
>                 }
>         }
>