| Message ID | 1610051425-20632-3-git-send-email-khsieh@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/msm/dpu: consider vertical front porch in the prefill bw calculation | expand |
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
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 */
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(-)