Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable

[Bjorn Andersson <andersson@kernel.org>]

On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
> >
> > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO
> > pinmuxed into DP controller. HPD plug/unplug interrupts cannot be enabled until
> > internal_hpd flag is set to true.
> > At both bootup and resume time, the DP driver will enable external DP
> > plugin interrupts and handle plugin interrupt accordingly. Unfortunately
> > dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> > flag to true later than where DP driver expected during bootup time.
> >
> > This causes external DP plugin event to not get detected and display stays blank.
> > Move enabling HDP plugin/unplugged interrupts to dp_bridge_hpd_enable()/disable() to
> > set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
> > simultaneously to avoid timing issue during bootup and resume.
> >
> > Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> 
> Thanks for debugging this!
> 
> However after looking at the driver I think there is more than this.
> 
> We have several other places gated on internal_hpd flag, where we do
> not have a strict ordering of events.
> I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
> DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
> internal_hpd. Can we toggle all 4 interrupts from the
> hpd_enable/hpd_disable functions? If we can do it, then I think we can
> drop the internal_hpd flag completely.
> 

Yes, that's what I believe the DRM framework intend us to do.

The problem, and reason why I didn't do tat in my series, was that in
order to update the INT_MASKs you need to clock the IP-block and that's
done elsewhere.

So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
and pm_runtime_put() in hpd_disable().


But for edp and external HPD-signal we also need to make sure power is
on while something is connected...

> I went on and checked other places where it is used:
> - dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
> think we can drop these two calls completely. The function is under
> the event_mutex protection, so other events can not interfere.
> - dp_bridge_hpd_notify(). What is the point of this check? If some
> other party informs us of the HPD event, we'd better handle it instead
> of dropping it. Correct?  In other words, I'd prefer seeing the
> hpd_event_thread removal. Instead of that I think that on
> HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
> then the hpd_notify call should process those events.
> 

I agree, that seems to be what's expected of us from the DRM framework.

Regards,
Bjorn

> 
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 3e13acdf..71aa944 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> >         dp_display_host_init(dp);
> >         dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> > -       /* Enable plug and unplug interrupts only if requested */
> > -       if (dp->dp_display.internal_hpd)
> > -               dp_catalog_hpd_config_intr(dp->catalog,
> > -                               DP_DP_HPD_PLUG_INT_MASK |
> > -                               DP_DP_HPD_UNPLUG_INT_MASK,
> > -                               true);
> > -
> >         /* Enable interrupt first time
> >          * we are leaving dp clocks on during disconnect
> >          * and never disable interrupt
> > @@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
> >
> >         dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> > -       if (dp->dp_display.internal_hpd)
> > -               dp_catalog_hpd_config_intr(dp->catalog,
> > -                               DP_DP_HPD_PLUG_INT_MASK |
> > -                               DP_DP_HPD_UNPLUG_INT_MASK,
> > -                               true);
> > -
> >         if (dp_catalog_link_is_connected(dp->catalog)) {
> >                 /*
> >                  * set sink to normal operation mode -- D0
> > @@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
> >  {
> >         struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> >         struct msm_dp *dp_display = dp_bridge->dp_display;
> > +       struct dp_display_private *dp;
> > +
> > +       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >
> >         dp_display->internal_hpd = true;
> > +       dp_catalog_hpd_config_intr(dp->catalog,
> > +                               DP_DP_HPD_PLUG_INT_MASK |
> > +                               DP_DP_HPD_UNPLUG_INT_MASK,
> > +                               true);
> >  }
> >
> >  void dp_bridge_hpd_disable(struct drm_bridge *bridge)
> >  {
> >         struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> >         struct msm_dp *dp_display = dp_bridge->dp_display;
> > +       struct dp_display_private *dp;
> > +
> > +       dp = container_of(dp_display, struct dp_display_private, dp_display);
> >
> > +       dp_catalog_hpd_config_intr(dp->catalog,
> > +                               DP_DP_HPD_PLUG_INT_MASK |
> > +                               DP_DP_HPD_UNPLUG_INT_MASK,
> > +                               false);
> >         dp_display->internal_hpd = false;
> >  }
> 
> --
> With best wishes
> Dmitry