[PATCH 0/2] *** fix missing unplug interrupt problem ***

[PATCH 0/2] *** fix missing unplug interrupt problem ***

[Kuogee Hsieh]

Both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts.
Therefore irq_hpd handler should not issues either aux or sw reset
to avoid following unplug interrupt be cleared accidentally.

Kuogee Hsieh (2):
  drm/msm/dp: postpone irq_hpd event during connection pending state
  drm/msm/dp: unplug interrupt missed after irq_hpd handler

 drivers/gpu/drm/msm/dp/dp_aux.c     |  7 -------
 drivers/gpu/drm/msm/dp/dp_catalog.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 15 ++++++++++-----
 drivers/gpu/drm/msm/dp/dp_display.c |  7 +++++++
 drivers/gpu/drm/msm/dp/dp_panel.c   | 12 +++++++++---
 5 files changed, 50 insertions(+), 15 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Kuogee Hsieh]

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

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) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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;

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

[khsieh]

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;

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

[Stephen Boyd]

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

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

[Stephen Boyd]

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?

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

[khsieh]

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

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

[Stephen Boyd]

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.

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

[khsieh]

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.

[PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[Kuogee Hsieh]

There is HPD unplug interrupts missed at scenario of an irq_hpd
followed by unplug interrupts with around 10 ms in between.
Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
irq_hpd handler should not issues either aux or sw reset to avoid
following unplug interrupt be cleared accidentally.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |  7 -------
 drivers/gpu/drm/msm/dp/dp_catalog.c | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 15 ++++++++++-----
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 19b35ae..1c6e1d2 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -336,7 +336,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	ssize_t ret;
 	int const aux_cmd_native_max = 16;
 	int const aux_cmd_i2c_max = 128;
-	int const retry_count = 5;
 	struct dp_aux_private *aux = container_of(dp_aux,
 		struct dp_aux_private, dp_aux);
 
@@ -378,12 +377,6 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	ret = dp_aux_cmd_fifo_tx(aux, msg);
 
 	if (ret < 0) {
-		if (aux->native) {
-			aux->retry_cnt++;
-			if (!(aux->retry_cnt % retry_count))
-				dp_catalog_aux_update_cfg(aux->catalog);
-			dp_catalog_aux_reset(aux->catalog);
-		}
 		usleep_range(400, 500); /* at least 400us to next try */
 		goto unlock_exit;
 	}
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 44f0c57..9c0ce98 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog)
 	return 0;
 }
 
+/**
+ * dp_catalog_aux_reset() - reset AUX controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset AUX controller
+ *
+ * NOTE: reset AUX controller will also clear any pending HPD related interrupts
+ * 
+ */
 void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
 {
 	u32 aux_ctrl;
@@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog *dp_catalog,
 	return 0;
 }
 
+/**
+ * dp_catalog_ctrl_reset() - reset DP controller
+ *
+ * @aux: DP catalog structure
+ *
+ * return: void
+ *
+ * This function reset DP controller
+ *
+ * NOTE: reset DP controller will also clear any pending HPD related interrupts
+ * 
+ */
 void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
 {
 	u32 sw_reset;
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e3462f5..f96c415 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
 	 * transitioned to PUSH_IDLE. In order to start transmitting
 	 * a link training pattern, we have to first do soft reset.
 	 */
-	dp_catalog_ctrl_reset(ctrl->catalog);
+	if (*training_step != DP_TRAINING_NONE)
+		dp_catalog_ctrl_reset(ctrl->catalog);
 
 	ret = dp_ctrl_link_train(ctrl, cr, training_step);
 
@@ -1491,15 +1492,18 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
 	return 0;
 }
 
