Re: [PATCH v15 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received

[Kuogee Hsieh]

On 1/14/2022 1:41 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-01-14 13:11:47)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 7cc4d21..7cd6222 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -696,12 +699,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
> >           * dp core (ahb/aux clks) must be initialized before
> >           * irq_hpd be handled
> >           */
> > -       if (dp->core_initialized) {
> > -               ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> > -               if (ret == -ECONNRESET) { /* cable unplugged */
> > -                       dp->core_initialized = false;
> > -               }
> > -       }
> > +       if (dp->core_initialized)
> When is this condition false? The irq isn't unmasked until the core has
> been initialized. On the resume path I suppose the irq is enabled in
> dp_display_host_init() calling dp_ctrl_reset_irq_ctrl(), and then we
> could immediately get the interrupt but it will block on the event_mutex
> lock.

This is left over form Lazor.

I remember that there is an extreme case that several irq_hpd interrupts 
happen right after dongle plug in  (happen at resume too) and sometime 
cause system crash at dpcd read due to AHB clock is not enabled yet. It 
took some time to debug it.

From looking into code, it does not look likely it will happen. But it 
did happen at real world.

So that I would like to keep this condition checking.


> > +               dp_display_usbpd_attention_cb(&dp->pdev->dev);
> > +
> >          DRM_DEBUG_DP("hpd_state=%d\n", state);
> >
> >          mutex_unlock(&dp->event_mutex);
> > @@ -1363,14 +1373,16 @@ static int dp_pm_suspend(struct device *dev)
> >                  if (dp_power_clk_status(dp->power, DP_CTRL_PM))
> >                          dp_ctrl_off_link_stream(dp->ctrl);
> >
> > +               dp_display_host_phy_exit(dp);
> > +
> > +               /* host_init will be called at pm_resume */
> >                  dp_display_host_deinit(dp);
> > +       } else {
> > +               dp_display_host_phy_exit(dp);
> I fail to see where this condition happens. Can we suspend the device
> without the irq being installed?

Agree, with this new mechanism it should not happen.

Will remove it.

> >          }
> >
> >          dp->hpd_state = ST_SUSPENDED;
> >
> > -       /* host_init will be called at pm_resume */
> > -       dp->core_initialized = false;
> > -
> >          DRM_DEBUG_DP("After, core_inited=%d power_on=%d\n",
> >                          dp->core_initialized, dp_display->power_on);