diff mbox series

[v5] drm/msm/dp: force link training for display resolution change

Message ID 1655156917-21726-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v5] drm/msm/dp: force link training for display resolution change | expand

Commit Message

Kuogee Hsieh June 13, 2022, 9:48 p.m. UTC
During display resolution changes display have to be disabled first
followed by display enabling with new resolution. Display disable
will turn off both pixel clock and main link clock so that main link
have to be re-trained during display enable to have new video stream
flow again. At current implementation, display enable function manually
kicks up irq_hpd_handle which will read panel link status and start link
training if link status is not in sync state. However, there is rare
case that a particular panel links status keep staying in sync for
some period of time after main link had been shut down previously at
display disabled. Main link retraining will not be executed by
irq_hdp_handle() if the link status read from panel shows it is in
sync state. If this was happen, then video stream of newer display
resolution will fail to be transmitted to panel due to main link is
not in sync between host and panel. This patch force main link always
be retrained during display enable procedure to prevent this rare
failed case from happening. Also this implementation are more
efficient than manual kicking off irq_hpd_handle function.

Changes in v2:
-- set force_link_train flag on DP only (is_edp == false)

Changes in v3:
-- revise commit  text
-- add Fixes tag

Changes in v4:
-- revise commit  text

Changes in v5:
-- fix spelling at commit text

Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  6 +++---
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
 drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
 3 files changed, 12 insertions(+), 11 deletions(-)

Comments

Stephen Boyd June 14, 2022, 8:38 a.m. UTC | #1
Quoting Kuogee Hsieh (2022-06-13 14:48:37)
> During display resolution changes display have to be disabled first
> followed by display enabling with new resolution. Display disable
> will turn off both pixel clock and main link clock so that main link
> have to be re-trained during display enable to have new video stream
> flow again. At current implementation, display enable function manually
> kicks up irq_hpd_handle which will read panel link status and start link
> training if link status is not in sync state. However, there is rare
> case that a particular panel links status keep staying in sync for
> some period of time after main link had been shut down previously at
> display disabled. Main link retraining will not be executed by
> irq_hdp_handle() if the link status read from panel shows it is in
> sync state. If this was happen, then video stream of newer display
> resolution will fail to be transmitted to panel due to main link is
> not in sync between host and panel. This patch force main link always
> be retrained during display enable procedure to prevent this rare
> failed case from happening. Also this implementation are more
> efficient than manual kicking off irq_hpd_handle function.

How is resolution change different from disabling and enabling the
display? The commit text talks about resolution changes, but the code
doesn't compare resolutions from before and after to know when to
retrain the link. Can the code be made to actually do what the commit
text says? It would be clearer if the code looked for actual resolution
changes instead of hooking the dp_bridge_enable() function.

>
> Changes in v2:
> -- set force_link_train flag on DP only (is_edp == false)
>
> Changes in v3:
> -- revise commit  text
> -- add Fixes tag
>
> Changes in v4:
> -- revise commit  text
>
> Changes in v5:
> -- fix spelling at commit text
>
> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    |  6 +++---
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>  drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
>  3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index af7a80c..bea93eb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>
>         ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>         if (!ret)
> -               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
> +               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);

Does this even matter if it's true or false? The 'sink_request' has
DP_TEST_LINK_PHY_TEST_PATTERN set from what I can tell, and then
dp_ctrl_on_stream() bails out before calling dp_ctrl_link_retrain()
anyway. It would be nice if we could split dp_ctrl_on_stream() so that
the part after the check for the sink request is a different function
that is called by dp_display.c and then this code can call the 'prepare'
function that does the first part. Then we can ignore the testing path
in the code, and possibly remove the conditional in dp_ctrl_on_stream()?