+static void dp_ctrl_link_idle_reset(struct dp_ctrl_private *ctrl)
+{
+	dp_ctrl_push_idle(&ctrl->dp_ctrl);
+	dp_catalog_ctrl_reset(ctrl->catalog);
+}
+
 static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 {
 	int ret = 0;
 	struct dp_cr_status cr;
 	int training_step = DP_TRAINING_NONE;
 
-	dp_ctrl_push_idle(&ctrl->dp_ctrl);
-	dp_catalog_ctrl_reset(ctrl->catalog);
-
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
 	ret = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
@@ -1626,6 +1630,7 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
 
 	if (sink_request & DP_TEST_LINK_TRAINING) {
 		dp_link_send_test_response(ctrl->link);
+		dp_ctrl_link_idle_reset(ctrl);
 		if (dp_ctrl_link_maintenance(ctrl)) {
 			DRM_ERROR("LM failed: TEST_LINK_TRAINING\n");
 			return;
@@ -1679,7 +1684,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 			break;
 		}
 
-		training_step = DP_TRAINING_NONE;
+		training_step = DP_TRAINING_1;
 		rc = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
 		if (rc == 0) {
 			/* training completed successfully */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-01-07 12:30:25)
> There is HPD unplug interrupts missed at scenario of an irq_hpd
> followed by unplug interrupts with around 10 ms in between.
> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
> irq_hpd handler should not issues either aux or sw reset to avoid
> following unplug interrupt be cleared accidentally.

So the problem is that we're resetting the DP aux phy in the middle of
the HPD state machine transitioning states?

> 
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 44f0c57..9c0ce98 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog)
>         return 0;
>  }
>  
> +/**
> + * dp_catalog_aux_reset() - reset AUX controller
> + *
> + * @aux: DP catalog structure
> + *
> + * return: void
> + *
> + * This function reset AUX controller
> + *
> + * NOTE: reset AUX controller will also clear any pending HPD related interrupts
> + * 
> + */
>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
>  {
>         u32 aux_ctrl;
> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog *dp_catalog,
>         return 0;
>  }
>  
> +/**
> + * dp_catalog_ctrl_reset() - reset DP controller
> + *
> + * @aux: DP catalog structure

It's called dp_catalog though.

> + *
> + * return: void
> + *
> + * This function reset DP controller

resets the

> + *
> + * NOTE: reset DP controller will also clear any pending HPD related interrupts
> + * 
> + */
>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
>  {
>         u32 sw_reset;
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e3462f5..f96c415 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
>          * transitioned to PUSH_IDLE. In order to start transmitting
>          * a link training pattern, we have to first do soft reset.
>          */
> -       dp_catalog_ctrl_reset(ctrl->catalog);
> +       if (*training_step != DP_TRAINING_NONE)

Can we check for the positive value instead? i.e.
DP_TRAINING_1/DP_TRAINING_2

> +               dp_catalog_ctrl_reset(ctrl->catalog);
>  
>         ret = dp_ctrl_link_train(ctrl, cr, training_step);
>

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[khsieh]

On 2021-01-11 11:54, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-01-07 12:30:25)
>> There is HPD unplug interrupts missed at scenario of an irq_hpd
>> followed by unplug interrupts with around 10 ms in between.
>> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
>> irq_hpd handler should not issues either aux or sw reset to avoid
>> following unplug interrupt be cleared accidentally.
> 
> So the problem is that we're resetting the DP aux phy in the middle of
> the HPD state machine transitioning states?
> 
yes, after reset aux, hw clear pending hpd interrupts
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 44f0c57..9c0ce98 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct 
>> dp_catalog *dp_catalog)
>>         return 0;
>>  }
>> 
>> +/**
>> + * dp_catalog_aux_reset() - reset AUX controller
>> + *
>> + * @aux: DP catalog structure
>> + *
>> + * return: void
>> + *
>> + * This function reset AUX controller
>> + *
>> + * NOTE: reset AUX controller will also clear any pending HPD related 
>> interrupts
>> + *
>> + */
>>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
>>  {
>>         u32 aux_ctrl;
>> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog 
>> *dp_catalog,
>>         return 0;
>>  }
>> 
>> +/**
>> + * dp_catalog_ctrl_reset() - reset DP controller
>> + *
>> + * @aux: DP catalog structure
> 
> It's called dp_catalog though.
registers access are through dp_catalog_xxxx
> 
>> + *
>> + * return: void
>> + *
>> + * This function reset DP controller
> 
> resets the
> 
>> + *
>> + * NOTE: reset DP controller will also clear any pending HPD related 
>> interrupts
>> + *
>> + */
>>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
>>  {
>>         u32 sw_reset;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index e3462f5..f96c415 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct 
>> dp_ctrl_private *ctrl,
>>          * transitioned to PUSH_IDLE. In order to start transmitting
>>          * a link training pattern, we have to first do soft reset.
>>          */
>> -       dp_catalog_ctrl_reset(ctrl->catalog);
>> +       if (*training_step != DP_TRAINING_NONE)
> 
> Can we check for the positive value instead? i.e.
> DP_TRAINING_1/DP_TRAINING_2
> 
>> +               dp_catalog_ctrl_reset(ctrl->catalog);
>> 
>>         ret = dp_ctrl_link_train(ctrl, cr, training_step);
>> 

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-01-13 09:48:25)
> On 2021-01-11 11:54, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-01-07 12:30:25)
> >> There is HPD unplug interrupts missed at scenario of an irq_hpd
> >> followed by unplug interrupts with around 10 ms in between.
> >> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
> >> irq_hpd handler should not issues either aux or sw reset to avoid
> >> following unplug interrupt be cleared accidentally.
> > 
> > So the problem is that we're resetting the DP aux phy in the middle of
> > the HPD state machine transitioning states?
> > 
> yes, after reset aux, hw clear pending hpd interrupts
> >> 
> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> >> ---
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> index 44f0c57..9c0ce98 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct 
> >> dp_catalog *dp_catalog)
> >>         return 0;
> >>  }
> >> 
> >> +/**
> >> + * dp_catalog_aux_reset() - reset AUX controller
> >> + *
> >> + * @aux: DP catalog structure
> >> + *
> >> + * return: void
> >> + *
> >> + * This function reset AUX controller
> >> + *
> >> + * NOTE: reset AUX controller will also clear any pending HPD related 
> >> interrupts
> >> + *
> >> + */
> >>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
> >>  {
> >>         u32 aux_ctrl;
> >> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog 
> >> *dp_catalog,
> >>         return 0;
> >>  }
> >> 
> >> +/**
> >> + * dp_catalog_ctrl_reset() - reset DP controller
> >> + *
> >> + * @aux: DP catalog structure
> > 
> > It's called dp_catalog though.
> registers access are through dp_catalog_xxxx

Agreed. The variable is not called 'aux' though, it's called
'dp_catalog'.

> > 
> >> + *
> >> + * return: void
> >> + *
> >> + * This function reset DP controller
> > 
> > resets the
> > 
> >> + *
> >> + * NOTE: reset DP controller will also clear any pending HPD related 
> >> interrupts
> >> + *
> >> + */
> >>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)

