diff mbox series

[1/2] drm/msm/dp: postpone irq_hpd event during connection pending state

Message ID 1610051425-20632-2-git-send-email-khsieh@codeaurora.org
State Accepted
Commit 2b5f09cadfc576817c0450e01d454f750909b103
Headers show
Series *** fix missing unplug interrupt problem *** | expand

Commit Message

Kuogee Hsieh Jan. 7, 2021, 8:30 p.m. UTC
irq_hpd event can only be executed at connected state. Therefore
irq_hpd event should be postponed if it happened at connection
pending state. This patch also make sure both link rate and lane
are valid before start link training.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c |  7 +++++++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Stephen Boyd Jan. 11, 2021, 7:55 p.m. UTC | #1
Quoting Kuogee Hsieh (2021-01-07 12:30:24)
> irq_hpd event can only be executed at connected state. Therefore

> irq_hpd event should be postponed if it happened at connection

> pending state. This patch also make sure both link rate and lane


Why does it happen at connection pending state?

> are valid before start link training.


Can this part about link rate and lane being valid be split off into
another patch?

> 

> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>

> ---


Any fixes tag?

>  drivers/gpu/drm/msm/dp/dp_display.c |  7 +++++++

>  drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +++++++++---

>  2 files changed, 16 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c

> index 6e971d5..3bc7ed2 100644

> --- a/drivers/gpu/drm/msm/dp/dp_display.c

> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

> @@ -693,6 +693,13 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)

>                 return 0;

>         }

>  

> +       if (state == ST_CONNECT_PENDING) {

> +               /* wait until ST_CONNECTED */

> +               dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */

> +               mutex_unlock(&dp->event_mutex);

> +               return 0;

> +       }

> +

>         ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);

>         if (ret == -ECONNRESET) { /* cable unplugged */

>                 dp->core_initialized = false;
Kuogee Hsieh Jan. 13, 2021, 5:44 p.m. UTC | #2
On 2021-01-11 11:55, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-01-07 12:30:24)

>> irq_hpd event can only be executed at connected state. Therefore

>> irq_hpd event should be postponed if it happened at connection

>> pending state. This patch also make sure both link rate and lane

> 

> Why does it happen at connection pending state?

plug in need two state to complete it.
advance to connection pending state once link training completed and 
sent uevent notification to frame work.
transition to connected state after frame work provided resolution 
timing and start transmit video panel.
Therefore irq_hpd should not be handled if it occurred before connected 
state.
> 

>> are valid before start link training.

> 

> Can this part about link rate and lane being valid be split off into

> another patch?

> 

ok, i will spilt this patch into two.
I will merge irq_hpd event part into 2nd patch (drm/msm/dp: unplug 
interrupt missed after irq_hpd handler).

>> 

>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>

>> ---

> 

> Any fixes tag?

> 

>>  drivers/gpu/drm/msm/dp/dp_display.c |  7 +++++++

>>  drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +++++++++---

>>  2 files changed, 16 insertions(+), 3 deletions(-)

>> 

>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 

>> b/drivers/gpu/drm/msm/dp/dp_display.c

>> index 6e971d5..3bc7ed2 100644

>> --- a/drivers/gpu/drm/msm/dp/dp_display.c

>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

>> @@ -693,6 +693,13 @@ static int dp_irq_hpd_handle(struct 

>> dp_display_private *dp, u32 data)

>>                 return 0;

>>         }

>> 

>> +       if (state == ST_CONNECT_PENDING) {

>> +               /* wait until ST_CONNECTED */

>> +               dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 

>> */

>> +               mutex_unlock(&dp->event_mutex);

>> +               return 0;

>> +       }

>> +

>>         ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);

>>         if (ret == -ECONNRESET) { /* cable unplugged */

>>                 dp->core_initialized = false;
Stephen Boyd Jan. 13, 2021, 8:21 p.m. UTC | #3
Quoting khsieh@codeaurora.org (2021-01-13 09:44:24)
> On 2021-01-11 11:55, Stephen Boyd wrote:

> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)

