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

[khsieh]

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.

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?