Quoting khsieh@codeaurora.org (2021-05-03 12:23:31)
On 2021-04-29 20:11, Stephen Boyd wrote:
Quoting khsieh@codeaurora.org (2021-04-29 10:23:31)
On 2021-04-29 02:26, Stephen Boyd wrote:
Quoting khsieh@codeaurora.org (2021-04-28 10:38:11)
On 2021-04-27 17:00, Stephen Boyd wrote:
Quoting aravindh@codeaurora.org (2021-04-21 11:55:21) > On 2021-04-21 10:26, khsieh@codeaurora.org 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
- plug dongle only
- plug hdmi monitor into dongle (generated irq_hpd with sinc_count = 1)
- 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.