| Message ID | 1653585978-30090-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dp: force link training for display resolution change | expand |
Quoting Kuogee Hsieh (2022-05-26 10:26:18) > During display resolution changes display have to be disabled first > followed by display enable with new resolution. This patch force > main link always be retrained during display enable procedure to > simplify implementation instead of manually kicking of irq_hpd > handle. How is that better? Can you please describe how it improves things? I suppose it avoids some round trips through the event queue just to turn on the display? > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> Any Fixes tag? Or it's not fixing anything, just making things simpler? > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 5ddb4e8..8b71013 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h > index 2433edb..dcc7af21 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h > @@ -20,7 +20,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); Can we have another API like dp_ctrl_on_stream_retrain()? Or name 'force_link_train' to 'retrain'? > int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl); > int dp_ctrl_off(struct dp_ctrl *dp_ctrl); > void dp_ctrl_push_idle(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 bfc6581..9246421 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -892,7 +892,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data) Any reason why dp_display_enable() is forward declared and why it takes a u32 data argument? It's not part of the eventqueue calling code from what I can tell so we should be able to rename 'data' to 'retrain_link'? > return 0; > } > > - rc = dp_ctrl_on_stream(dp->ctrl); > + rc = dp_ctrl_on_stream(dp->ctrl, data); > if (!rc) > dp_display->power_on = true; > > @@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > 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) { > @@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > > 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 = 1; Use true instead of 1 if the type is a bool please. > + } > > - dp_display_enable(dp_display, 0); > + dp_display_enable(dp_display, force_link_train); > > rc = dp_display_post_enable(dp); > if (rc) {
On 5/26/2022 5:07 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-05-26 10:26:18) >> During display resolution changes display have to be disabled first >> followed by display enable with new resolution. This patch force >> main link always be retrained during display enable procedure to >> simplify implementation instead of manually kicking of irq_hpd >> handle. > How is that better? Can you please describe how it improves things? I > suppose it avoids some round trips through the event queue just to turn > on the display? This try to fix customer issues which only happen on one particular panel which fails on display resolution changes sometimes. I will add more description. > >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > Any Fixes tag? Or it's not fixing anything, just making things simpler? will add. > >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 5ddb4e8..8b71013 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> index 2433edb..dcc7af21 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h >> @@ -20,7 +20,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); > Can we have another API like dp_ctrl_on_stream_retrain()? Or name > 'force_link_train' to 'retrain'? > >> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl); >> int dp_ctrl_off(struct dp_ctrl *dp_ctrl); >> void dp_ctrl_push_idle(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 bfc6581..9246421 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -892,7 +892,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data) > Any reason why dp_display_enable() is forward declared and why it takes > a u32 data argument? It's not part of the eventqueue calling code from > what I can tell so we should be able to rename 'data' to 'retrain_link'? > >> return 0; >> } >> >> - rc = dp_ctrl_on_stream(dp->ctrl); >> + rc = dp_ctrl_on_stream(dp->ctrl, data); >> if (!rc) >> dp_display->power_on = true; >> >> @@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) >> 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) { >> @@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) >> >> 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 = 1; > Use true instead of 1 if the type is a bool please. > >> + } >> >> - dp_display_enable(dp_display, 0); >> + 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 5ddb4e8..8b71013 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1545,7 +1545,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"); @@ -1802,7 +1802,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; @@ -1827,7 +1827,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) } } - 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 2433edb..dcc7af21 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h @@ -20,7 +20,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(struct dp_ctrl *dp_ctrl); void dp_ctrl_push_idle(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 bfc6581..9246421 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -892,7 +892,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; @@ -1594,6 +1594,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) 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) { @@ -1622,10 +1623,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) 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 = 1; + } - dp_display_enable(dp_display, 0); + dp_display_enable(dp_display, force_link_train); rc = dp_display_post_enable(dp); if (rc) { @@ -1634,10 +1637,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) 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 enable with new resolution. This patch force main link always be retrained during display enable procedure to simplify implementation instead of manually kicking of irq_hpd handle. 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 | 13 ++++++------- 3 files changed, 10 insertions(+), 11 deletions(-)