linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: Wed, 19 May 2021 17:06:32 -0400	[thread overview]
Message-ID: <CAE-0n50yRCA00ck_FtXwzKw_R8UcocMzTh8V7NOe4ob__3G3bg@mail.gmail.com> (raw)
In-Reply-To: <c1a3ced9ac4682bae310712a11576322@codeaurora.org>

Quoting khsieh@codeaurora.org (2021-05-19 09:01:02)
> On 2021-05-18 14:42, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-05-14 10:35:13)
> >> irq_hpd interrupt should be handled after dongle plugged in and
> >> before dongle unplugged. Hence irq_hpd interrupt is enabled at
> >> the end of the plugin handle and disabled at the beginning of
> >> unplugged handle. Current irq_hpd with sink_count = 0 is wrongly
> >> handled same as the dongle unplugged which tears down the mainlink
> >> and disables the phy. This patch fixes this problem by only tearing
> >> down the mainlink but keeping phy enabled at irq_hpd with
> >> sink_count = 0 handle so that next irq_hpd with sink_count =1 can be
> >> handled by setup mainlink only.
> >>
> >> Changes in v2:
> >> -- add ctrl->phy_Power_count
> >>
> >> Changes in v3:
> >> -- del ctrl->phy_Power_count
> >> -- add phy_power_off to dp_ctrl_off_link_stream()
> >>
> >> Changes in v4:
> >> -- return immediately if clock disable failed at
> >> dp_ctrl_off_link_stream()
> >>
> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> >
> > I think we want some Fixes tag. Not sure what it would be though.
> >
> > I also noticed that if I plug and unplug the HDMI cable from my apple
> > dongle that I see this error message
> >
> >   [drm:dp_display_usbpd_attention_cb] *ERROR* Disconnected, no
> > DP_LINK_STATUS_UPDATED
>
> > *ERROR* Disconnected, no DP_LINK_STATUS_UPDATED <== this is caused by
> > dongle generate the second
> irq_hpd with sink_count = 0 after first first irq_hpd with sink_count =
> 0. The fix is you have
> set dongle to D3 (power off) state after first irq_pd with sink_count =
> 0 handled.
> I have a patch fix this problem. I will merge and re submit for review.

That's good. I still don't understand how the kthread can't race with
irq_hpd and hpd going low though. Userspace will have to disable the
display and that could happen far later than the time that the hpd low
interrupt fires and is processed. Can't hpd go high during that time and
then blip before userspace notices and disables the display?

Put another way, putting the dongle into D3 state may make the race
window smaller, but it's not fixing the root cause of the problem which
is that the kthread is running later and userspace is involved in the
state of the dongle while irqs are firing. The three different contexts
have to coordinate work, so it feels like a better approach would be to
shut off the irq_hpd interrupt once hpd goes low for an unplug in
hardirq context so that we don't have to consider the cable state or
userspace changing the state after we notify it.

  reply	other threads:[~2021-05-19 21:06 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 17:35 [PATCH v4 1/2] drm/msm/dp: handle irq_hpd with sink_count = 0 correctly Kuogee Hsieh
2021-05-18 21:42 ` Stephen Boyd
2021-05-19 16:01   ` khsieh
2021-05-19 21:06     ` Stephen Boyd [this message]
2021-05-20 16:08       ` khsieh
2021-05-20 19:28         ` Stephen Boyd
2021-05-20 20:05           ` khsieh
2021-05-20 21:28             ` Stephen Boyd
2021-05-20 22:39               ` khsieh
2021-05-21  5:00                 ` Stephen Boyd
2021-05-21 15:21                   ` khsieh
2021-05-21 19:18                     ` Stephen Boyd
2021-05-21 19:45                       ` khsieh
2021-05-21 20:54                         ` Stephen Boyd
2021-05-20 20:36           ` khsieh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAE-0n50yRCA00ck_FtXwzKw_R8UcocMzTh8V7NOe4ob__3G3bg@mail.gmail.com \
    --to=swboyd@chromium.org \
    --cc=abhinavk@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=aravindh@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=khsieh@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=sean@poorly.run \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).