| Message ID | 1655240702-12230-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] drm/msm/dp: force link training for display resolution change | expand |
Quoting Kuogee Hsieh (2022-06-14 14:05:02) > Display resolution change is implemented through drm modeset. Older > modeset (resolution) has to be disabled first before newer modeset > (resolution) can be enabled. Display disable will turn off both > pixel clock and main link clock so that main link have to be > re-trained during display enable to have new video stream flow > again. At current implementation, display enable function manually > kicks up irq_hpd_handle which will read panel link status and start > link training if link status is not in sync state. > > However, there is rare case that a particular panel links status keep > staying in sync for some period of time after main link had been shut > down previously at display disabled. In this case, main link retraining > will not be executed by irq_hdp_handle(). Hence video stream of newer > display resolution will fail to be transmitted to panel due to main > link is not in sync between host and panel. > > This patch will bypass irq_hpd_hanle() in favor of directly call s/hanle/handle/ > dp_ctrl_on_stream() to always perform link training in regardless of > main link status. So that no unexpected exception resolution change > failure cases will happen. Also this implementation are more efficient > than manual kicking off irq_hpd_handle function. > > Changes in v2: > -- set force_link_train flag on DP only (is_edp == false) > > Changes in v3: > -- revise commit text > -- add Fixes tag > > Changes in v4: > -- revise commit text > > Changes in v5: > -- fix spelling at commit text > > Changes in v6: > -- split dp_ctrl_on_stream() for phy test case > -- revise commit text for modeset > > Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 31 +++++++++++++++++++++++-------- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 ++- > drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++------- > 3 files changed, 31 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index af7a80c..cb9c7af 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1807,7 +1807,27 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl) > return dp_ctrl_setup_main_link(ctrl, &training_step); > } > > -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) > +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl) > +{ > + int ret = 0; Drop assignment please. > + struct dp_ctrl_private *ctrl; > + > + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); > + > + ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock; > + > + ret = dp_ctrl_enable_stream_clocks(ctrl); > + if (ret) { > + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret); > + return ret; > + } > + > + dp_ctrl_send_phy_test_pattern(ctrl); None of this code needs to be run in the normal display on case? > + > + return 0; > +} > + > +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train) > { > int ret = 0; > bool mainlink_ready = false; > @@ -1843,12 +1863,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) > goto end; > } > > - if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) { > - dp_ctrl_send_phy_test_pattern(ctrl); > - return 0; > - } > - > - if (!dp_ctrl_channel_eq_ok(ctrl)) > + if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl)) > dp_ctrl_link_retrain(ctrl); > > /* stop txing train pattern to end link training */ > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index c388323..b6d25ab 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1688,10 +1689,12 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) > > state = dp_display->hpd_state; > > - if (state == ST_DISPLAY_OFF) > + if (state == ST_DISPLAY_OFF) { > dp_display_host_phy_init(dp_display); > + force_link_train = true; > + } > > - dp_display_enable(dp_display, 0); > + dp_display_enable(dp_display, force_link_train); Do we need to pass it from here? Why can't dp_display_enable() simply check for 'state == ST_DISPLAY_OFF' and then force retrain the link?
On 6/14/2022 2:59 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-06-14 14:05:02) >> Display resolution change is implemented through drm modeset. Older >> modeset (resolution) has to be disabled first before newer modeset >> (resolution) can be enabled. Display disable will turn off both >> pixel clock and main link clock so that main link have to be >> re-trained during display enable to have new video stream flow >> again. At current implementation, display enable function manually >> kicks up irq_hpd_handle which will read panel link status and start >> link training if link status is not in sync state. >> >> However, there is rare case that a particular panel links status keep >> staying in sync for some period of time after main link had been shut >> down previously at display disabled. In this case, main link retraining >> will not be executed by irq_hdp_handle(). Hence video stream of newer >> display resolution will fail to be transmitted to panel due to main >> link is not in sync between host and panel. >> >> This patch will bypass irq_hpd_hanle() in favor of directly call > s/hanle/handle/ > >> dp_ctrl_on_stream() to always perform link training in regardless of >> main link status. So that no unexpected exception resolution change >> failure cases will happen. Also this implementation are more efficient >> than manual kicking off irq_hpd_handle function. >> >> Changes in v2: >> -- set force_link_train flag on DP only (is_edp == false) >> >> Changes in v3: >> -- revise commit text >> -- add Fixes tag >> >> Changes in v4: >> -- revise commit text >> >> Changes in v5: >> -- fix spelling at commit text >> >> Changes in v6: >> -- split dp_ctrl_on_stream() for phy test case >> -- revise commit text for modeset >> >> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 31 +++++++++++++++++++++++-------- >> drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 ++- >> drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++------- >> 3 files changed, 31 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index af7a80c..cb9c7af 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1807,7 +1807,27 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl) >> return dp_ctrl_setup_main_link(ctrl, &training_step); >> } >> >> -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl) >> +{ >> + int ret = 0; > Drop assignment please. > >> + struct dp_ctrl_private *ctrl; >> + >> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); >> + >> + ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock; >> + >> + ret = dp_ctrl_enable_stream_clocks(ctrl); >> + if (ret) { >> + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret); >> + return ret; >> + } >> + >> + dp_ctrl_send_phy_test_pattern(ctrl); > None of this code needs to be run in the normal display on case? + ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock; + + ret = dp_ctrl_enable_stream_clocks(ctrl); These two lines are also used at normal display on case (dp_ctrl_on_stream()). I have to copy them to here to form a stand alone dp_ctrl_on_stream_phy_test_report(). >> + >> + return 0; >> +} >> + >> +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train) >> { >> int ret = 0; >> bool mainlink_ready = false; >> @@ -1843,12 +1863,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> goto end; >> } >> >> - if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) { >> - dp_ctrl_send_phy_test_pattern(ctrl); >> - return 0; >> - } >> - >> - if (!dp_ctrl_channel_eq_ok(ctrl)) >> + if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl)) >> dp_ctrl_link_retrain(ctrl); >> >> /* stop txing train pattern to end link training */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index c388323..b6d25ab 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1688,10 +1689,12 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) >> >> state = dp_display->hpd_state; >> >> - if (state == ST_DISPLAY_OFF) >> + if (state == ST_DISPLAY_OFF) { >> dp_display_host_phy_init(dp_display); >> + force_link_train = true; >> + } >> >> - dp_display_enable(dp_display, 0); >> + dp_display_enable(dp_display, force_link_train); > Do we need to pass it from here? Why can't dp_display_enable() simply > check for 'state == ST_DISPLAY_OFF' and then force retrain the link?
On 6/14/2022 2:59 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-06-14 14:05:02) >> Display resolution change is implemented through drm modeset. Older >> modeset (resolution) has to be disabled first before newer modeset >> (resolution) can be enabled. Display disable will turn off both >> pixel clock and main link clock so that main link have to be >> re-trained during display enable to have new video stream flow >> again. At current implementation, display enable function manually >> kicks up irq_hpd_handle which will read panel link status and start >> link training if link status is not in sync state. >> >> However, there is rare case that a particular panel links status keep >> staying in sync for some period of time after main link had been shut >> down previously at display disabled. In this case, main link retraining >> will not be executed by irq_hdp_handle(). Hence video stream of newer >> display resolution will fail to be transmitted to panel due to main >> link is not in sync between host and panel. >> >> This patch will bypass irq_hpd_hanle() in favor of directly call > s/hanle/handle/ > >> dp_ctrl_on_stream() to always perform link training in regardless of >> main link status. So that no unexpected exception resolution change >> failure cases will happen. Also this implementation are more efficient >> than manual kicking off irq_hpd_handle function. >> >> Changes in v2: >> -- set force_link_train flag on DP only (is_edp == false) >> >> Changes in v3: >> -- revise commit text >> -- add Fixes tag >> >> Changes in v4: >> -- revise commit text >> >> Changes in v5: >> -- fix spelling at commit text >> >> Changes in v6: >> -- split dp_ctrl_on_stream() for phy test case >> -- revise commit text for modeset >> >> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 31 +++++++++++++++++++++++-------- >> drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 ++- >> drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++------- >> 3 files changed, 31 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index af7a80c..cb9c7af 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1807,7 +1807,27 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl) >> return dp_ctrl_setup_main_link(ctrl, &training_step); >> } >> >> -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl) >> +{ >> + int ret = 0; > Drop assignment please. > >> + struct dp_ctrl_private *ctrl; >> + >> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); >> + >> + ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock; >> + >> + ret = dp_ctrl_enable_stream_clocks(ctrl); >> + if (ret) { >> + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret); >> + return ret; >> + } >> + >> + dp_ctrl_send_phy_test_pattern(ctrl); > None of this code needs to be run in the normal display on case? > >> + >> + return 0; >> +} >> + >> +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train) >> { >> int ret = 0; >> bool mainlink_ready = false; >> @@ -1843,12 +1863,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> goto end; >> } >> >> - if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) { >> - dp_ctrl_send_phy_test_pattern(ctrl); >> - return 0; >> - } >> - >> - if (!dp_ctrl_channel_eq_ok(ctrl)) >> + if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl)) >> dp_ctrl_link_retrain(ctrl); >> >> /* stop txing train pattern to end link training */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index c388323..b6d25ab 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1688,10 +1689,12 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) >> >> state = dp_display->hpd_state; >> >> - if (state == ST_DISPLAY_OFF) >> + if (state == ST_DISPLAY_OFF) { >> dp_display_host_phy_init(dp_display); >> + force_link_train = true; >> + } >> >> - dp_display_enable(dp_display, 0); >> + dp_display_enable(dp_display, force_link_train); > Do we need to pass it from here? Why can't dp_display_enable() simply > check for 'state == ST_DISPLAY_OFF' and then force retrain the link? can we keep this as it is? it is more readable that we do need force link re-training due to ST_DISPLAY_OFF at top level. Also we only need to do (state == ST_DISPLAY_OFF) checking one time.
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c index af7a80c..cb9c7af 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl) ret = dp_ctrl_on_link(&ctrl->dp_ctrl); if (!ret) - ret = dp_ctrl_on_stream(&ctrl->dp_ctrl); + ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl); else DRM_ERROR("failed to enable DP link controller\n"); @@ -1807,7 +1807,27 @@ static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl) return dp_ctrl_setup_main_link(ctrl, &training_step); } -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl) +{ + int ret = 0; + struct dp_ctrl_private *ctrl; + + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); + + ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock; + + ret = dp_ctrl_enable_stream_clocks(ctrl); + if (ret) { + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret); + return ret; + } + + dp_ctrl_send_phy_test_pattern(ctrl); + + return 0; +} + +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train) { int ret = 0; bool mainlink_ready = false; @@ -1843,12 +1863,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) goto end; } - if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) { - dp_ctrl_send_phy_test_pattern(ctrl); - return 0; - } - - if (!dp_ctrl_channel_eq_ok(ctrl)) + if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl)) dp_ctrl_link_retrain(ctrl); /* stop txing train pattern to end link training */ diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h index 0745fde..9a39b00 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h @@ -21,7 +21,8 @@ struct 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_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train); +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl); int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl); int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl); int dp_ctrl_off(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 c388323..b6d25ab 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data) return 0; } - rc = dp_ctrl_on_stream(dp->ctrl); + rc = dp_ctrl_on_stream(dp->ctrl, data); if (!rc) dp_display->power_on = true; @@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) int rc = 0; struct dp_display_private *dp_display; u32 state; + bool force_link_train = false; dp_display = container_of(dp, struct dp_display_private, dp_display); if (!dp_display->dp_mode.drm_mode.clock) { @@ -1688,10 +1689,12 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) state = dp_display->hpd_state; - if (state == ST_DISPLAY_OFF) + if (state == ST_DISPLAY_OFF) { dp_display_host_phy_init(dp_display); + force_link_train = true; + } - dp_display_enable(dp_display, 0); + dp_display_enable(dp_display, force_link_train); rc = dp_display_post_enable(dp); if (rc) { @@ -1700,10 +1703,6 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) dp_display_unprepare(dp); } - /* manual kick off plug event to train link */ - if (state == ST_DISPLAY_OFF) - dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0); - /* completed connection */ dp_display->hpd_state = ST_CONNECTED;
Display resolution change is implemented through drm modeset. Older modeset (resolution) has to be disabled first before newer modeset (resolution) can be enabled. Display disable will turn off both pixel clock and main link clock so that main link have to be re-trained during display enable to have new video stream flow again. At current implementation, display enable function manually kicks up irq_hpd_handle which will read panel link status and start link training if link status is not in sync state. However, there is rare case that a particular panel links status keep staying in sync for some period of time after main link had been shut down previously at display disabled. In this case, main link retraining will not be executed by irq_hdp_handle(). Hence video stream of newer display resolution will fail to be transmitted to panel due to main link is not in sync between host and panel. This patch will bypass irq_hpd_hanle() in favor of directly call dp_ctrl_on_stream() to always perform link training in regardless of main link status. So that no unexpected exception resolution change failure cases will happen. Also this implementation are more efficient than manual kicking off irq_hpd_handle function. Changes in v2: -- set force_link_train flag on DP only (is_edp == false) Changes in v3: -- revise commit text -- add Fixes tag Changes in v4: -- revise commit text Changes in v5: -- fix spelling at commit text Changes in v6: -- split dp_ctrl_on_stream() for phy test case -- revise commit text for modeset Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_ctrl.c | 31 +++++++++++++++++++++++-------- drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 ++- drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++------- 3 files changed, 31 insertions(+), 16 deletions(-)