>         else
>                 DRM_ERROR("failed to enable DP link controller\n");
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c388323..370348d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>                 return 0;
>         }
>
> -       rc = dp_ctrl_on_stream(dp->ctrl);
> +       rc = dp_ctrl_on_stream(dp->ctrl, data);
>         if (!rc)
>                 dp_display->power_on = true;
>
> @@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>         int rc = 0;
>         struct dp_display_private *dp_display;
>         u32 state;
> +       bool force_link_train = false;
>
>         dp_display = container_of(dp, struct dp_display_private, dp_display);
>         if (!dp_display->dp_mode.drm_mode.clock) {
> @@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>
>         state =  dp_display->hpd_state;
>
> -       if (state == ST_DISPLAY_OFF)
> +       if (state == ST_DISPLAY_OFF) {
>                 dp_display_host_phy_init(dp_display);
>
> -       dp_display_enable(dp_display, 0);
> +               if (!dp->is_edp)

I didn't see any answer to my question about why edp is special on v4.
Can you at least add a comment to the code about why edp doesn't need to
unconditionally retrain, but DP does?

> +                       force_link_train = true;
> +       }
> +
> +       dp_display_enable(dp_display, force_link_train);
>
>         rc = dp_display_post_enable(dp);
>         if (rc) {
Kuogee Hsieh June 14, 2022, 3:27 p.m. UTC | #2
On 6/14/2022 1:38 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-13 14:48:37)
>> During display resolution changes display have to be disabled first
>> followed by display enabling with new resolution. Display disable
>> will turn off both pixel clock and main link clock so that main link
>> have to be re-trained during display enable to have new video stream
>> flow again. At current implementation, display enable function manually
>> kicks up irq_hpd_handle which will read panel link status and start link
>> training if link status is not in sync state. However, there is rare
>> case that a particular panel links status keep staying in sync for
>> some period of time after main link had been shut down previously at
>> display disabled. Main link retraining will not be executed by
>> irq_hdp_handle() if the link status read from panel shows it is in
>> sync state. If this was happen, then video stream of newer display
>> resolution will fail to be transmitted to panel due to main link is
>> not in sync between host and panel. This patch force main link always
>> be retrained during display enable procedure to prevent this rare
>> failed case from happening. Also this implementation are more
>> efficient than manual kicking off irq_hpd_handle function.
> How is resolution change different from disabling and enabling the
> display? The commit text talks about resolution changes, but the code
> doesn't compare resolutions from before and after to know when to
> retrain the link. Can the code be made to actually do what the commit
> text says? It would be clearer if the code looked for actual resolution
> changes instead of hooking the dp_bridge_enable() function.

Resolution changes are implemented through modeset. Therefore you can 
treat resolution changes equal to modeset.

dp_bridge_enable() and dp_bridge_disable() are end of function of 
modeset since DP driver is implemented as DRM bridge.

you have to use modeset to disable previous resolution and modeset to 
enable newer resolution.

Resolution changes is the term from end user point of view. For DP 
driver, we should use modeset.

I will revise the commit text for this.




>> Changes in v2:
>> -- set force_link_train flag on DP only (is_edp == false)
>>
>> Changes in v3:
>> -- revise commit  text
>> -- add Fixes tag
>>
>> Changes in v4:
>> -- revise commit  text
>>
>> Changes in v5:
>> -- fix spelling at commit text
>>
>> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c    |  6 +++---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>>   drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
>>   3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..bea93eb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>>
>>          ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>>          if (!ret)
>> -               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
>> +               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);
> Does this even matter if it's true or false? The 'sink_request' has
> DP_TEST_LINK_PHY_TEST_PATTERN set from what I can tell, and then
> dp_ctrl_on_stream() bails out before calling dp_ctrl_link_retrain()
> anyway. It would be nice if we could split dp_ctrl_on_stream() so that
> the part after the check for the sink request is a different function
> that is called by dp_display.c and then this code can call the 'prepare'
> function that does the first part. Then we can ignore the testing path
> in the code, and possibly remove the conditional in dp_ctrl_on_stream()?
>
>>          else
>>                  DRM_ERROR("failed to enable DP link controller\n");
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..370348d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>>                  return 0;
>>          }
>>
>> -       rc = dp_ctrl_on_stream(dp->ctrl);
>> +       rc = dp_ctrl_on_stream(dp->ctrl, data);
>>          if (!rc)
>>                  dp_display->power_on = true;
>>
>> @@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>          int rc = 0;
>>          struct dp_display_private *dp_display;
>>          u32 state;
>> +       bool force_link_train = false;
>>
>>          dp_display = container_of(dp, struct dp_display_private, dp_display);
>>          if (!dp_display->dp_mode.drm_mode.clock) {
>> @@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>
>>          state =  dp_display->hpd_state;
>>
>> -       if (state == ST_DISPLAY_OFF)
>> +       if (state == ST_DISPLAY_OFF) {
>>                  dp_display_host_phy_init(dp_display);
>>
>> -       dp_display_enable(dp_display, 0);
>> +               if (!dp->is_edp)
> I didn't see any answer to my question about why edp is special on v4.
> Can you at least add a comment to the code about why edp doesn't need to
> unconditionally retrain, but DP does?
>
>> +                       force_link_train = true;
>> +       }
>> +
>> +       dp_display_enable(dp_display, force_link_train);
>>
>>          rc = dp_display_post_enable(dp);
>>          if (rc) {
Kuogee Hsieh June 14, 2022, 8:27 p.m. UTC | #3
On 6/14/2022 1:38 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-13 14:48:37)
>> During display resolution changes display have to be disabled first
>> followed by display enabling with new resolution. Display disable
>> will turn off both pixel clock and main link clock so that main link
>> have to be re-trained during display enable to have new video stream
>> flow again. At current implementation, display enable function manually
>> kicks up irq_hpd_handle which will read panel link status and start link
>> training if link status is not in sync state. However, there is rare
>> case that a particular panel links status keep staying in sync for
>> some period of time after main link had been shut down previously at
>> display disabled. Main link retraining will not be executed by
>> irq_hdp_handle() if the link status read from panel shows it is in
>> sync state. If this was happen, then video stream of newer display
>> resolution will fail to be transmitted to panel due to main link is
>> not in sync between host and panel. This patch force main link always
>> be retrained during display enable procedure to prevent this rare
>> failed case from happening. Also this implementation are more
>> efficient than manual kicking off irq_hpd_handle function.
> How is resolution change different from disabling and enabling the
> display? The commit text talks about resolution changes, but the code
> doesn't compare resolutions from before and after to know when to
> retrain the link. Can the code be made to actually do what the commit
> text says? It would be clearer if the code looked for actual resolution
> changes instead of hooking the dp_bridge_enable() function.
>
>> Changes in v2:
>> -- set force_link_train flag on DP only (is_edp == false)
>>
>> Changes in v3:
>> -- revise commit  text
>> -- add Fixes tag
>>
>> Changes in v4:
>> -- revise commit  text
>>
>> Changes in v5:
>> -- fix spelling at commit text
>>
>> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.c    |  6 +++---
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>>   drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
>>   3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..bea93eb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>>
>>          ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>>          if (!ret)
>> -               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
>> +               ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);
> Does this even matter if it's true or false? The 'sink_request' has
> DP_TEST_LINK_PHY_TEST_PATTERN set from what I can tell, and then
> dp_ctrl_on_stream() bails out before calling dp_ctrl_link_retrain()
> anyway. It would be nice if we could split dp_ctrl_on_stream() so that
> the part after the check for the sink request is a different function
> that is called by dp_display.c and then this code can call the 'prepare'
> function that does the first part. Then we can ignore the testing path
> in the code, and possibly remove the conditional in dp_ctrl_on_stream()?
>
>>          else
>>                  DRM_ERROR("failed to enable DP link controller\n");
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..370348d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>>                  return 0;
>>          }
>>
>> -       rc = dp_ctrl_on_stream(dp->ctrl);
>> +       rc = dp_ctrl_on_stream(dp->ctrl, data);
>>          if (!rc)
>>                  dp_display->power_on = true;
>>
>> @@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>          int rc = 0;
>>          struct dp_display_private *dp_display;
>>          u32 state;
>> +       bool force_link_train = false;
>>
>>          dp_display = container_of(dp, struct dp_display_private, dp_display);
>>          if (!dp_display->dp_mode.drm_mode.clock) {
>> @@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>
>>          state =  dp_display->hpd_state;
>>
>> -       if (state == ST_DISPLAY_OFF)
>> +       if (state == ST_DISPLAY_OFF) {
>>                  dp_display_host_phy_init(dp_display);
>>
>> -       dp_display_enable(dp_display, 0);
>> +               if (!dp->is_edp)
> I didn't see any answer to my question about why edp is special on v4.
> Can you at least add a comment to the code about why edp doesn't need to
> unconditionally retrain, but DP does?

Sorry, missed this one.

This is my mistake, both DP and eDP are same. will remove is_edp flag 
checking.

>
>> +                       force_link_train = true;
>> +       }
>> +
>> +       dp_display_enable(dp_display, force_link_train);
>>
>>          rc = dp_display_post_enable(dp);
>>          if (rc) {
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index af7a80c..bea93eb 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1551,7 +1551,7 @@  static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
 
 	ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
 	if (!ret)
-		ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
+		ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);
 	else
 		DRM_ERROR("failed to enable DP link controller\n");
 
@@ -1807,7 +1807,7 @@  static int dp_ctrl_link_retrain(struct dp_ctrl_private *ctrl)
 	return dp_ctrl_setup_main_link(ctrl, &training_step);
 }
 
