Re: [PATCH 7/7] drm/msm/dp: retrain link when loss of symbol lock detected

[khsieh@codeaurora.org]

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);
>>