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(-)
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) {
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;
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;
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/2b5f09cadfc576817c0450e01d45...
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?
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).
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
- 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.
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
- 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.
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 */
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);
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);
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?
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
dri-devel@lists.freedesktop.org
-
khsieh＠codeaurora.org -
Kuogee Hsieh
-
Stephen Boyd