Re: [PATCH v3 2/7] drm/msm/dp: replace is_connected with link_ready

[Dmitry Baryshkov]

On Mon, 18 Sept 2023 at 20:09, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
>
>
> On 9/15/2023 6:51 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> wrote:
> >> The is_connected flag is set to true after DP mainlink successfully
> >> finish link training. Replace the is_connected flag with link_ready
> > finishes.
> > Also this is not a replace, this patch renames the flag.
> yes, it is rename.
> >
> >> flag to avoid confusing.
> > confusing what with what?
>
> we have ST_MAINLINK_RAEDY state which means mainlink had finished link
> training and ready for video.
>
> Therefore I think link_ready is more meaningful than is_connected.

Guess, all this should have been in the commit message.

>
>
> >
> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
> >>   drivers/gpu/drm/msm/dp/dp_display.h |  2 +-
> >>   drivers/gpu/drm/msm/dp/dp_drm.c     | 14 +++++++-------
> >>   3 files changed, 17 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index c217430..18d16c7 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -367,12 +367,11 @@ 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)
> >>   {
> >> -       if ((hpd && dp->dp_display.is_connected) ||
> >> -                       (!hpd && !dp->dp_display.is_connected)) {
> >> +       if ((hpd && dp->dp_display.link_ready) ||
> >> +                       (!hpd && !dp->dp_display.link_ready)) {
> >>                  drm_dbg_dp(dp->drm_dev, "HPD already %s\n",
> >>                                  (hpd ? "on" : "off"));
> >>                  return 0;
> >> @@ -382,7 +381,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
> >>          if (!hpd)
> >>                  dp->panel->video_test = false;
> >>
> >> -       dp->dp_display.is_connected = hpd;
> >> +       dp->dp_display.link_ready = hpd;
> >>
> >>          drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
> >>                          dp->dp_display.connector_type, hpd);
> >> @@ -922,7 +921,7 @@ int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> >>
> >>          dp_display->plugged_cb = fn;
> >>          dp_display->codec_dev = codec_dev;
> >> -       plugged = dp_display->is_connected;
> >> +       plugged = dp_display->link_ready;
> >>          dp_display_handle_plugged_change(dp_display, plugged);
> >>
> >>          return 0;
> >> @@ -1352,16 +1351,16 @@ static int dp_pm_resume(struct device *dev)
> >>           * also only signal audio when disconnected
> >>           */
> >>          if (dp->link->sink_count) {
> >> -               dp->dp_display.is_connected = true;
> >> +               dp->dp_display.link_ready = true;
> >>          } else {
> >> -               dp->dp_display.is_connected = false;
> >> +               dp->dp_display.link_ready = false;
> >>                  dp_display_handle_plugged_change(dp_display, false);
> >>          }
> >>
> >>          drm_dbg_dp(dp->drm_dev,
> >>                  "After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
> >>                  dp->dp_display.connector_type, dp->link->sink_count,
> >> -               dp->dp_display.is_connected, dp->core_initialized,
> >> +               dp->dp_display.link_ready, dp->core_initialized,
> >>                  dp->phy_initialized, dp_display->power_on);
> >>
> >>          mutex_unlock(&dp->event_mutex);
> >> @@ -1754,8 +1753,8 @@ void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> >>                  return;
> >>          }
> >>
> >> -       if (!dp_display->is_connected && status == connector_status_connected)
> >> +       if (!dp_display->link_ready && status == connector_status_connected)
> >>                  dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> >> -       else if (dp_display->is_connected && status == connector_status_disconnected)
> >> +       else if (dp_display->link_ready && status == connector_status_disconnected)
> >>                  dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> >>   }
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> >> index b3c08de..d65693e 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> >> @@ -16,7 +16,7 @@ struct msm_dp {
> >>          struct drm_bridge *bridge;
> >>          struct drm_connector *connector;
> >>          struct drm_bridge *next_bridge;
> >> -       bool is_connected;
> >> +       bool link_ready;
> >>          bool audio_enabled;
> >>          bool power_on;
> >>          unsigned int connector_type;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> >> index 785d766..ee945ca 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> >> @@ -24,10 +24,10 @@ static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
> >>
> >>          dp = to_dp_bridge(bridge)->dp_display;
> >>
> >> -       drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
> >> -               (dp->is_connected) ? "true" : "false");
> >> +       drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
> >> +               (dp->link_ready) ? "true" : "false");
> >>
> >> -       return (dp->is_connected) ? connector_status_connected :
> >> +       return (dp->link_ready) ? connector_status_connected :
> >>                                          connector_status_disconnected;
> >>   }
> >>
> >> @@ -40,8 +40,8 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
> >>
> >>          dp = to_dp_bridge(bridge)->dp_display;
> >>
> >> -       drm_dbg_dp(dp->drm_dev, "is_connected = %s\n",
> >> -               (dp->is_connected) ? "true" : "false");
> >> +       drm_dbg_dp(dp->drm_dev, "link_ready = %s\n",
> >> +               (dp->link_ready) ? "true" : "false");
> >>
> >>          /*
> >>           * There is no protection in the DRM framework to check if the display
> >> @@ -55,7 +55,7 @@ static int dp_bridge_atomic_check(struct drm_bridge *bridge,
> >>           * After that this piece of code can be removed.
> >>           */
> >>          if (bridge->ops & DRM_BRIDGE_OP_HPD)
> >> -               return (dp->is_connected) ? 0 : -ENOTCONN;
> >> +               return (dp->link_ready) ? 0 : -ENOTCONN;
> >>
> >>          return 0;
> >>   }
> >> @@ -78,7 +78,7 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *
> >>          dp = to_dp_bridge(bridge)->dp_display;
> >>
> >>          /* pluggable case assumes EDID is read when HPD */
> >> -       if (dp->is_connected) {
> >> +       if (dp->link_ready) {
> >>                  rc = dp_display_get_modes(dp);
> >>                  if (rc <= 0) {
> >>                          DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
> >> --
> >> 2.7.4
> >>
> >



-- 
With best wishes
Dmitry