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

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

[Kuogee Hsieh]

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>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c |  5 ++--
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 52 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++++++++++-------
 4 files changed, 92 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b1a9b1b..f4f53f2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -582,10 +582,9 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
 
 	u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
 
-	/* enable HPD interrupts */
+	/* enable HPD plug and unplug interrupts */
 	dp_catalog_hpd_config_intr(dp_catalog,
-		DP_DP_HPD_PLUG_INT_MASK | DP_DP_IRQ_HPD_INT_MASK
-		| DP_DP_HPD_UNPLUG_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true);
+		DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
 
 	/* Configure REFTIMER and enable it */
 	reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 8d59eb9..cd5b293 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1811,6 +1811,58 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 	return ret;
 }
 
+int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
+{
+	struct dp_ctrl_private *ctrl;
+	struct dp_io *dp_io;
+	struct phy *phy;
+	int ret;
+
+	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+	dp_io = &ctrl->parser->io;
+	phy = dp_io->phy;
+
+	dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
+
+	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
+	if (ret) {
+		DRM_ERROR("Failed to disable pixel clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
+	if (ret) {
+		DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
+		return ret;
+	}
+
+	phy_power_off(phy);
+
+	/* aux channel down, reinit phy */
+	phy_exit(phy);
+	phy_init(phy);
+
+	DRM_DEBUG_DP("DP off link/stream done\n");
+	return ret;
+}
+
+void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl)
+{
+	struct dp_ctrl_private *ctrl;
+	struct dp_io *dp_io;
+	struct phy *phy;
+
+	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+	dp_io = &ctrl->parser->io;
+	phy = dp_io->phy;
+
+	dp_catalog_ctrl_reset(ctrl->catalog);
+
+	phy_exit(phy);
+
+	DRM_DEBUG_DP("DP off phy done\n");
+}
+
 int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
 {
 	struct dp_ctrl_private *ctrl;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index a836bd3..25e4f75 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -23,6 +23,8 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset);
 void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
+void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
 void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
 void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0ba71c7..c2b2050 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -570,6 +570,10 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 		dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
 	}
 
+	/* enable HDP irq_hpd/replug interrupt */
+	dp_catalog_hpd_config_intr(dp->catalog,
+		DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true);
+
 	mutex_unlock(&dp->event_mutex);
 
 	/* uevent will complete connection part */
@@ -619,7 +623,26 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	mutex_lock(&dp->event_mutex);
 
 	state = dp->hpd_state;
-	if (state == ST_DISCONNECT_PENDING || state == ST_DISCONNECTED) {
+
+	/* disable irq_hpd/replug interrupts */
+	dp_catalog_hpd_config_intr(dp->catalog,
+		DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, false);
+
+	/* unplugged, no more irq_hpd handle */
+	dp_del_event(dp, EV_IRQ_HPD_INT);
+
+	if (state == ST_DISCONNECTED) {
+		/* triggered by irq_hdp with sink_count = 0 */
+		if (dp->link->sink_count == 0) {
+			dp_ctrl_off_phy(dp->ctrl);
+			hpd->hpd_high = 0;
+			dp->core_initialized = false;
+		}
+		mutex_unlock(&dp->event_mutex);
+		return 0;
+	}
+
+	if (state == ST_DISCONNECT_PENDING) {
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -633,9 +656,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 
 	dp->hpd_state = ST_DISCONNECT_PENDING;
 
-	/* disable HPD plug interrupt until disconnect is done */
-	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK
-				| DP_DP_IRQ_HPD_INT_MASK, false);
+	/* disable HPD plug interrupts */
+	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
 
 	hpd->hpd_high = 0;
 
@@ -652,8 +674,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	reinit_completion(&dp->audio_comp);
 	dp_display_handle_plugged_change(g_dp_display, false);
 
-	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK |
-					DP_DP_IRQ_HPD_INT_MASK, true);
+	/* enable HDP plug interrupt to prepare for next plugin */
+	dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
 
 	/* uevent will complete disconnection part */
 	mutex_unlock(&dp->event_mutex);
@@ -684,7 +706,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
 
 	/* irq_hpd can happen at either connected or disconnected state */
 	state =  dp->hpd_state;
