[PATCH 0/7] add fixes to pass DP Link Layer compliance test cases

[PATCH 0/7] add fixes to pass DP Link Layer compliance test cases

[Kuogee Hsieh]

add fixes to pass DP Link Layer compliance test cases

Kuogee Hsieh (7):
  drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run
  drm/msm/dp: reduce link rate if failed at link training 1
  drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.
  drm/msm/dp: replug event is converted into an unplug followed by an
    plug events
  drm/msm/dp: return correct edid checksum after corrupted edid checksum
    read
  drm/msm/dp: do not end dp link training until video is ready
  drm/msm/dp: retrain link when loss of symbol lock detected

 drivers/gpu/drm/msm/dp/dp_aux.c     |   3 +
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 158 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/msm/dp/dp_display.c |  13 ++-
 drivers/gpu/drm/msm/dp/dp_panel.c   |   9 +-
 4 files changed, 127 insertions(+), 56 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run

[Kuogee Hsieh]

DP cable should always connect to DPU during the entire PHY compliance
testing run. Since DP PHY compliance test is executed at irq_hpd event
context, dp_ctrl_off_link_stream() should be used instead of dp_ctrl_off().
dp_ctrl_off() is used for unplug event which is triggered when DP cable is
dis connected.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index caf71fa..27fb0f0 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1530,7 +1530,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 	 * running. Add the global reset just before disabling the
 	 * link clocks and core clocks.
 	 */
-	ret = dp_ctrl_off(&ctrl->dp_ctrl);
+	ret = dp_ctrl_off_link_stream(&ctrl->dp_ctrl);
 	if (ret) {
 		DRM_ERROR("failed to disable DP controller\n");
 		return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-07-06 10:20:14)
> DP cable should always connect to DPU during the entire PHY compliance
> testing run. Since DP PHY compliance test is executed at irq_hpd event
> context, dp_ctrl_off_link_stream() should be used instead of dp_ctrl_off().
> dp_ctrl_off() is used for unplug event which is triggered when DP cable is
> dis connected.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---

Is this

Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0 correctly")

or

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")

? It's not clear how dp_ctrl_off() was working for compliance tests
before commit f21c8a276c2d.

>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index caf71fa..27fb0f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1530,7 +1530,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>          * running. Add the global reset just before disabling the
>          * link clocks and core clocks.
>          */
> -       ret = dp_ctrl_off(&ctrl->dp_ctrl);
> +       ret = dp_ctrl_off_link_stream(&ctrl->dp_ctrl);
>         if (ret) {
>                 DRM_ERROR("failed to disable DP controller\n");
>                 return ret;

Re: [PATCH 1/7] drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run

[khsieh]

On 2021-07-08 00:03, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-07-06 10:20:14)
>> DP cable should always connect to DPU during the entire PHY compliance
>> testing run. Since DP PHY compliance test is executed at irq_hpd event
>> context, dp_ctrl_off_link_stream() should be used instead of 
>> dp_ctrl_off().
>> dp_ctrl_off() is used for unplug event which is triggered when DP 
>> cable is
>> dis connected.
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
> 
> Is this
> 
> Fixes: f21c8a276c2d ("drm/msm/dp: handle irq_hpd with sink_count = 0 
> correctly")
> 
> or
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")

should be fixes at f21c8a276c2d ("drm/msm/dp: handle irq_hpd with 
sink_count = 0 correctly")

> 
> ? It's not clear how dp_ctrl_off() was working for compliance tests
> before commit f21c8a276c2d.
both dp_ctrl_off() and dp_ctrl_off_link_strea() are work for 
dp_ctrl_process_phy_test_request()
The problem is after dp_ctrl_off(), aux channel is down, hence next phy 
test will failed due to dpcd read failed.
So that cable unplugged and replug back to required to run next test 
case.
dp_ctrl_off_link_stream() will keep aux channel up and other phy test 
case can be continued.



>>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index caf71fa..27fb0f0 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1530,7 +1530,7 @@ static int 
>> dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>>          * running. Add the global reset just before disabling the
>>          * link clocks and core clocks.
>>          */
>> -       ret = dp_ctrl_off(&ctrl->dp_ctrl);
>> +       ret = dp_ctrl_off_link_stream(&ctrl->dp_ctrl);
>>         if (ret) {
>>                 DRM_ERROR("failed to disable DP controller\n");
>>                 return ret;

[PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1

[Kuogee Hsieh]

Reduce link rate and re start link training if link training 1
failed due to loss of clock recovery done to fix Link Layer
CTS case 4.3.1.7.  Also only update voltage and pre-emphasis
swing level after link training started to fix Link Layer CTS
case 4.3.1.6.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 ++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 27fb0f0..6f8443d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -83,13 +83,6 @@ struct dp_ctrl_private {
 	struct completion video_comp;
 };
 
-struct dp_cr_status {
-	u8 lane_0_1;
-	u8 lane_2_3;
-};
-
-#define DP_LANE0_1_CR_DONE	0x11
-
 static int dp_aux_link_configure(struct drm_dp_aux *aux,
 					struct dp_link_info *link)
 {
@@ -1080,7 +1073,7 @@ static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
 }
 
 static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
-		struct dp_cr_status *cr, int *training_step)
+		u8 *cr, int *training_step)
 {
 	int tries, old_v_level, ret = 0;
 	u8 link_status[DP_LINK_STATUS_SIZE];
@@ -1109,8 +1102,8 @@ static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
 		if (ret)
 			return ret;
 
-		cr->lane_0_1 = link_status[0];
-		cr->lane_2_3 = link_status[1];
+		cr[0] = link_status[0];
+		cr[1] = link_status[1];
 
 		if (drm_dp_clock_recovery_ok(link_status,
 			ctrl->link->link_params.num_lanes)) {
@@ -1188,7 +1181,7 @@ static void dp_ctrl_clear_training_pattern(struct dp_ctrl_private *ctrl)
 }
 
 static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
-		struct dp_cr_status *cr, int *training_step)
+		u8 *cr, int *training_step)
 {
 	int tries = 0, ret = 0;
 	char pattern;
@@ -1204,10 +1197,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
 	else
 		pattern = DP_TRAINING_PATTERN_2;
 
-	ret = dp_ctrl_update_vx_px(ctrl);
-	if (ret)
-		return ret;
-
 	ret = dp_catalog_ctrl_set_pattern(ctrl->catalog, pattern);
 	if (ret)
 		return ret;
@@ -1220,8 +1209,8 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
 		ret = dp_ctrl_read_link_status(ctrl, link_status);
 		if (ret)
 			return ret;
-		cr->lane_0_1 = link_status[0];
-		cr->lane_2_3 = link_status[1];
+		cr[0] = link_status[0];
+		cr[1] = link_status[1];
 
 		if (drm_dp_channel_eq_ok(link_status,
 			ctrl->link->link_params.num_lanes)) {
@@ -1241,7 +1230,7 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
 static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
 
 static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
-		struct dp_cr_status *cr, int *training_step)
+		u8 *cr, int *training_step)
 {
 	int ret = 0;
 	u8 encoding = DP_SET_ANSI_8B10B;
@@ -1282,7 +1271,7 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
 }
 
 static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
-		struct dp_cr_status *cr, int *training_step)
+		u8 *cr, int *training_step)
 {
 	int ret = 0;
 
@@ -1496,14 +1485,14 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
 static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 {
 	int ret = 0;
-	struct dp_cr_status cr;
+	u8 cr_status[2];
 	int training_step = DP_TRAINING_NONE;
 
 	dp_ctrl_push_idle(&ctrl->dp_ctrl);
 
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
-	ret = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
+	ret = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
 	if (ret)
 		goto end;
 
@@ -1634,6 +1623,41 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
 	}
 }
 