> >> irq_hpd event can only be executed at connected state. Therefore

> >> irq_hpd event should be postponed if it happened at connection

> >> pending state. This patch also make sure both link rate and lane

> > 

> > Why does it happen at connection pending state?

> plug in need two state to complete it.

> advance to connection pending state once link training completed and 

> sent uevent notification to frame work.

> transition to connected state after frame work provided resolution 

> timing and start transmit video panel.

> Therefore irq_hpd should not be handled if it occurred before connected 

> state.

> > 

> >> are valid before start link training.

> > 

> > Can this part about link rate and lane being valid be split off into

> > another patch?

> > 

> ok, i will spilt this patch into two.

> I will merge irq_hpd event part into 2nd patch (drm/msm/dp: unplug 

> interrupt missed after irq_hpd handler).


It looks like Rob already picked this patch up

https://gitlab.freedesktop.org/drm/msm/-/commit/2b5f09cadfc576817c0450e01d454f750909b103
Stephen Boyd Jan. 13, 2021, 8:22 p.m. UTC | #4
Quoting khsieh@codeaurora.org (2021-01-13 09:44:24)
> On 2021-01-11 11:55, Stephen Boyd wrote:

> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)

> >> irq_hpd event can only be executed at connected state. Therefore

> >> irq_hpd event should be postponed if it happened at connection

> >> pending state. This patch also make sure both link rate and lane

> > 

> > Why does it happen at connection pending state?

> plug in need two state to complete it.

> advance to connection pending state once link training completed and 

> sent uevent notification to frame work.

> transition to connected state after frame work provided resolution 

> timing and start transmit video panel.

> Therefore irq_hpd should not be handled if it occurred before connected 

> state.


Sure that's what's going on in the patch but you didn't answer my
question. Why does irq_hpd happen before connected state?
Kuogee Hsieh Jan. 13, 2021, 11:44 p.m. UTC | #5
On 2021-01-13 12:22, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-01-13 09:44:24)

>> On 2021-01-11 11:55, Stephen Boyd wrote:

>> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)

>> >> irq_hpd event can only be executed at connected state. Therefore

>> >> irq_hpd event should be postponed if it happened at connection

>> >> pending state. This patch also make sure both link rate and lane

>> >

>> > Why does it happen at connection pending state?

>> plug in need two state to complete it.

>> advance to connection pending state once link training completed and

>> sent uevent notification to frame work.

>> transition to connected state after frame work provided resolution

>> timing and start transmit video panel.

>> Therefore irq_hpd should not be handled if it occurred before 

>> connected

>> state.

> 

> Sure that's what's going on in the patch but you didn't answer my

> question. Why does irq_hpd happen before connected state?


I have no idea why it happen this way.
during debug 
https://partnerissuetracker.corp.google.com/issues/170598152
I saw two different scenario
1) irq_hpd followed by unplug with less than 20 ms in between. this one 
fixed by this patch set.
2) plug followed by irq_hpd around 300ms in between. it does not cause 
problem. but it should be handled in order (after connected state).
Stephen Boyd Jan. 14, 2021, midnight UTC | #6
Quoting khsieh@codeaurora.org (2021-01-13 15:44:32)
> On 2021-01-13 12:22, Stephen Boyd wrote:

> > Quoting khsieh@codeaurora.org (2021-01-13 09:44:24)

> >> On 2021-01-11 11:55, Stephen Boyd wrote:

> >> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)

> >> >> irq_hpd event can only be executed at connected state. Therefore

> >> >> irq_hpd event should be postponed if it happened at connection

> >> >> pending state. This patch also make sure both link rate and lane

> >> >

> >> > Why does it happen at connection pending state?

> >> plug in need two state to complete it.

> >> advance to connection pending state once link training completed and

> >> sent uevent notification to frame work.

> >> transition to connected state after frame work provided resolution

> >> timing and start transmit video panel.

> >> Therefore irq_hpd should not be handled if it occurred before 

> >> connected

> >> state.

> > 

> > Sure that's what's going on in the patch but you didn't answer my