-	if (state == ST_DISPLAY_OFF) {
+	if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -903,9 +925,13 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
 
 	dp_display->audio_enabled = false;
 
-	dp_ctrl_off(dp->ctrl);
-
-	dp->core_initialized = false;
+	/* triggered by irq_hpd with sink_count = 0 */
+	if (dp->link->sink_count == 0) {
+		dp_ctrl_off_link_stream(dp->ctrl);
+	} else {
+		dp_ctrl_off(dp->ctrl);
+		dp->core_initialized = false;
+	}
 
 	dp_display->power_on = false;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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

which looks like the irq_hpd comes in while I'm disconnecting the HDMI
cable but the hpd_state is ST_DISCONNECTED. The state is set to
ST_DISCONNECTED in msm_dp_display_disable() so it seems that userspace
has turned off the external display, and then the kthread runs for the
irq_hpd but it's too late.

Something is missing from this patch then to properly disable the
IRQ_HPD interrupt before telling userspace that the external display is
disconnected. Shouldn't we be toggling the irq enable bits from the
hardirq context when we figure out what it is? The logic would be

 in_hardirq() {

   if (hpd high)
      enable_irq_hpd(); // Probably this can be delayed to the kthread
after enabling the link

   if (hpd_low)
      disable_irq_hpd(); // But this certainly cannot be in the kthread

   else if (irq_hpd) // Notice the else-if so that if hpd is low we
don't even try to handle irq_hpd if it came in at the same time
      handle_irq_hpd();
 }

Because we can't really mess with the irq controls in the kthread when
hpd goes low, it will be too late. For all we know, the kthread could
run seconds later, after an irq_hpd has come bouncing in at the same
time and pushed an irq_hpd handling event onto the kthread.

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

[khsieh]

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.

> which looks like the irq_hpd comes in while I'm disconnecting the HDMI
> cable but the hpd_state is ST_DISCONNECTED. The state is set to
> ST_DISCONNECTED in msm_dp_display_disable() so it seems that userspace
> has turned off the external display, and then the kthread runs for the
> irq_hpd but it's too late.
> 
> Something is missing from this patch then to properly disable the
> IRQ_HPD interrupt before telling userspace that the external display is
> disconnected. Shouldn't we be toggling the irq enable bits from the
> hardirq context when we figure out what it is? The logic would be
> 
>  in_hardirq() {
> 
>    if (hpd high)
>       enable_irq_hpd(); // Probably this can be delayed to the kthread
> after enabling the link
> 
>    if (hpd_low)
>       disable_irq_hpd(); // But this certainly cannot be in the kthread
> 
>    else if (irq_hpd) // Notice the else-if so that if hpd is low we
> don't even try to handle irq_hpd if it came in at the same time
>       handle_irq_hpd();
>  }
> 
> Because we can't really mess with the irq controls in the kthread when
> hpd goes low, it will be too late. For all we know, the kthread could
> run seconds later, after an irq_hpd has come bouncing in at the same
> time and pushed an irq_hpd handling event onto the kthread.

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

[Stephen Boyd]

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.

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

[khsieh]

On 2021-05-19 14:06, Stephen Boyd wrote:
> 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 
>> =display_disable
>> 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 
> thectrl_off_link_stream()).
> 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.

There is no race condition here.
The interrupts are converted into event and stored at event q.
event thread service event sequentially and make sure transaction had 
been completed before service next event.
The first irq_hpd with sink_count = 0 is handled and this transaction 
will not completed until user space frame work turn off display 
(msm_dp_display_disable()).
After that, the second irq_hpd with sink_count will be service which 
found that display is off so it spill out DP_LINK_STATUS_UPDATED warning 
message and do nothing.

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.

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

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-05-20 09:08:03)
> On 2021-05-19 14:06, Stephen Boyd wrote:
> > 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
> >> =display_disable
> >> 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
> > thectrl_off_link_stream()).
> > 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.
>
> There is no race condition here.
> The interrupts are converted into event and stored at event q.
> event thread service event sequentially and make sure transaction had
> been completed before service next event.
> The first irq_hpd with sink_count = 0 is handled and this transaction
> will not completed until user space frame work turn off display
> (msm_dp_display_disable()).
> After that, the second irq_hpd with sink_count will be service which
> found that display is off so it spill out DP_LINK_STATUS_UPDATED warning
> message and do nothing.
>
> 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.

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

[khsieh]

On 2021-05-20 12:28, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-05-20 09:08:03)
>> On 2021-05-19 14:06, Stephen Boyd wrote:
>> > 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
>> >> =display_disable
>> >> 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
>> > thectrl_off_link_stream()).
>> > 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.
>> 
>> There is no race condition here.
>> The interrupts are converted into event and stored at event q.
>> event thread service event sequentially and make sure transaction had
>> been completed before service next event.
>> The first irq_hpd with sink_count = 0 is handled and this transaction
>> will not completed until user space frame work turn off display
>> (msm_dp_display_disable()).
>> After that, the second irq_hpd with sink_count will be service which
>> found that display is off so it spill out DP_LINK_STATUS_UPDATED 
>> warning
>> message and do nothing.
>> 
>> 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.