Right here.

> >>  {
> >>         u32 sw_reset;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
> >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> index e3462f5..f96c415 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> >> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct 
> >> dp_ctrl_private *ctrl,
> >>          * transitioned to PUSH_IDLE. In order to start transmitting
> >>          * a link training pattern, we have to first do soft reset.
> >>          */
> >> -       dp_catalog_ctrl_reset(ctrl->catalog);
> >> +       if (*training_step != DP_TRAINING_NONE)
> > 
> > Can we check for the positive value instead? i.e.
> > DP_TRAINING_1/DP_TRAINING_2
> > 

Any answer?

Re: [PATCH 2/2] drm/msm/dp: unplug interrupt missed after irq_hpd handler

[khsieh]

On 2021-01-13 12:23, Stephen Boyd wrote:
> Quoting khsieh@codeaurora.org (2021-01-13 09:48:25)
>> On 2021-01-11 11:54, Stephen Boyd wrote:
>> > Quoting Kuogee Hsieh (2021-01-07 12:30:25)
>> >> There is HPD unplug interrupts missed at scenario of an irq_hpd
>> >> followed by unplug interrupts with around 10 ms in between.
>> >> Since both AUX_SW_RESET and DP_SW_RESET clear pending HPD interrupts,
>> >> irq_hpd handler should not issues either aux or sw reset to avoid
>> >> following unplug interrupt be cleared accidentally.
>> >
>> > So the problem is that we're resetting the DP aux phy in the middle of
>> > the HPD state machine transitioning states?
>> >
>> yes, after reset aux, hw clear pending hpd interrupts
>> >>
>> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> >> ---
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> >> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> >> index 44f0c57..9c0ce98 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> >> @@ -190,6 +190,18 @@ int dp_catalog_aux_clear_hw_interrupts(struct
>> >> dp_catalog *dp_catalog)
>> >>         return 0;
>> >>  }
>> >>
>> >> +/**
>> >> + * dp_catalog_aux_reset() - reset AUX controller
>> >> + *
>> >> + * @aux: DP catalog structure
>> >> + *
>> >> + * return: void
>> >> + *
>> >> + * This function reset AUX controller
>> >> + *
>> >> + * NOTE: reset AUX controller will also clear any pending HPD related
>> >> interrupts
>> >> + *
>> >> + */
>> >>  void dp_catalog_aux_reset(struct dp_catalog *dp_catalog)
>> >>  {
>> >>         u32 aux_ctrl;
>> >> @@ -483,6 +495,18 @@ int dp_catalog_ctrl_set_pattern(struct dp_catalog
>> >> *dp_catalog,
>> >>         return 0;
>> >>  }
>> >>
>> >> +/**
>> >> + * dp_catalog_ctrl_reset() - reset DP controller
>> >> + *
>> >> + * @aux: DP catalog structure
>> >
>> > It's called dp_catalog though.
>> registers access are through dp_catalog_xxxx
> 
> Agreed. The variable is not called 'aux' though, it's called
> 'dp_catalog'.
> 
>> >
>> >> + *
>> >> + * return: void
>> >> + *
>> >> + * This function reset DP controller
>> >
>> > resets the
>> >
>> >> + *
>> >> + * NOTE: reset DP controller will also clear any pending HPD related
>> >> interrupts
>> >> + *
>> >> + */
>> >>  void dp_catalog_ctrl_reset(struct dp_catalog *dp_catalog)
> 
> Right here.

fixed at v2
> 
>> >>  {
>> >>         u32 sw_reset;
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> >> index e3462f5..f96c415 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> >> @@ -1296,7 +1296,8 @@ static int dp_ctrl_setup_main_link(struct
>> >> dp_ctrl_private *ctrl,
>> >>          * transitioned to PUSH_IDLE. In order to start transmitting
>> >>          * a link training pattern, we have to first do soft reset.
>> >>          */
>> >> -       dp_catalog_ctrl_reset(ctrl->catalog);
>> >> +       if (*training_step != DP_TRAINING_NONE)
>> >
>> > Can we check for the positive value instead? i.e.
>> > DP_TRAINING_1/DP_TRAINING_2
>> >
> 
> Any answer?

fixed at v2