Re: [PATCH v7 2/2] drm/msm/dp: do not initialize phy until plugin interrupt received

[Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>]

On 12/14/2021 5:50 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-12-09 13:35:07)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 0766752..cfbc5e4 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -83,6 +83,7 @@ struct dp_display_private {
> >
> >          /* state variables */
> >          bool core_initialized;
> > +       bool phy_initialized;
> >          bool hpd_irq_on;
> >          bool audio_supported;
> >
> > @@ -371,21 +372,46 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
> >          return rc;
> >   }
> >
> > -static void dp_display_host_init(struct dp_display_private *dp, int reset)
> > +static void dp_display_host_phy_init(struct dp_display_private *dp)
> >   {
> > -       bool flip = false;
> > +       DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> > +                       dp->core_initialized, dp->phy_initialized);
> >
> > +       if (!dp->phy_initialized) {
> Is this check only here because we don't know when this function is
> getting called? I see in the DP case we get here from
> dp_display_usbpd_configure_cb() but in the eDP case we get here from
> dp_display_host_init() and presumably again from
> dp_display_usbpd_configure_cb() called by dp_hpd_plug_handle().
>
> If at all possible, I'd prefer to not have another tracking variable and
> call dp_display_host_phy_init() from the same place regardless of DP or
> eDP. Doing that would make it symmetric, per the commit text.

Agree, but  dp->phy_initialized variable used to track some other 
conditions.

For example, phy required to be re inited if dpcd read failed due to 
cable unplugged.

other dpcd read failed will not required to re inited.

Therefore I think dp->phy_initialized variable is still required.

> > +               dp_ctrl_phy_init(dp->ctrl);
> > +               dp->phy_initialized = true;
> > +       }
> > +}
> > +
> > +static void dp_display_host_phy_exit(struct dp_display_private *dp)
> > +{
> > +       DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
> > +                       dp->core_initialized, dp->phy_initialized);
> > +
> > +       if (dp->phy_initialized) {
> > +               dp_ctrl_phy_exit(dp->ctrl);
> > +               dp->phy_initialized = false;
> > +       }
> > +}
> > +
> > +static void dp_display_host_init(struct dp_display_private *dp)
> > +{
> >          DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized);
> >          if (dp->core_initialized) {
> >                  DRM_DEBUG_DP("DP core already initialized\n");
> >                  return;
> >          }
> >
> > -       if (dp->usbpd->orientation == ORIENTATION_CC2)
> > -               flip = true;
> > +       dp_power_init(dp->power, false);
> > +       dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> > +
> > +       /*
> > +        * eDP is the embedded primary display and has its own phy
> > +        * initialize phy immediately
> Can we get some more details here? Why is it better to initialize the
> phy here vs. when HPD goes high on the panel? The comment says what the
> code is doing but it isn't telling us why that's OK.

Will do.

> > +        */
> > +       if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP)
> > +               dp_display_host_phy_init(dp);
> >
> > -       dp_power_init(dp->power, flip);
> > -       dp_ctrl_host_init(dp->ctrl, flip, reset);
> >          dp_aux_init(dp->aux);
> >          dp->core_initialized = true;
> >   }