Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20201113222639.18786-1-khsieh@codeaurora.org>
References: <20201113222639.18786-1-khsieh@codeaurora.org>
Subject: Re: [PATCH] drm/msm/dp: fix connect/disconnect handled at ir_hdp
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     tanmay@codeaurora.org, abhinavk@codeaurora.org,
        aravindh@codeaurora.org, khsieh@codeaurora.org, airlied@linux.ie,
        daniel@ffwll.ch, linux-arm-msm@vger.kernel.org,
        dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
To:     Kuogee Hsieh <khsieh@codeaurora.org>, robdclark@gmail.com,
        sean@poorly.run
Date:   Mon, 16 Nov 2020 17:00:12 -0800
Message-ID: <160557481298.60232.17821932910000059522@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

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);
>  }
> =20
> -static int dp_display_send_hpd_notification(struct dp_display_private *d=
p,
> -                                           bool hpd)
> +
> +static void dp_display_set_encoder_mode(struct dp_display_private *dp)
>  {
> -       static bool encoder_mode_set;
>         struct msm_drm_private *priv =3D dp->dp_display.drm_dev->dev_priv=
ate;
>         struct msm_kms *kms =3D 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);
> =20
> +               encoder_mode_set =3D true;
> +       }
> +}
> +
> +static int dp_display_send_hpd_notification(struct dp_display_private *d=
p,
> +                                           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 dev=
ice *dev)
>         if (!rc) {
>                 sink_request =3D dp->link->sink_request;
>                 if (sink_request & DS_PORT_STATUS_CHANGED) {
> -                       /* same as unplugged */
> -                       hpd->hpd_high =3D 0;
> -                       dp->hpd_state =3D ST_DISCONNECT_PENDING;
> -                       dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> -               }
> -
> -               rc =3D dp_display_handle_irq_hpd(dp);
> -
> -               if (!rc && (sink_request & DS_PORT_STATUS_CHANGED)) {
> -                       hpd->hpd_high =3D 1;
> -                       dp->hpd_state =3D 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 !=3D ST_DISCONNECTED) {
> +                                       hpd->hpd_high =3D 0;
> +                                       dp->hpd_state =3D ST_DISCONNECT_P=
ENDING;
> +                                       dp_add_event(dp, EV_USER_NOTIFICA=
TION, false, 0);
> +                               }
> +                               rc =3D -ENOTCONN;
> +                       } else {
> +                               if (dp->hpd_state =3D=3D ST_DISCONNECTED)=
 {
> +                                       hpd->hpd_high =3D 1;

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

> +                                       dp->hpd_state =3D ST_CONNECT_PEND=
ING;
> +
> +                                       rc =3D dp_display_process_hpd_hig=
h(dp);
> +                                       if (rc) {
> +                                               hpd->hpd_high =3D 0;
> +                                               dp->hpd_state =3D ST_DISC=
ONNECTED;
> +                                       }
> +                               }
> +                       }
> +               } else {
> +                       if (!dp_display_is_ds_bridge(dp->panel))
> +                               rc =3D dp_display_handle_irq_hpd(dp);
>                 }
>         }
>