Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

[Stephen Boyd]

Quoting khsieh@xxxxxxxxxxxxxx (2021-05-03 12:23:31)
> On 2021-04-29 20:11, Stephen Boyd wrote:
> > Quoting khsieh@xxxxxxxxxxxxxx (2021-04-29 10:23:31)
> >> On 2021-04-29 02:26, Stephen Boyd wrote:
> >> > Quoting khsieh@xxxxxxxxxxxxxx (2021-04-28 10:38:11)
> >> >> On 2021-04-27 17:00, Stephen Boyd wrote:
> >> >> > Quoting aravindh@xxxxxxxxxxxxxx (2021-04-21 11:55:21)
> >> >> >> On 2021-04-21 10:26, khsieh@xxxxxxxxxxxxxx wrote:
> >> >> >> >>
> >> >> >> >>> +
> >> >> >> >>>         mutex_unlock(&dp->event_mutex);
> >> >> >> >>>
> >> >> >> >>>         return 0;
> >> >> >> >>> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp,
> >> >> >> >>> struct drm_encoder *encoder)
> >> >> >> >>>         /* stop sentinel checking */
> >> >> >> >>>         dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >> >> >> >>>
> >> >> >> >>> +       /* link is down, delete pending irq_hdps */
> >> >> >> >>> +       dp_del_event(dp_display, EV_IRQ_HPD_INT);
> >> >> >> >>> +
> >> >> >> >>
> >> >> >> >> I'm becoming convinced that the whole kthread design and event queue
> >> >> >> >> is
> >> >> >> >> broken. These sorts of patches are working around the larger problem
> >> >> >> >> that the kthread is running independently of the driver and irqs can
> >> >> >> >> come in at any time but the event queue is not checked from the irq
> >> >> >> >> handler to debounce the irq event. Is the event queue necessary at
> >> >> >> >> all?
> >> >> >> >> I wonder if it would be simpler to just use an irq thread and process
> >> >> >> >> the hpd signal from there. Then we're guaranteed to not get an irq
> >> >> >> >> again
> >> >> >> >> until the irq thread is done processing the event. This would
> >> >> >> >> naturally
> >> >> >> >> debounce the irq hpd event that way.
> >> >> >> > event q just like bottom half of irq handler. it turns irq into event
> >> >> >> > and handle them sequentially.
> >> >> >> > irq_hpd is asynchronous event from panel to bring up attention of hsot
> >> >> >> > during run time of operation.
> >> >> >> > Here, the dongle is unplugged and main link had teared down so that no
> >> >> >> > need to service pending irq_hpd if any.
> >> >> >> >
> >> >> >>
> >> >> >> As Kuogee mentioned, IRQ_HPD is a message received from the panel and
> >> >> >> is
> >> >> >> not like your typical HW generated IRQ. There is no guarantee that we
> >> >> >> will not receive an IRQ_HPD until we are finished with processing of
> >> >> >> an
> >> >> >> earlier HPD message or an IRQ_HPD message. For example - when you run
> >> >> >> the protocol compliance, when we get a HPD from the sink, we are
> >> >> >> expected to start reading DPCD, EDID and proceed with link training.
> >> >> >> As
> >> >> >> soon as link training is finished (which is marked by a specific DPCD
> >> >> >> register write), the sink is going to issue an IRQ_HPD. At this point,
> >> >> >> we may not done with processing the HPD high as after link training we
> >> >> >> would typically notify the user mode of the newly connected display,
> >> >> >> etc.
> >
> > I re-read this. I think you're saying that IRQ_HPD can come in after
> > HPD
> > goes high and we finish link training? That sounds like we should
> > enable
> > IRQ_HPD in the hardware once we finish link training, instead of having
> > it enabled all the time. Then we can finish the threaded irq handler
> > and
> > the irq should be pending again once IRQ_HPD is sent over. Is there
> > ever
> > a need to be processing some IRQ_HPD and then get another IRQ_HPD while
> > processing the first one?
> yes, for example
> 1) plug dongle only
> 2) plug hdmi monitor into dongle (generated irq_hpd with sinc_count = 1)
> 3) unplug hdmi monitor out of the dongle (generate irq_hpd with
> sinc_count = 0)
> 4) go back to 2) for n times
> 5) unplug dongle
>
> This patch is not fix this problem either.
> The existing code has major issue which is handle irq_hpd with
> sink_count = 0 same way as handle of dongle unplugged.
> I think this cause external dp display failed to work and cause crash at
> suspend/resume test case.
> I will drop this patch.
> I am working on handle irq_hpd with sink_count = 0 as asymmetric as
> opposite to  irq_hpd with sink_count = 1.
> This means irq_hdp sink_count = 0 handle only tear down the main link
> but keep phy/aux intact.
> I will re submit patch for review.
>

Ok makes sense. I'll look out for the next revision of this patch.