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

[khsieh@codeaurora.org]

On 2021-05-20 14:28, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (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?