Messages in this thread | | | Date | Tue, 14 Jun 2022 13:27:12 -0700 | Subject | Re: [PATCH v5] drm/msm/dp: force link training for display resolution change | From | Kuogee Hsieh <> |
| |
On 6/14/2022 1:38 AM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-06-13 14:48:37) >> 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 panel 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. > How is resolution change different from disabling and enabling the > display? The commit text talks about resolution changes, but the code > doesn't compare resolutions from before and after to know when to > retrain the link. Can the code be made to actually do what the commit > text says? It would be clearer if the code looked for actual resolution > changes instead of hooking the dp_bridge_enable() 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 >> >> 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); > Does this even matter if it's true or false? The 'sink_request' has > DP_TEST_LINK_PHY_TEST_PATTERN set from what I can tell, and then > dp_ctrl_on_stream() bails out before calling dp_ctrl_link_retrain() > anyway. It would be nice if we could split dp_ctrl_on_stream() so that > the part after the check for the sink request is a different function > that is called by dp_display.c and then this code can call the 'prepare' > function that does the first part. Then we can ignore the testing path > in the code, and possibly remove the conditional in dp_ctrl_on_stream()? > >> else >> DRM_ERROR("failed to enable DP link controller\n"); >> >> 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) > I didn't see any answer to my question about why edp is special on v4. > Can you at least add a comment to the code about why edp doesn't need to > unconditionally retrain, but DP does?
Sorry, missed this one.
This is my mistake, both DP and eDP are same. will remove is_edp flag checking.
> >> + force_link_train = true; >> + } >> + >> + dp_display_enable(dp_display, force_link_train); >> >> rc = dp_display_post_enable(dp); >> if (rc) {
| |