I think you mistakenly think DP_LINK_STATUS_UPDATED is caused by unplug 
interrupt.
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.

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.

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

[Stephen Boyd]

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)?

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@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?

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

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-05-20 15:39:09)
> 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.

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.

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

[khsieh]

On 2021-05-20 22:00, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-05-20 15:39:09)
>> 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.
> 
> 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?
> 
yes,
we had a previous bug with this scenarios already.
https://partnerissuetracker.corp.google.com/issues/170598152
At this case, dongle produce two interrupts, irq_hpd followed by unplug 
immediately (not presented at isr status register at same time), at the 
time dongle unplugged form DTU.
But due to dp ctrl reset at handling irq_hpd which cause unplug mask bit 
be cleared so that unplug interrupt got lost.


I think V6 patch is good to go.


>> 
>> 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.

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

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-05-21 08:21:58)
> >
> > 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?
> >
> yes,
> we had a previous bug with this scenarios already.
> https://partnerissuetracker.corp.google.com/issues/170598152
> At this case, dongle produce two interrupts, irq_hpd followed by unplug
> immediately (not presented at isr status register at same time), at the
> time dongle unplugged form DTU.
> But due to dp ctrl reset at handling irq_hpd which cause unplug mask bit
> be cleared so that unplug interrupt got lost.
>

Again, wouldn't that be too late if the hardirq handler is delayed to
the point that the two irqs are pending in the isr status register?

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

[khsieh]

On 2021-05-21 12:18, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-05-21 08:21:58)
>> >
>> > 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?
>> >
>> yes,
>> we had a previous bug with this scenarios already.
>> https://partnerissuetracker.corp.google.com/issues/170598152
>> At this case, dongle produce two interrupts, irq_hpd followed by 
>> unplug
>> immediately (not presented at isr status register at same time), at 
>> the
>> time dongle unplugged form DTU.
>> But due to dp ctrl reset at handling irq_hpd which cause unplug mask 
>> bit
>> be cleared so that unplug interrupt got lost.
>> 
> 
> Again, wouldn't that be too late if the hardirq handler is delayed to
> the point that the two irqs are pending in the isr status register?

yes,
but that not much dp driver can do.
As long as DP driver can recovery (shut down gracefully) and ready for 
next plugin, then i think it should be fine.

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

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-05-21 12:45:35)
> On 2021-05-21 12:18, Stephen Boyd wrote:
> > Quoting khsieh@codeaurora.org (2021-05-21 08:21:58)
> >> >
> >> > 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?
> >> >
> >> yes,
> >> we had a previous bug with this scenarios already.
> >> https://partnerissuetracker.corp.google.com/issues/170598152
> >> At this case, dongle produce two interrupts, irq_hpd followed by
> >> unplug
> >> immediately (not presented at isr status register at same time), at
> >> the
> >> time dongle unplugged form DTU.
> >> But due to dp ctrl reset at handling irq_hpd which cause unplug mask
> >> bit
> >> be cleared so that unplug interrupt got lost.
> >>
> >
> > Again, wouldn't that be too late if the hardirq handler is delayed to
> > the point that the two irqs are pending in the isr status register?
>
> yes,
> but that not much dp driver can do.
> As long as DP driver can recovery (shut down gracefully) and ready for
> next plugin, then i think it should be fine.
>

Yes. I'm trying to make the point that the least the DP driver could do
would be to mask the interrupts for HPD IRQ when unplug irq happens at
hardirq context and also ignore HPD IRQ when unplug is also there. And
going even further, it feels like we should only enable the unplug irq
once a plug in irq happens, and vice versa, so that we don't get into
the case where a super long delayed irq sees all three bits: plug,
unplug, and hpd irq.

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

[khsieh]

On 2021-05-20 12:28, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-05-20 09:08:03)
>> On 2021-05-19 14:06, Stephen Boyd wrote:
>> > 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
>> >> =display_disable
>> >> 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
>> > thectrl_off_link_stream()).
>> > 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.
>> 
>> There is no race condition here.
>> The interrupts are converted into event and stored at event q.
>> event thread service event sequentially and make sure transaction had
>> been completed before service next event.
>> The first irq_hpd with sink_count = 0 is handled and this transaction
>> will not completed until user space frame work turn off display
>> (msm_dp_display_disable()).
>> After that, the second irq_hpd with sink_count will be service which
>> found that display is off so it spill out DP_LINK_STATUS_UPDATED 
>> warning
>> message and do nothing.
>> 
>> 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.

  [drm:dp_display_usbpd_attention_cb] *ERROR* Disconnected, no 
DP_LINK_STATUS_UPDATED <== you can see this warning message by just 
unplugged hdmi cable out of dongle (dongle still attached to DUT).