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.
