Messages in this thread | | | Date | Fri, 09 Jul 2021 10:16:52 -0700 | From | khsieh@codeauro ... | Subject | Re: [PATCH 7/7] drm/msm/dp: retrain link when loss of symbol lock detected |
| |
On 2021-07-08 00:21, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2021-07-06 10:20:20) >> Main link symbol locked is achieved at end of link training 2. Some >> dongle main link symbol may become unlocked again if host did not end >> link training soon enough after completion of link training 2. Host >> have to re train main link if loss of symbol lock detected before >> end link training so that the coming video stream can be transmitted >> to sink properly. >> >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> > > I guess this is a fix for the original driver, so it should be tagged > with Fixes appropriately. Actually, this is fix on patch #6 : drm/msm/dp: do not end dp link training until video is ready Should i merge patch #6 and #7 together? Or can you suggest what should I do? > >> --- >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 >> ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 0cb01a9..e616ab2 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -1661,6 +1661,25 @@ static bool dp_ctrl_any_lane_cr_lose(struct >> dp_ctrl_private *ctrl, >> return false; >> } >> >> +static bool dp_ctrl_loss_symbol_lock(struct dp_ctrl_private *ctrl) >> +{ >> + u8 link_status[6]; > > Can we use link_status[DP_LINK_STATUS_SIZE] instead? > >> + u8 status; >> + int i; >> + int lane = ctrl->link->link_params.num_lanes; > > s/lane/num_lanes/ > > would make the code easier to read > >> + >> + dp_ctrl_read_link_status(ctrl, link_status); >> + >> + for (i = 0; i < lane; i++) { >> + status = link_status[i / 2]; >> + status >>= ((i % 2) * 4); >> + if (!(status & DP_LANE_SYMBOL_LOCKED)) >> + return true; >> + } >> + >> + return false; >> +} >> + >> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl) >> { >> int rc = 0; >> @@ -1777,6 +1796,17 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl) >> return rc; >> } >> >> +static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl) >> +{ >> + int ret = 0; > > Please drop init of ret. > >> + u8 cr_status[2]; >> + int training_step = DP_TRAINING_NONE; >> + >> + ret = dp_ctrl_setup_main_link(ctrl, cr_status, >> &training_step); > > as it is assigned here. > >> + >> + return ret; > > And indeed, it could be 'return dp_ctrl_setup_main_link()' instead. > >> +} >> + >> int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> { >> int ret = 0; >> @@ -1802,6 +1832,10 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) >> } >> } >> >> + /* if loss symbol lock happen, then retaining the link */ > > retain or retrain? The comment seems to be saying what the code says > "if > loss retrain", so the comment is not very useful. > >> + if (dp_ctrl_loss_symbol_lock(ctrl)) >> + dp_ctrl_link_retrain(ctrl); >> + >> /* stop txing train pattern to end link training */ >> dp_ctrl_clear_training_pattern(ctrl); >>
| |