| Message ID | 1653687133-32331-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] drm/msm/dp: force link training for display resolution change | expand |
Any one has any comments? Thanks, On 5/27/2022 2:32 PM, Kuogee Hsieh wrote: > During display resolution changes display have to be disabled first > followed by display enabling with new resolution. 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. Main link retraining will not be executed by > irq_hdp_handle() if the link status read from pane shows it is in > sync state. If this was happen, then 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 force main link always > be retrained during display enable procedure to prevent this rare > failed case from happening. 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 > > 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 | 6 +++--- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- > drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- > 3 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index af7a80c..bea93eb 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(&ctrl->dp_ctrl, false); > else > DRM_ERROR("failed to enable DP link controller\n"); > > @@ -1807,7 +1807,7 @@ 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(struct dp_ctrl *dp_ctrl, bool force_link_train) > { > int ret = 0; > bool mainlink_ready = false; > @@ -1848,7 +1848,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_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..b563e2e 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h > @@ -21,7 +21,7 @@ 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_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..370348d 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,14 @@ 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); > > - dp_display_enable(dp_display, 0); > + if (!dp->is_edp) > + force_link_train = true; > + } > + > + dp_display_enable(dp_display, force_link_train); > > rc = dp_display_post_enable(dp); > if (rc) { > @@ -1700,10 +1705,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; >
Quoting Kuogee Hsieh (2022-05-27 14:32:13) > During display resolution changes display have to be disabled first > followed by display enabling with new resolution. 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 s/re/re-/ > 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. Main link retraining will not be executed by > irq_hdp_handle() if the link status read from pane shows it is in s/pane/panel/ > sync state. If this was happen, then 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 force main link always > be retrained during display enable procedure to prevent this rare > failed case from happening. Also this implementation are more > efficient than manual kicking off irq_hpd_handle function. The description makes it sound like the link status is not updated, sometimes. Isn't that the real issue here? Not that link training needs to be done again (which it always does apparently), but that disabling the display doesn't wait for the link to go down. Or disabling the link is causing some sort of glitch on the sink causing it to report the status as OK when it really isn't. > > 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 > > Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index c388323..370348d 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1688,10 +1689,14 @@ 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); > > - dp_display_enable(dp_display, 0); > + if (!dp->is_edp) Does this assume eDP has one resolution? I don't understand why eDP is special here, especially if eDP has more than one resolution available it seems like we would want to retrain the link regardless. > + force_link_train = true; > + } > + > + dp_display_enable(dp_display, force_link_train); > > rc = dp_display_post_enable(dp); > if (rc) {
On 6/10/2022 2:04 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-05-27 14:32:13) >> During display resolution changes display have to be disabled first >> followed by display enabling with new resolution. 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 > s/re/re-/ > >> 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. Main link retraining will not be executed by >> irq_hdp_handle() if the link status read from pane shows it is in > s/pane/panel/ > >> sync state. If this was happen, then 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 force main link always >> be retrained during display enable procedure to prevent this rare >> failed case from happening. Also this implementation are more >> efficient than manual kicking off irq_hpd_handle function. > The description makes it sound like the link status is not updated, > sometimes. Isn't that the real issue here? Not that link training needs > to be done again (which it always does apparently), but that disabling > the display doesn't wait for the link to go down. Or disabling the link > is causing some sort of glitch on the sink causing it to report the > status as OK when it really isn't. As soon as mainlink teared down, sink has to reflect the mainlink at out of synch state in real time. link re-training always required for display resolution changes since resolution changes will involve link configuration changes, such as 2 lanes change to 1 changes or 5.7G link rate changes to 2.7G or vise versa. Therefore there is no need to check sinker's main link status to decide re training is required or not. >> 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 >> >> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index c388323..370348d 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1688,10 +1689,14 @@ 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); >> >> - dp_display_enable(dp_display, 0); >> + if (!dp->is_edp) > Does this assume eDP has one resolution? I don't understand why eDP is > special here, especially if eDP has more than one resolution available > it seems like we would want to retrain the link regardless. > >> + force_link_train = true; >> + } >> + >> + dp_display_enable(dp_display, force_link_train); >> >> rc = dp_display_post_enable(dp); >> if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c index af7a80c..bea93eb 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(&ctrl->dp_ctrl, false); else DRM_ERROR("failed to enable DP link controller\n"); @@ -1807,7 +1807,7 @@ 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(struct dp_ctrl *dp_ctrl, bool force_link_train) { int ret = 0; bool mainlink_ready = false; @@ -1848,7 +1848,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_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..b563e2e 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h @@ -21,7 +21,7 @@ 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_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..370348d 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,14 @@ 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); - dp_display_enable(dp_display, 0); + if (!dp->is_edp) + force_link_train = true; + } + + dp_display_enable(dp_display, force_link_train); rc = dp_display_post_enable(dp); if (rc) { @@ -1700,10 +1705,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;
During display resolution changes display have to be disabled first followed by display enabling with new resolution. 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. Main link retraining will not be executed by irq_hdp_handle() if the link status read from pane shows it is in sync state. If this was happen, then 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 force main link always be retrained during display enable procedure to prevent this rare failed case from happening. 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 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 | 6 +++--- drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- 3 files changed, 12 insertions(+), 11 deletions(-)