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

From: Stephen Boyd <swboyd@chromium.org>
To: khsieh@codeaurora.org
Cc: agross@kernel.org, bjorn.andersson@linaro.org,
	robdclark@gmail.com, sean@poorly.run, vkoul@kernel.org,
	abhinavk@codeaurora.org, aravindh@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/2] drm/msm/dp: handle irq_hpd with sink_count = 0 correctly
Date: Thu, 20 May 2021 14:28:57 -0700	[thread overview]
Message-ID: <CAE-0n51+mbCAqWWTOMDA4Rx_=96V4tK8g+UWVZ-nnp50dFzRPA@mail.gmail.com> (raw)
In-Reply-To: <f476d82d0798e0d7eb9e12949aa2c8f1@codeaurora.org>

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() {
   status = DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK
   <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)?