On Wed, 17 May 2023 at 20:42, Kuogee Hsieh <quic_khs...@quicinc.com> wrote:
>
>
> On 5/16/2023 4:20 PM, Dmitry Baryshkov wrote:
> > On 17/05/2023 01:35, Abhinav Kumar wrote:
> >>
> >>
> >> On 5/16/2023 6:50 AM, Dmitry Baryshkov wrote:
> >>> On 13/05/2023 00:28, Abhinav Kumar wrote:
> >>>> Hi Bjorn and Dmitry
> >>>>
> >>>> On 5/12/2023 12:34 PM, Kuogee Hsieh wrote:
> >>>>>
> >>>>> On 5/12/2023 10:28 AM, Dmitry Baryshkov wrote:
> >>>>>> On Fri, 12 May 2023 at 19:52, Kuogee Hsieh
> >>>>>> <quic_khs...@quicinc.com> wrote:
> >>>>>>>
> >>>>>>> On 5/11/2023 5:54 PM, Dmitry Baryshkov wrote:
> >>>>>>>> On Fri, 12 May 2023 at 03:16, Kuogee Hsieh
> >>>>>>>> <quic_khs...@quicinc.com> wrote:
> >>>>>>>>> On 5/11/2023 8:57 AM, Dmitry Baryshkov wrote:
> >>>>>>>>>> On 11/05/2023 18:53, Bjorn Andersson wrote:
> >>>>>>>>>>> On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>> On Wed, 10 May 2023 at 23:31, Kuogee Hsieh
> >>>>>>>>>>>> <quic_khs...@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_khs...@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.
> >>>>>>>>>>>>
> >>>>
> >>>> No we cannot. The HPD logic works in a flip-flop model. When we get
> >>>> the plug interrupt, we need to flip to tell the hw to wait for
> >>>> unplug and when we get unplug, we need to tell the hw to wait for
> >>>> plug.
> >>>
> >>> But, doesn't dp_display_config_hpd() (current code) or
> >>> dp_bridge_hpd_enable() (after this patch) enable both plug and
> >>> unplug interrupts? This doesn't fit well into the flip-flop
> >>> description.
> >>>
> >>
> >> Let me clarify / correct the response. Ideally thats what is usually
> >> done to wait for disconnect when we get connect and vice-versa. HDMI
> >> still does it the same way.
> >>
> >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c#L196
> >>
> >
> > So, HDMI HPD is real flip-flop, sounds fine.
> >
> >>
> >> But I checked with kuogee that DP always enabled HPD connect and
> >> disconnect interrupts by default and he mentioned its mainly because
> >> we wanted to enable HPD connect / disconnect by default but not the
> >> others.
> >>
> >> That being said, the logic is close to flip-flop that when you get a
> >> connect event, you wait for the "other" interrupts which are possible
> >> which is IRQ_HPD and REPLUG and during disconnect, those are not
> >> possible so disable them. Thats why the calls to
> >> dp_catalog_hpd_config_intr() are present in the plug_handle /
> >> unplug_handle to enable the "other possible" interrupts.
> >
> > Can we keep them always enabled? Are these interrupts edge-triggered
> > or level-triggered? What prevents us from enabling these interrupts
> > all the time? Or enabling all 4 interrupts in hpd_enable() and
> > disabling them in hpd_disable()? Can IRQ_HPD or REPLUG fire when the
> > cable is disconnected?
>
> > 1) edge-trigger at hpd pin low. However hpd block will start
> > debouncing logic and set status bit (plug, unplug, hpd_irq ore replug)
> > properly base on the result of debunce.
>
> 2) there should be fine to have all interrupts enabled
>
> 3) IRQ_HPD and REPLUG will not happen when disconnected.
>
>
> >>
> >> The logic from dp_display_config_hpd() is getting removed in this
> >> patch, in case you didnt check to align just with hpd_enable /
> >> hpd_disable callbacks.
> >
> > I saw this. The code is being moved to dp_bridge_hpd_enable(), as I
> > mentioned in the email.
> >
> >>
> >>>>
> >>>> The two calls in plug_handle() / unplug_handle() are doing that
> >>>> whereas hpd_enable/hpd_disable are disabling the hpd interrupts
> >>>> altogether.
> >>>>
> >>>> In other words, we cannot rely on hpd_enable() / hpd_disable()
> >>>> calls to do the flip part as that has to be done every plug/unplug.
> >>>> In addition we need to handle the compliance test cases with REPLUG.
> >>>
> >>> Thank you for the explanation. Would it be possible to keep
> >>> mask/unmask, but make hpd_enable/hpd_disable toggle
> >>> DP_DP_HPD_CTRL_HPD_EN instead?
> >>>
> >>
> >> Yes, this should be possible but we would like to test this. But what
> >> about the interrupt masks then. So you are saying, hpd_enable will
> >> only toggle the DP_DP_HPD_CTRL_HPD_EN but leave the HPD connect and
> >> disconnect interrupts intact? That also doesnt sound right.
> >>
> >> enabling the block all the time and then toggling the interrupt masks
> >> seemed like a better thing.
> >
> > Why? We do not need the block outside of the
> > hpd_enable()/hpd_disable() pair. Even from the power consumption
> > perspective, disabling the unused block sounds better to me.
>
>
> We are mean within the block of hpd_enabled and hdp_disabled pair,
>
> At hpd_enabled, we will do both item 1) and 2) instead of just only item
> 1) as you mentioned. you still need power on hpd block to just do item2).
>
> 1) enabled DP_DP_HPD_CTRL_HPD_EN
>
> 2) enable HDP interrupts (plug, unplug, hpd_irq and replug)
If I got you correct (and you are proposing to move
DP_DP_HPD_CTRL_HPD_EN and toggling all 4 interrupts to
hpd_enable/hpd_disable), this should be fine with me. It will allow us
to remove most of the internal_hpd cruft while keeping the logic
functional. I'm looking forward to seeing this patch.
--
With best wishes
Dmitry