+static bool dp_ctrl_any_lane_cr_done(struct dp_ctrl_private *ctrl,
+					u8 *cr_status)
+
+{
+	int i;
+	u8 status;
+	int lane = ctrl->link->link_params.num_lanes;
+
+	for (i = 0; i < lane; i++) {
+		status = cr_status[i / 2];
+		status >>= ((i % 2) * 4);
+		if (status & DP_LANE_CR_DONE)
+			return true;
+	}
+
+	return false;
+}
+
+static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl,
+					u8 *cr_status)
+{
+	int i;
+	u8 status;
+	int lane = ctrl->link->link_params.num_lanes;
+
+	for (i = 0; i < lane; i++) {
+		status = cr_status[i / 2];
+		status >>= ((i % 2) * 4);
+		if (!(status & DP_LANE_CR_DONE))
+			return true;
+	}
+
+	return false;
+}
+
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 {
 	int rc = 0;
@@ -1641,7 +1665,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 	u32 rate = 0;
 	int link_train_max_retries = 5;
 	u32 const phy_cts_pixel_clk_khz = 148500;
-	struct dp_cr_status cr;
+	u8 cr_status[2];
 	unsigned int training_step;
 
 	if (!dp_ctrl)
@@ -1681,19 +1705,18 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 		}
 
 		training_step = DP_TRAINING_NONE;
