add fixes to pass DP Link Layer compliance test cases

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 <[email protected]>

---

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -1

index caf71fa..27fb0f0 100644
@@ -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;

Quoting Kuogee Hsieh (2021-07-06 10:20:14)

Quoted 8 lines

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 <[email protected]>

---

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.

Quoted 17 lines

 drivers/gpu/drm/msm/dp/dp_ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -1

index caf71fa..27fb0f0 100644
@@ -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;

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 ++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 30 deletions(-)

drivers/gpu/drm/msm/dp/dp_ctrl.c +9 -20

index 27fb0f0..6f8443d 100644
@@ -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];

drivers/gpu/drm/msm/dp/dp_ctrl.c +24 -0

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

drivers/gpu/drm/msm/dp/dp_ctrl.c +12 -10

+
 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

Quoting Kuogee Hsieh (2021-07-06 10:20:15)

Quoted 124 lines

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 86 
++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 30 deletions(-)

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -8

index 27fb0f0..6f8443d 100644
@@ -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];

drivers/gpu/drm/msm/dp/dp_ctrl.c +2 -2

@@ -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)) {

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -1

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

drivers/gpu/drm/msm/dp/dp_ctrl.c +0 -4

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

drivers/gpu/drm/msm/dp/dp_ctrl.c +2 -2

@@ -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)) {

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -1

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

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -3

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

Quoted 2 lines

        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?

Quoted 34 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +28 -0

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

drivers/gpu/drm/msm/dp/dp_ctrl.c void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl) · line 1659

Quoted 6 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +6 -0

+               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()?

Quoted 71 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -0

+
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
 {

        int rc = 0;

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -5

@@ -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

drivers/gpu/drm/msm/dp/dp_ctrl.c +6 -4

@@ -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

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +11 -0

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

Quoted 8 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl) · line 1659

Quoted 6 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +6 -0

+               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,

Quoted 2 lines

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?

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/msm/dp/dp_aux.c +0 -0

index 4a3293b..eb40d84 100644

drivers/gpu/drm/msm/dp/dp_aux.c +1 -1

+                       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

Quoting Kuogee Hsieh (2021-07-06 10:20:16)

Quoted 20 lines

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_aux.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/msm/dp/dp_aux.c +0 -0

index 4a3293b..eb40d84 100644
@@ -353,6 +353,9 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux 
 *dp_aux,
                        if (!(aux->retry_cnt % MAX_AUX_RETRIES))
 

drivers/gpu/drm/msm/dp/dp_aux.c static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, · line 357

Quoted 4 lines

drivers/gpu/drm/msm/dp/dp_aux.c +2 -0

 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?

Quoted 9 lines

drivers/gpu/drm/msm/dp/dp_aux.c +1 -0

+                       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

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 <[email protected]>

---

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/dp/dp_panel.c +1 -1

index 88196f7..0fdb551 100644
@@ -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);
 

drivers/gpu/drm/msm/dp/dp_panel.c +0 -0

                dp_link_send_test_response(panel->link);

Quoting Kuogee Hsieh (2021-07-06 10:20:18)

Quoted 39 lines

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_panel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/dp/dp_panel.c +1 -1

index 88196f7..0fdb551 100644
@@ -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");

drivers/gpu/drm/msm/dp/dp_panel.c +4 -1

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

drivers/gpu/drm/msm/dp/dp_panel.c +2 -0

+               else
+                       checksum = 

dp_panel->connector->real_edid_checksum;

Quoted 1 line

                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?

Quoted 2 lines

                dp_link_send_test_response(panel->link);

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 <[email protected]>

---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

drivers/gpu/drm/msm/dp/dp_ctrl.c +0 -0

index 0cb01a9..e616ab2 100644

drivers/gpu/drm/msm/dp/dp_ctrl.c +13 -0

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

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -0

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

Quoting Kuogee Hsieh (2021-07-06 10:20:20)

Quoted 7 lines

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.

Quoted 1 line

Signed-off-by Kuogee Hsieh <[email protected]>

I guess this is a fix for the original driver, so it should be tagged with Fixes appropriately.

Quoted 11 lines

---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 
++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

drivers/gpu/drm/msm/dp/dp_ctrl.c +0 -0

index 0cb01a9..e616ab2 100644

drivers/gpu/drm/msm/dp/dp_ctrl.c static bool dp_ctrl_any_lane_cr_lose(struct · line 1667

Quoted 7 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -0

@@ -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?

drivers/gpu/drm/msm/dp/dp_ctrl.c static bool dp_ctrl_any_lane_cr_lose(struct dp_ctrl_private *ctrl, · line 1669

Quoted 3 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -0

+       u8 status;
+       int i;
+       int lane = ctrl->link->link_params.num_lanes;

s/lane/num_lanes/

would make the code easier to read

Quoted 17 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +13 -0

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

drivers/gpu/drm/msm/dp/dp_ctrl.c int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl) · line 1801

Quoted 6 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -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.

drivers/gpu/drm/msm/dp/dp_ctrl.c int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)

Quoted 5 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +4 -0

+       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.

drivers/gpu/drm/msm/dp/dp_ctrl.c int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl) · line 1808

Quoted 2 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +2 -0

+
+       return ret;

And indeed, it could be 'return dp_ctrl_setup_main_link()' instead.

drivers/gpu/drm/msm/dp/dp_ctrl.c int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) · line 1835

Quoted 10 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +2 -0

+}
+
 int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 {

        int ret = 0;

drivers/gpu/drm/msm/dp/dp_ctrl.c +1 -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.

Quoted 6 lines

drivers/gpu/drm/msm/dp/dp_ctrl.c +3 -0

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

Quoted 8 lines

Quoting Kuogee Hsieh (2021-07-06 10:20:20)

Quoted 7 lines

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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Kuogee Hsieh <[email protected]>

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?