Re: [PATCH v4 1/2] drm/msm/dp: handle irq_hpd with sink_count = 0 correctly

[Stephen Boyd]

Quoting khsieh@xxxxxxxxxxxxxx (2021-05-20 15:39:09)
> On 2021-05-20 14:28, Stephen Boyd wrote:
> > Quoting khsieh@xxxxxxxxxxxxxx (2021-05-20 13:05:48)
> >> On 2021-05-20 12:28, Stephen Boyd wrote:
> >> >> Put dongle to D3 state so that it will not issue the unnecessary
> >> >> second
> >> >> irq_hpd with sink_count = 0. this will prevent the annoy but unharmful
> >> >> DP_LINK_STATUS_UPDATED warning message.
> >> >> Again, we can not disable hpd interrupt since dongle still attached
> >> >> and
> >> >> hdmi cable can be plugged in at any instant.
> >> >>
> >> >
> >> > Right I'm not suggesting to disable hpd interrupt, just the hpd_irq
> >> > interrupt once an unplug irq comes in, and do that in hardirq context.
> >> > Also, I'm suggesting that we consider unplug as a higher priority if
> >> > the
> >> > hard irq handler is delayed for some reason and both an unplug irq and
> >> > an hpd irq are pending in the hardware when the hard irq handler is
> >> > running. Putting the dongle into D3 state won't fix these problems.
> >>
> >>
> >>
> >> The unplug interrupt is not happen in this case since dongle still
> >> attached.
> >> The unplug interrupt only happen when dongle unplugged.
> >
> > Agreed.
> >
> >>
> >> I think you mistakenly think DP_LINK_STATUS_UPDATED is caused by
> >> unplug
> >> interrupt.
> >
> > Ok, got it.
> >
> >> DP_LINK_STATUS_UPDATED happen is due to dongle issue two consecutive
> >> irq_hpd with sink_count = 0 when hdmi cable unplugged from dongle.
> >> The first irq_hpd with sink_count = 0 is handled as expected to turn
> >> off
> >> display.
> >> After that the second irq_hpd with sink_count = 0 is handled.
> >> Since display had turned off, then there is nothing to do but spill
> >> DP_LINK_STATUS_UPDATED warning message.
> >> There is no unplug (hpd become low) happen in this case since dongle
> >> still attached.
> >
> > Agreed.
> >
> >>
> >> All interrupt (plug/irq_hpd and unplug) are required to be handled in
> >> the order of happening.
> >> We can not ignore any one.
> >> For example, you plug/unplug two different resolution monitor
> >> alternative to/from dongle and unplug dongle once for while.
> >>
> >> I think the race condition you describe here all had been taken care
> >> with
> >> 1) convert irq into event and store at event q in order.
> >> 2) irq handled base on transaction. Next irq can be handled when
> >> previous irq transaction is done.
> >>
> >
> > I'm mostly trying to point out that the irq handling and masking needs
> > to be done in the hard irq context and not in the kthread. It may or
> > may
> > not be related to this message that's printed.
> >
> > What happens if the hardirq is blocked by some other irq that takes a
> > long time to process? Imagine this scenario:
> >
> > CPU0                                CPU1
> > ----                                ----
> >  really_long_other_hardirq() {
> >                                     hpd_irq
> >                                   hpd_irq
> >                                   hpd low
> >  }
> >
> >  dp_display_irq_handler() {
> >
>
>
> >    <fork things to kthread>
> >  }
> >
> > Shouldn't we ignore any hpd_irq events in this scenario? And shouldn't
> > we be disabling the hpd_irq by masking it with DP_DP_IRQ_HPD_INT_MASK
> > when hpd goes low (i.e. DP_DP_HPD_UNPLUG_INT_MASK)?
>
>
>
> 1) irq_hpd interrupt always happen before unplug interrupt
> 2)if hdp_isr_status = (DP_DP_IRQ_HPD_INT_MASK |
> DP_DP_HPD_UNPLUG_INT_MASK) at the time when read at
> dp_display_irq_handler(),
> then DP_DP_IRQ_HPD_INT_MASK will be add into evetn q first followed by
> DP_DP_HPD_UNPLUG_INT_MASK be add into event q.
> So that DP_DP_IRQ_HPD_INT_MASK will be executed by the event thread
> before DP_DP_HPD_UNPLUG_INT_MASK.
> On the other word, IRQ_HPD has higher priority over UNPLUG in the timing
> matter.
> By doing that we can shut down display gracefully.

Ok. So you're saying that we want to put both events on the queue
regardless, and put IRQ_HPD there first because we want to check the
status bit? Doesn't reading the status bit require the dongle to be
connected though? So if an unplug came in along with an irq_hpd we may
queue both the irq_hpd and the unplug, but when it comes time to process
the irq_hpd in the kthread the link will be gone and so trying the dpcd
read for the link status will fail?

>
> If you insist, at hdp_isr_status = (DP_DP_IRQ_HPD_INT_MASK |
> DP_DP_HPD_UNPLUG_INT_MASK) case,
> we can have only add DP_DP_HPD_UNPLUG_INT_MASK to event q only by
> dropping DP_DP_IRQ_HPD_INT_MASK.
> Is this will work for you?
>

I'm not insisting on anything. I'm just trying to think of various
corner cases that we're not handling in the code so we don't have to
worry about them in the future.