Re: [PATCH] drm/msm/dp: power off DP phy base on mainlink status at suspend

[khsieh]

On 2021-05-26 15:30, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-05-26 11:08:23)
> > DP mainlink can be either enabled or disabled at the time of suspend
> > happen. Therefore DP phy teared down at suspend should base on 
> > mainlink
> > status at that instance.
>
> Please add some more details here. The system crashes if you plug in 
> the
> HDMI cable during system wide suspend. That seems to be because the DP
> phy isn't powered down during suspend if the HDMI cable is disconnected
> so we try to process the hpd plug event on the path to suspend instead
> of wait to bring up the phy and then the display?
>
> I'm trying to find the case when we would be entering suspend and only
> have called phy_init() without calling phy_exit(). What path is that? I
> guess it is dp_ctrl_off_link_stream() called when the sink count goes 
> to
> 0? So plug in HDMI cable to apple dongle, unplug HDMI cable to apple
> dongle and phy_power_off() followed by phy_exit() followed by 
> phy_init()
> and then enter suspend so we want to call phy_exit(). Then we only call
> phy_power_off() if we've called dp_ctrl_on()? I think I followed it 
> all.
ok, will do
> > Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 5 ++++-
> >  drivers/gpu/drm/msm/dp/dp_ctrl.h    | 2 +-
> >  drivers/gpu/drm/msm/dp/dp_display.c | 9 ++++++++-
> >  3 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
> > b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index dbd8943..5115c05 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -1398,7 +1398,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, 
> > bool flip, bool reset)
> >   * Perform required steps to uninitialize DP controller
> >   * and its resources.
> >   */
> > -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
> > +void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl, bool mainlink_on)
> >  {
> >         struct dp_ctrl_private *ctrl;
> >         struct dp_io *dp_io;
> > @@ -1414,6 +1414,9 @@ void dp_ctrl_host_deinit(struct dp_ctrl 
> > *dp_ctrl)
> >         phy = dp_io->phy;
> >
> >         dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> > +       if (mainlink_on)
> > +               phy_power_off(phy);
> > +
> >         phy_exit(phy);
> >
> >         DRM_DEBUG_DP("Host deinitialized successfully\n");
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h 
> > b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > index 25e4f75..a23ee2b 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> > @@ -20,7 +20,7 @@ struct dp_ctrl {
> >  };
> >
> >  int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool 
> > reset);
> > -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl);
> > +void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl, bool mainlink_on);
> >  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> >  int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
> >  int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index cdec0a3..88eeeb5 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -104,6 +104,8 @@ struct dp_display_private {
> >
> >         bool encoder_mode_set;
> >
> > +       bool mainlink_on;
> > +
>
> Is there a reason why this can't be stashed away in dp_ctrl.c in the
> 'struct dp_ctrl'? It seems to follow closely with dp_ctrl_*() APIs.
yes, I will do that.

> >         /* wait for audio signaling */
> >         struct completion audio_comp;
> >
> > @@ -353,11 +355,14 @@ static int dp_display_process_hpd_high(struct 
> > dp_display_private *dp)
> >         dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> >
> >         dp_link_reset_phy_params_vx_px(dp->link);
> > +
> > +       dp->mainlink_on = false;
>
> Isn't this too late to be setting it to false? i.e. it should be false
> by default, and then set to false when a dp_ctrl_off() call is made?
>
> >         rc = dp_ctrl_on_link(dp->ctrl);
> >         if (rc) {
> >                 DRM_ERROR("failed to complete DP link training\n");
> >                 goto end;
> >         }
> > +       dp->mainlink_on = true;
> >
> >         dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
> >
> > @@ -392,7 +397,7 @@ static void dp_display_host_deinit(struct 
> > dp_display_private *dp)
> >                 return;
> >         }
> >
> > -       dp_ctrl_host_deinit(dp->ctrl);
> > +       dp_ctrl_host_deinit(dp->ctrl, dp->mainlink_on);
> >         dp_aux_deinit(dp->aux);
> >         dp_power_deinit(dp->power);
> >
> > @@ -941,6 +946,8 @@ static int dp_display_disable(struct 
> > dp_display_private *dp, u32 data)
> >                 dp->core_initialized = false;
> >         }
> >
> > +       dp->mainlink_on = false;
> > +
> >         dp_display->power_on = false;
> >
> >         return 0;
>
> It would certainly help to keep it contained to one file instead of 
> two.
agree,