-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
 {
 	int ret = 0;
 	bool mainlink_ready = false;
@@ -1848,7 +1848,7 @@  int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
 		return 0;
 	}
 
-	if (!dp_ctrl_channel_eq_ok(ctrl))
+	if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl))
 		dp_ctrl_link_retrain(ctrl);
 
 	/* stop txing train pattern to end link training */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 0745fde..b563e2e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -21,7 +21,7 @@  struct dp_ctrl {
 };
 
 int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
-int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
+int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train);
 int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
 int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index c388323..370348d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -872,7 +872,7 @@  static int dp_display_enable(struct dp_display_private *dp, u32 data)
 		return 0;
 	}
 
-	rc = dp_ctrl_on_stream(dp->ctrl);
+	rc = dp_ctrl_on_stream(dp->ctrl, data);
 	if (!rc)
 		dp_display->power_on = true;
 
@@ -1654,6 +1654,7 @@  void dp_bridge_enable(struct drm_bridge *drm_bridge)
 	int rc = 0;
 	struct dp_display_private *dp_display;
 	u32 state;
+	bool force_link_train = false;
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 	if (!dp_display->dp_mode.drm_mode.clock) {
@@ -1688,10 +1689,14 @@  void dp_bridge_enable(struct drm_bridge *drm_bridge)
 
 	state =  dp_display->hpd_state;
 
-	if (state == ST_DISPLAY_OFF)
+	if (state == ST_DISPLAY_OFF) {
 		dp_display_host_phy_init(dp_display);
 
-	dp_display_enable(dp_display, 0);
+		if (!dp->is_edp)
+			force_link_train = true;
+	}
+
+	dp_display_enable(dp_display, force_link_train);
 
 	rc = dp_display_post_enable(dp);
 	if (rc) {
@@ -1700,10 +1705,6 @@  void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		dp_display_unprepare(dp);
 	}
 
-	/* manual kick off plug event to train link */
-	if (state == ST_DISPLAY_OFF)
-		dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0);
-
 	/* completed connection */
 	dp_display->hpd_state = ST_CONNECTED;