> > question. Why does irq_hpd happen before connected state?

> 

> I have no idea why it happen this way.

> during debug 

> https://partnerissuetracker.corp.google.com/issues/170598152

> I saw two different scenario

> 1) irq_hpd followed by unplug with less than 20 ms in between. this one 

> fixed by this patch set.

> 2) plug followed by irq_hpd around 300ms in between. it does not cause 

> problem. but it should be handled in order (after connected state).


Ok. So nobody understands why the hardware is acting this way and we're
papering over the problem by forcing the HPD state to be what we think
it should be? That's not great.
Kuogee Hsieh Jan. 14, 2021, 5:13 p.m. UTC | #7
On 2021-01-13 16:00, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-01-13 15:44:32)

>> On 2021-01-13 12:22, Stephen Boyd wrote:

>> > Quoting khsieh@codeaurora.org (2021-01-13 09:44:24)

>> >> On 2021-01-11 11:55, Stephen Boyd wrote:

>> >> > Quoting Kuogee Hsieh (2021-01-07 12:30:24)

>> >> >> irq_hpd event can only be executed at connected state. Therefore

>> >> >> irq_hpd event should be postponed if it happened at connection

>> >> >> pending state. This patch also make sure both link rate and lane

>> >> >

>> >> > Why does it happen at connection pending state?

>> >> plug in need two state to complete it.

>> >> advance to connection pending state once link training completed and

>> >> sent uevent notification to frame work.

>> >> transition to connected state after frame work provided resolution

>> >> timing and start transmit video panel.

>> >> Therefore irq_hpd should not be handled if it occurred before

>> >> connected

>> >> state.

>> >

>> > Sure that's what's going on in the patch but you didn't answer my

>> > question. Why does irq_hpd happen before connected state?

>> 

>> I have no idea why it happen this way.

>> during debug

>> https://partnerissuetracker.corp.google.com/issues/170598152

>> I saw two different scenario

>> 1) irq_hpd followed by unplug with less than 20 ms in between. this 

>> one

>> fixed by this patch set.

>> 2) plug followed by irq_hpd around 300ms in between. it does not cause

>> problem. but it should be handled in order (after connected state).

> 

> Ok. So nobody understands why the hardware is acting this way and we're

> papering over the problem by forcing the HPD state to be what we think

> it should be? That's not great.


irq_hpd is issued from dongle.
it then go through EC ps8805 driver and reach DP driver finally.
Again, to duplicate problem #1 this at my set up, i have to 
intentionally wiggling type-c connector of dongle.
But I can not duplicate problem #2 and only saw it one time from Quantan 
provide logs.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 6e971d5..3bc7ed2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -693,6 +693,13 @@  static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
 		return 0;
 	}
 
+	if (state == ST_CONNECT_PENDING) {
+		/* wait until ST_CONNECTED */
+		dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */
+		mutex_unlock(&dp->event_mutex);
+		return 0;
+	}
+
 	ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
 	if (ret == -ECONNRESET) { /* cable unplugged */
 		dp->core_initialized = false;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 97dca3e..d1780bc 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -167,12 +167,18 @@  int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
 	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
 
 	rc = dp_panel_read_dpcd(dp_panel);
+	if (rc) {
+		DRM_ERROR("read dpcd failed %d\n", rc);
+		return rc;
+	}
+
 	bw_code = drm_dp_link_rate_to_bw_code(dp_panel->link_info.rate);
-	if (rc || !is_link_rate_valid(bw_code) ||
+	if (!is_link_rate_valid(bw_code) ||
 			!is_lane_count_valid(dp_panel->link_info.num_lanes) ||
 			(bw_code > dp_panel->max_bw_code)) {
-		DRM_ERROR("read dpcd failed %d\n", rc);
-		return rc;
+		DRM_ERROR("Illegal link rate=%d lane=%d\n", dp_panel->link_info.rate,
+				dp_panel->link_info.num_lanes);
+		return -EINVAL;
 	}
 
 	if (dp_panel->dfp_present) {