-		rc = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
+		rc = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
 		if (rc == 0) {
 			/* training completed successfully */
 			break;
 		} else if (training_step == DP_TRAINING_1) {
 			/* link train_1 failed */
-			if (!dp_catalog_link_is_connected(ctrl->catalog)) {
+			if (!dp_catalog_link_is_connected(ctrl->catalog))
 				break;
-			}
 
 			rc = dp_ctrl_link_rate_down_shift(ctrl);
 			if (rc < 0) { /* already in RBR = 1.6G */
-				if (cr.lane_0_1 & DP_LANE0_1_CR_DONE) {
+				if (dp_ctrl_any_lane_cr_done(ctrl, cr_status)) {
 					/*
 					 * some lanes are ready,
 					 * reduce lane number
@@ -1709,12 +1732,15 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 				}
 			}
 		} else if (training_step == DP_TRAINING_2) {
-			/* link train_2 failed, lower lane rate */
-			if (!dp_catalog_link_is_connected(ctrl->catalog)) {
+			/* link train_2 failed */
+			if (!dp_catalog_link_is_connected(ctrl->catalog))
 				break;
-			}
 
-			rc = dp_ctrl_link_lane_down_shift(ctrl);
+			if (dp_ctrl_any_lane_cr_lose(ctrl, cr_status))
+				rc = dp_ctrl_link_rate_down_shift(ctrl);
+			else
+				rc = dp_ctrl_link_lane_down_shift(ctrl);
+
 			if (rc < 0) {
 				/* end with failure */
 				break; /* lane == 1 already */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-07-06 10:20:15)
> Reduce link rate and re start link training if link training 1
> failed due to loss of clock recovery done to fix Link Layer
> CTS case 4.3.1.7.  Also only update voltage and pre-emphasis
> swing level after link training started to fix Link Layer CTS
> case 4.3.1.6.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 ++++++++++++++++++++++++++--------------
>  1 file changed, 56 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 27fb0f0..6f8443d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -83,13 +83,6 @@ struct dp_ctrl_private {
>         struct completion video_comp;
>  };
>
> -struct dp_cr_status {
> -       u8 lane_0_1;
> -       u8 lane_2_3;
> -};
> -
> -#define DP_LANE0_1_CR_DONE     0x11
> -
>  static int dp_aux_link_configure(struct drm_dp_aux *aux,
>                                         struct dp_link_info *link)
>  {
> @@ -1080,7 +1073,7 @@ static int dp_ctrl_read_link_status(struct dp_ctrl_private *ctrl,
>  }
>
>  static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
> -               struct dp_cr_status *cr, int *training_step)
> +               u8 *cr, int *training_step)
>  {
>         int tries, old_v_level, ret = 0;
>         u8 link_status[DP_LINK_STATUS_SIZE];
> @@ -1109,8 +1102,8 @@ static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
>                 if (ret)
>                         return ret;
>
> -               cr->lane_0_1 = link_status[0];
> -               cr->lane_2_3 = link_status[1];
> +               cr[0] = link_status[0];
> +               cr[1] = link_status[1];
>
>                 if (drm_dp_clock_recovery_ok(link_status,
>                         ctrl->link->link_params.num_lanes)) {
> @@ -1188,7 +1181,7 @@ static void dp_ctrl_clear_training_pattern(struct dp_ctrl_private *ctrl)
>  }
>
>  static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
> -               struct dp_cr_status *cr, int *training_step)
> +               u8 *cr, int *training_step)
>  {
>         int tries = 0, ret = 0;
>         char pattern;
> @@ -1204,10 +1197,6 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>         else
>                 pattern = DP_TRAINING_PATTERN_2;
>
> -       ret = dp_ctrl_update_vx_px(ctrl);
> -       if (ret)
> -               return ret;
> -
>         ret = dp_catalog_ctrl_set_pattern(ctrl->catalog, pattern);
>         if (ret)
>                 return ret;
> @@ -1220,8 +1209,8 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>                 ret = dp_ctrl_read_link_status(ctrl, link_status);
>                 if (ret)
>                         return ret;
> -               cr->lane_0_1 = link_status[0];
> -               cr->lane_2_3 = link_status[1];
> +               cr[0] = link_status[0];
> +               cr[1] = link_status[1];
>
>                 if (drm_dp_channel_eq_ok(link_status,
>                         ctrl->link->link_params.num_lanes)) {
> @@ -1241,7 +1230,7 @@ static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>  static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl);
>
>  static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
> -               struct dp_cr_status *cr, int *training_step)
> +               u8 *cr, int *training_step)
>  {
>         int ret = 0;
>         u8 encoding = DP_SET_ANSI_8B10B;
> @@ -1282,7 +1271,7 @@ static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>  }
>
>  static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
> -               struct dp_cr_status *cr, int *training_step)
> +               u8 *cr, int *training_step)
>  {
>         int ret = 0;
>
> @@ -1496,14 +1485,14 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
>  static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>  {
>         int ret = 0;
> -       struct dp_cr_status cr;
> +       u8 cr_status[2];
>         int training_step = DP_TRAINING_NONE;
>
>         dp_ctrl_push_idle(&ctrl->dp_ctrl);
>
>         ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>
> -       ret = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
> +       ret = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
>         if (ret)
>                 goto end;

Do we need to extract the link status information from deep in these
functions? Why not read it again when we need to?

>
> @@ -1634,6 +1623,41 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
>         }
>  }
>
> +static bool dp_ctrl_any_lane_cr_done(struct dp_ctrl_private *ctrl,
> +                                       u8 *cr_status)
> +
> +{
> +       int i;
> +       u8 status;
> +       int lane = ctrl->link->link_params.num_lanes;
> +
> +       for (i = 0; i < lane; i++) {
> +               status = cr_status[i / 2];
> +               status >>= ((i % 2) * 4);
> +               if (status & DP_LANE_CR_DONE)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
> +static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl,
> +                                       u8 *cr_status)
> +{
> +       int i;
> +       u8 status;
> +       int lane = ctrl->link->link_params.num_lanes;
> +
> +       for (i = 0; i < lane; i++) {
> +               status = cr_status[i / 2];
> +               status >>= ((i % 2) * 4);
> +               if (!(status & DP_LANE_CR_DONE))
> +                       return true;
> +       }
> +
> +       return false;
> +}

Why not use !drm_dp_clock_recovery_ok() for dp_ctrl_any_lane_cr_lose()?
And then move dp_ctrl_any_lane_cr_done() next to
drm_dp_clock_recovery_ok() and call it drm_dp_clock_recovery_any_ok()?

> +
>  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>  {
>         int rc = 0;
> @@ -1641,7 +1665,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>         u32 rate = 0;
>         int link_train_max_retries = 5;
>         u32 const phy_cts_pixel_clk_khz = 148500;
> -       struct dp_cr_status cr;
> +       u8 cr_status[2];
>         unsigned int training_step;
>
>         if (!dp_ctrl)
> @@ -1681,19 +1705,18 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>                 }
>
>                 training_step = DP_TRAINING_NONE;
> -               rc = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
> +               rc = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
>                 if (rc == 0) {
>                         /* training completed successfully */
>                         break;
>                 } else if (training_step == DP_TRAINING_1) {
>                         /* link train_1 failed */
> -                       if (!dp_catalog_link_is_connected(ctrl->catalog)) {
> +                       if (!dp_catalog_link_is_connected(ctrl->catalog))
>                                 break;
> -                       }
>
>                         rc = dp_ctrl_link_rate_down_shift(ctrl);
>                         if (rc < 0) { /* already in RBR = 1.6G */
> -                               if (cr.lane_0_1 & DP_LANE0_1_CR_DONE) {
> +                               if (dp_ctrl_any_lane_cr_done(ctrl, cr_status)) {
>                                         /*
>                                          * some lanes are ready,
>                                          * reduce lane number
> @@ -1709,12 +1732,15 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>                                 }
>                         }
>                 } else if (training_step == DP_TRAINING_2) {
> -                       /* link train_2 failed, lower lane rate */
> -                       if (!dp_catalog_link_is_connected(ctrl->catalog)) {
> +                       /* link train_2 failed */
> +                       if (!dp_catalog_link_is_connected(ctrl->catalog))
>                                 break;
> -                       }
>
> -                       rc = dp_ctrl_link_lane_down_shift(ctrl);
> +                       if (dp_ctrl_any_lane_cr_lose(ctrl, cr_status))
> +                               rc = dp_ctrl_link_rate_down_shift(ctrl);
> +                       else
> +                               rc = dp_ctrl_link_lane_down_shift(ctrl);
> +
>                         if (rc < 0) {
>                                 /* end with failure */
>                                 break; /* lane == 1 already */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1

[khsieh]

On 2021-07-08 00:33, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-07-06 10:20:15)
>> Reduce link rate and re start link training if link training 1
>> failed due to loss of clock recovery done to fix Link Layer
>> CTS case 4.3.1.7.  Also only update voltage and pre-emphasis
>> swing level after link training started to fix Link Layer CTS
>> case 4.3.1.6.
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 
>> ++++++++++++++++++++++++++--------------
>>  1 file changed, 56 insertions(+), 30 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 27fb0f0..6f8443d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -83,13 +83,6 @@ struct dp_ctrl_private {
>>         struct completion video_comp;
>>  };
>> 
>> -struct dp_cr_status {
>> -       u8 lane_0_1;
>> -       u8 lane_2_3;
>> -};
>> -
>> -#define DP_LANE0_1_CR_DONE     0x11
>> -
>>  static int dp_aux_link_configure(struct drm_dp_aux *aux,
>>                                         struct dp_link_info *link)
>>  {
>> @@ -1080,7 +1073,7 @@ static int dp_ctrl_read_link_status(struct 
>> dp_ctrl_private *ctrl,
>>  }
>> 
>>  static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
>> -               struct dp_cr_status *cr, int *training_step)
>> +               u8 *cr, int *training_step)
>>  {
>>         int tries, old_v_level, ret = 0;
>>         u8 link_status[DP_LINK_STATUS_SIZE];
>> @@ -1109,8 +1102,8 @@ static int dp_ctrl_link_train_1(struct 
>> dp_ctrl_private *ctrl,
>>                 if (ret)
>>                         return ret;
>> 
>> -               cr->lane_0_1 = link_status[0];
>> -               cr->lane_2_3 = link_status[1];
>> +               cr[0] = link_status[0];
>> +               cr[1] = link_status[1];
>> 
>>                 if (drm_dp_clock_recovery_ok(link_status,
>>                         ctrl->link->link_params.num_lanes)) {
>> @@ -1188,7 +1181,7 @@ static void 
>> dp_ctrl_clear_training_pattern(struct dp_ctrl_private *ctrl)
>>  }
>> 
>>  static int dp_ctrl_link_train_2(struct dp_ctrl_private *ctrl,
>> -               struct dp_cr_status *cr, int *training_step)
>> +               u8 *cr, int *training_step)
>>  {
>>         int tries = 0, ret = 0;
>>         char pattern;
>> @@ -1204,10 +1197,6 @@ static int dp_ctrl_link_train_2(struct 
>> dp_ctrl_private *ctrl,
>>         else
>>                 pattern = DP_TRAINING_PATTERN_2;
>> 
>> -       ret = dp_ctrl_update_vx_px(ctrl);
>> -       if (ret)
>> -               return ret;
>> -
>>         ret = dp_catalog_ctrl_set_pattern(ctrl->catalog, pattern);
>>         if (ret)
>>                 return ret;
>> @@ -1220,8 +1209,8 @@ static int dp_ctrl_link_train_2(struct 
>> dp_ctrl_private *ctrl,
>>                 ret = dp_ctrl_read_link_status(ctrl, link_status);
>>                 if (ret)
>>                         return ret;
>> -               cr->lane_0_1 = link_status[0];
>> -               cr->lane_2_3 = link_status[1];
>> +               cr[0] = link_status[0];
>> +               cr[1] = link_status[1];
>> 
>>                 if (drm_dp_channel_eq_ok(link_status,
>>                         ctrl->link->link_params.num_lanes)) {
>> @@ -1241,7 +1230,7 @@ static int dp_ctrl_link_train_2(struct 
>> dp_ctrl_private *ctrl,
>>  static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private 
>> *ctrl);
>> 
>>  static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
>> -               struct dp_cr_status *cr, int *training_step)
>> +               u8 *cr, int *training_step)
>>  {
>>         int ret = 0;
>>         u8 encoding = DP_SET_ANSI_8B10B;
>> @@ -1282,7 +1271,7 @@ static int dp_ctrl_link_train(struct 
>> dp_ctrl_private *ctrl,
>>  }
>> 
>>  static int dp_ctrl_setup_main_link(struct dp_ctrl_private *ctrl,
>> -               struct dp_cr_status *cr, int *training_step)
>> +               u8 *cr, int *training_step)
>>  {
>>         int ret = 0;
>> 
>> @@ -1496,14 +1485,14 @@ static int 
>> dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
>>  static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>>  {
>>         int ret = 0;
>> -       struct dp_cr_status cr;
>> +       u8 cr_status[2];
>>         int training_step = DP_TRAINING_NONE;
>> 
>>         dp_ctrl_push_idle(&ctrl->dp_ctrl);
>> 
>>         ctrl->dp_ctrl.pixel_rate = 
>> ctrl->panel->dp_mode.drm_mode.clock;
>> 
>> -       ret = dp_ctrl_setup_main_link(ctrl, &cr, &training_step);
>> +       ret = dp_ctrl_setup_main_link(ctrl, cr_status, 
>> &training_step);
>>         if (ret)
>>                 goto end;
> 
> Do we need to extract the link status information from deep in these
> functions? Why not read it again when we need to?
> 
>> 
>> @@ -1634,6 +1623,41 @@ void dp_ctrl_handle_sink_request(struct dp_ctrl 
>> *dp_ctrl)
>>         }
>>  }
>> 
>> +static bool dp_ctrl_any_lane_cr_done(struct dp_ctrl_private *ctrl,
>> +                                       u8 *cr_status)
>> +
>> +{
>> +       int i;
>> +       u8 status;
>> +       int lane = ctrl->link->link_params.num_lanes;
>> +
>> +       for (i = 0; i < lane; i++) {
>> +               status = cr_status[i / 2];
>> +               status >>= ((i % 2) * 4);
>> +               if (status & DP_LANE_CR_DONE)
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
>> +
>> +static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl,
>> +                                       u8 *cr_status)
>> +{
>> +       int i;
>> +       u8 status;
>> +       int lane = ctrl->link->link_params.num_lanes;
>> +
>> +       for (i = 0; i < lane; i++) {
>> +               status = cr_status[i / 2];
>> +               status >>= ((i % 2) * 4);
>> +               if (!(status & DP_LANE_CR_DONE))
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
> 
> Why not use !drm_dp_clock_recovery_ok() for dp_ctrl_any_lane_cr_lose()?
ok,

> And then move dp_ctrl_any_lane_cr_done() next to
> drm_dp_clock_recovery_ok() and call it drm_dp_clock_recovery_any_ok()?
no understand how it work, can you elaborate it more?
> 
>> +
>>  int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>>  {
>>         int rc = 0;
>> @@ -1641,7 +1665,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>>         u32 rate = 0;
>>         int link_train_max_retries = 5;
>>         u32 const phy_cts_pixel_clk_khz = 148500;
>> -       struct dp_cr_status cr;
>> +       u8 cr_status[2];
>>         unsigned int training_step;
>> 
>>         if (!dp_ctrl)
>> @@ -1681,19 +1705,18 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>>                 }
>> 
>>                 training_step = DP_TRAINING_NONE;
>> -               rc = dp_ctrl_setup_main_link(ctrl, &cr, 
>> &training_step);
>> +               rc = dp_ctrl_setup_main_link(ctrl, cr_status, 
>> &training_step);
>>                 if (rc == 0) {
>>                         /* training completed successfully */
>>                         break;
>>                 } else if (training_step == DP_TRAINING_1) {
>>                         /* link train_1 failed */
>> -                       if 
>> (!dp_catalog_link_is_connected(ctrl->catalog)) {
>> +                       if 
>> (!dp_catalog_link_is_connected(ctrl->catalog))
>>                                 break;
>> -                       }
>> 
>>                         rc = dp_ctrl_link_rate_down_shift(ctrl);
>>                         if (rc < 0) { /* already in RBR = 1.6G */
>> -                               if (cr.lane_0_1 & DP_LANE0_1_CR_DONE) 
>> {
>> +                               if (dp_ctrl_any_lane_cr_done(ctrl, 
>> cr_status)) {
>>                                         /*
>>                                          * some lanes are ready,
>>                                          * reduce lane number
>> @@ -1709,12 +1732,15 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>>                                 }
>>                         }
>>                 } else if (training_step == DP_TRAINING_2) {
>> -                       /* link train_2 failed, lower lane rate */
>> -                       if 
>> (!dp_catalog_link_is_connected(ctrl->catalog)) {
>> +                       /* link train_2 failed */
>> +                       if 
>> (!dp_catalog_link_is_connected(ctrl->catalog))
>>                                 break;
>> -                       }
>> 
>> -                       rc = dp_ctrl_link_lane_down_shift(ctrl);
>> +                       if (dp_ctrl_any_lane_cr_lose(ctrl, cr_status))
>> +                               rc = 
>> dp_ctrl_link_rate_down_shift(ctrl);
>> +                       else
>> +                               rc = 
>> dp_ctrl_link_lane_down_shift(ctrl);
>> +
>>                         if (rc < 0) {
>>                                 /* end with failure */
>>                                 break; /* lane == 1 already */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH 2/7] drm/msm/dp: reduce link rate if failed at link training 1

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-07-09 10:46:41)
> On 2021-07-08 00:33, Stephen Boyd wrote:
> >> +
> >> +static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl,
> >> +                                       u8 *cr_status)
> >> +{
> >> +       int i;
> >> +       u8 status;
> >> +       int lane = ctrl->link->link_params.num_lanes;
> >> +
> >> +       for (i = 0; i < lane; i++) {
> >> +               status = cr_status[i / 2];
> >> +               status >>= ((i % 2) * 4);
> >> +               if (!(status & DP_LANE_CR_DONE))
> >> +                       return true;
> >> +       }
> >> +
> >> +       return false;
> >> +}
> >
> > Why not use !drm_dp_clock_recovery_ok() for dp_ctrl_any_lane_cr_lose()?
> ok,
>
> > And then move dp_ctrl_any_lane_cr_done() next to
> > drm_dp_clock_recovery_ok() and call it drm_dp_clock_recovery_any_ok()?
> no understand how it work, can you elaborate it more?

I'm suggesting to make a new function called
drm_dp_clock_recovery_any_ok(), written next to
drm_dp_clock_recovery_ok(). Then call it from here instead of implement
it locally in the qcom DP driver.

[PATCH 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.

[Kuogee Hsieh]

Aux hardware calibration sequence requires resetting the aux controller
in order for the new setting to take effect. However resetting the AUX
controller will also clear HPD interrupt status which may accidentally
cause pending unplug interrupt to get lost. Therefore reset aux
controller only when link is in connection state when dp_aux_cmd_fifo_tx()
fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 4a3293b..eb40d84 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
 				dp_catalog_aux_update_cfg(aux->catalog);
 		}
+		/* reset aux if link is in connected state */
+		if (dp_catalog_link_is_connected(aux->catalog))
+			dp_catalog_aux_reset(aux->catalog);
 	} else {
 		aux->retry_cnt = 0;
 		switch (aux->aux_error_num) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-07-06 10:20:16)
> Aux hardware calibration sequence requires resetting the aux controller
> in order for the new setting to take effect. However resetting the AUX
> controller will also clear HPD interrupt status which may accidentally
> cause pending unplug interrupt to get lost. Therefore reset aux
> controller only when link is in connection state when dp_aux_cmd_fifo_tx()
> fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 4a3293b..eb40d84 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                         if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>                                 dp_catalog_aux_update_cfg(aux->catalog);
>                 }
> +               /* reset aux if link is in connected state */
> +               if (dp_catalog_link_is_connected(aux->catalog))

How do we avoid resetting aux when hpd is unplugged and then plugged
back in during an aux transfer?

> +                       dp_catalog_aux_reset(aux->catalog);
>         } else {
>                 aux->retry_cnt = 0;
>                 switch (aux->aux_error_num) {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH 3/7] drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed.

[khsieh]

On 2021-07-08 00:34, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-07-06 10:20:16)
>> Aux hardware calibration sequence requires resetting the aux 
>> controller
>> in order for the new setting to take effect. However resetting the AUX
>> controller will also clear HPD interrupt status which may accidentally
>> cause pending unplug interrupt to get lost. Therefore reset aux
>> controller only when link is in connection state when 
>> dp_aux_cmd_fifo_tx()
>> fail. This fixes Link Layer CTS cases 4.2.1.1 and 4.2.1.2.
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 4a3293b..eb40d84 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
>> *dp_aux,
>>                         if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>>                                 
>> dp_catalog_aux_update_cfg(aux->catalog);
>>                 }
>> +               /* reset aux if link is in connected state */
>> +               if (dp_catalog_link_is_connected(aux->catalog))
> 
> How do we avoid resetting aux when hpd is unplugged and then plugged
> back in during an aux transfer?
i am not sure this is possible.
it should get unplug interrupt followed by plugin interrupt.
In this case, aux will be re set and initialized
> 
>> +                       dp_catalog_aux_reset(aux->catalog);
>>         } else {
>>                 aux->retry_cnt = 0;
>>                 switch (aux->aux_error_num) {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

[PATCH 4/7] drm/msm/dp: replug event is converted into an unplug followed by an plug events

[Kuogee Hsieh]

Remove special handling of replug interrupt and instead treat replug event
as a sequential unplug followed by a plugin event. This is needed to meet
the requirements of DP Link Layer CTS test case 4.2.1.3.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 78c5301..d089ada 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1146,9 +1146,6 @@ static int hpd_event_thread(void *data)
 		case EV_IRQ_HPD_INT:
 			dp_irq_hpd_handle(dp_priv, todo->data);
 			break;
-		case EV_HPD_REPLUG_INT:
-			/* do nothing */
-			break;
 		case EV_USER_NOTIFICATION:
 			dp_display_send_hpd_notification(dp_priv,
 						todo->data);
@@ -1192,10 +1189,8 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
 
 	if (hpd_isr_status & 0x0F) {
 		/* hpd related interrupts */
-		if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
-			hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
+		if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
 			dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
-		}
 
 		if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
 			/* stop sentinel connect pending checking */
@@ -1203,8 +1198,10 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
 			dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
 		}
 
-		if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK)
-			dp_add_event(dp, EV_HPD_REPLUG_INT, 0, 0);
+		if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
+			dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+			dp_add_event(dp, EV_HPD_PLUG_INT, 0, 3);
+		}
 
 		if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
 			dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/7] drm/msm/dp: replug event is converted into an unplug followed by an plug events

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-07-06 10:20:17)
> Remove special handling of replug interrupt and instead treat replug event
> as a sequential unplug followed by a plugin event. This is needed to meet
> the requirements of DP Link Layer CTS test case 4.2.1.3.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---

This needs a Fixes tag of some kind.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read

[Kuogee Hsieh]

Response with correct edid checksum saved at connector after corrupted edid
checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 88196f7..0fdb551 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -271,7 +271,7 @@ static u8 dp_panel_get_edid_checksum(struct edid *edid)
 {
 	struct edid *last_block;
 	u8 *raw_edid;
-	bool is_edid_corrupt;
+	bool is_edid_corrupt = false;
 
 	if (!edid) {
 		DRM_ERROR("invalid edid input\n");
@@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
 	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
 
 	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
-		u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
+		u8 checksum;
+
+		if (dp_panel->edid)
+			checksum = dp_panel_get_edid_checksum(dp_panel->edid);
+		else
+			checksum = dp_panel->connector->real_edid_checksum;
 
 		dp_link_send_edid_checksum(panel->link, checksum);
 		dp_link_send_test_response(panel->link);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read

[Stephen Boyd]

Quoting Kuogee Hsieh (2021-07-06 10:20:18)
> Response with correct edid checksum saved at connector after corrupted edid
> checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 88196f7..0fdb551 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -271,7 +271,7 @@ static u8 dp_panel_get_edid_checksum(struct edid *edid)
>  {
>         struct edid *last_block;
>         u8 *raw_edid;
> -       bool is_edid_corrupt;
> +       bool is_edid_corrupt = false;
>
>         if (!edid) {
>                 DRM_ERROR("invalid edid input\n");
> @@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
>         panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>
>         if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> -               u8 checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> +               u8 checksum;
> +
> +               if (dp_panel->edid)
> +                       checksum = dp_panel_get_edid_checksum(dp_panel->edid);
> +               else
> +                       checksum = dp_panel->connector->real_edid_checksum;
>
>                 dp_link_send_edid_checksum(panel->link, checksum);

It looks like this can be drm_dp_send_real_edid_checksum()? Then we
don't have to look at the connector internals sometimes and can drop
dp_panel_get_edid_checksum() entirely?

>                 dp_link_send_test_response(panel->link);

Re: [PATCH 5/7] drm/msm/dp: return correct edid checksum after corrupted edid checksum read

[khsieh]

On 2021-07-08 00:14, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-07-06 10:20:18)
>> Response with correct edid checksum saved at connector after corrupted 
>> edid
>> checksum read. This fixes Link Layer CTS cases 4.2.2.3, 4.2.2.6.
>> 
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 88196f7..0fdb551 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -271,7 +271,7 @@ static u8 dp_panel_get_edid_checksum(struct edid 
>> *edid)
>>  {
>>         struct edid *last_block;
>>         u8 *raw_edid;
>> -       bool is_edid_corrupt;
>> +       bool is_edid_corrupt = false;
>> 
>>         if (!edid) {
>>                 DRM_ERROR("invalid edid input\n");
>> @@ -303,7 +303,12 @@ void dp_panel_handle_sink_request(struct dp_panel 
>> *dp_panel)
>>         panel = container_of(dp_panel, struct dp_panel_private, 
>> dp_panel);
>> 
>>         if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
>> -               u8 checksum = 
>> dp_panel_get_edid_checksum(dp_panel->edid);
>> +               u8 checksum;
>> +
>> +               if (dp_panel->edid)
>> +                       checksum = 
>> dp_panel_get_edid_checksum(dp_panel->edid);
>> +               else
>> +                       checksum = 
>> dp_panel->connector->real_edid_checksum;
>> 
>>                 dp_link_send_edid_checksum(panel->link, checksum);
> 
> It looks like this can be drm_dp_send_real_edid_checksum()? Then we
> don't have to look at the connector internals sometimes and can drop
> dp_panel_get_edid_checksum() entirely?
you still need to pass read edid checksum into 
drm_dp_send_real_edid_checksum().
> 
>>                 dp_link_send_test_response(panel->link);

[PATCH 6/7] drm/msm/dp: do not end dp link training until video is ready

[Kuogee Hsieh]

Initialize both pre-emphasis and voltage swing level to 0 before
start link training and do not end link training until video is
ready to reduce the period between end of link training and video
start to meet Link Layer CTS requirement.  This fixes Link Layer
CTS cases 4.3.2.1, 4.3.2.2, 4.3.2.3 and 4.3.2.4.

Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 6f8443d..0cb01a9 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1490,6 +1490,9 @@ static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
 
 	dp_ctrl_push_idle(&ctrl->dp_ctrl);
 
+	ctrl->link->phy_params.p_level = 0;
+	ctrl->link->phy_params.v_level = 0;
+
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
 	ret = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
@@ -1692,6 +1695,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 		ctrl->link->link_params.rate,
 		ctrl->link->link_params.num_lanes, ctrl->dp_ctrl.pixel_rate);
 
+	ctrl->link->phy_params.p_level = 0;
+	ctrl->link->phy_params.v_level = 0;
+
 	rc = dp_ctrl_enable_mainlink_clocks(ctrl);
 	if (rc)
 		return rc;
@@ -1751,17 +1757,19 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 	if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
 		return rc;
 
-	/* stop txing train pattern */
-	dp_ctrl_clear_training_pattern(ctrl);
+	if (rc == 0) {  /* link train successfully */
+		/*
+		 * do not stop train pattern here
+		 * stop link training at on_stream
+		 * to pass compliance test
+		 */
+	} else  {
+		/*
+		 * link training failed
+		 * end txing train pattern here
+		 */
+		dp_ctrl_clear_training_pattern(ctrl);
 
-	/*
-	 * keep transmitting idle pattern until video ready
-	 * to avoid main link from loss of sync
-	 */
-	if (rc == 0)  /* link train successfully */
-		dp_ctrl_push_idle(dp_ctrl);
-	else  {
-		/* link training failed */
 		dp_ctrl_deinitialize_mainlink(ctrl);
 		rc = -ECONNRESET;
 	}
@@ -1771,7 +1779,6 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 {
-	u32 rate = 0;
 	int ret = 0;
 	bool mainlink_ready = false;
 	struct dp_ctrl_private *ctrl;
@@ -1781,10 +1788,6 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 
-	rate = ctrl->panel->link_info.rate;
-
-	ctrl->link->link_params.rate = rate;
-	ctrl->link->link_params.num_lanes = ctrl->panel->link_info.num_lanes;
 	ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
 
 	DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
@@ -1799,6 +1802,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 		}
 	}
 
+	/* stop txing train pattern to end link training */
+	dp_ctrl_clear_training_pattern(ctrl);
+
 	ret = dp_ctrl_enable_stream_clocks(ctrl);
 	if (ret) {
 		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Kuogee Hsieh]

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>
---
 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];
+	u8 status;
+	int i;
+	int lane = ctrl->link->link_params.num_lanes;
+
+	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;
+	u8 cr_status[2];
+	int training_step = DP_TRAINING_NONE;
+
+	ret = dp_ctrl_setup_main_link(ctrl, cr_status, &training_step);
+
+	return ret;
+}
+
 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 */
+	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);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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.

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

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

[khsieh]

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

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

[Stephen Boyd]

Quoting khsieh@codeaurora.org (2021-07-09 10:16:52)
> 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?

Yes if it fixes the patch before this one it should be combined.