* [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo
@ 2020-10-30 23:22 Kuogee Hsieh
2020-11-02 20:59 ` Stephen Boyd
0 siblings, 1 reply; 3+ messages in thread
From: Kuogee Hsieh @ 2020-10-30 23:22 UTC (permalink / raw)
To: robdclark, sean, swboyd
Cc: tanmay, abhinavk, aravindh, khsieh, rnayak, airlied, daniel,
linux-arm-msm, dri-devel, freedreno, linux-kernel
DP compo phy have to be enable to start link training. When
link training failed phy need to be disabled so that next
link trainng can be proceed smoothly at next plug in. This
patch de initialize mainlink to disable phy if link training
failed. This prevent system crash due to
disp_cc_mdss_dp_link_intf_clk stuck at "off" state. This patch
also perform checking power_on flag at dp_display_enable() and
dp_display_disable() to avoid crashing when unplug cable while
display is off.
Fixes: fdaf9a5e3c15 (drm/msm/dp: fixes wrong connection state caused by failure of link train
Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 +++++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++++++++
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index cee161c8ecc6..904698dfc7f7 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1468,6 +1468,29 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
return ret;
}
+static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
+{
+ struct dp_io *dp_io;
+ struct phy *phy;
+ int ret = 0;
+
+ dp_io = &ctrl->parser->io;
+ phy = dp_io->phy;
+
+ dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
+
+ dp_catalog_ctrl_reset(ctrl->catalog);
+
+ ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
+ if (ret)
+ DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
+
+ phy_power_off(phy);
+ phy_exit(phy);
+
+ return -ECONNRESET;
+}
+
static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
{
int ret = 0;
@@ -1648,8 +1671,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
if (rc)
return rc;
- while (--link_train_max_retries &&
- !atomic_read(&ctrl->dp_ctrl.aborted)) {
+ while (--link_train_max_retries) {
rc = dp_ctrl_reinitialize_mainlink(ctrl);
if (rc) {
DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
@@ -1664,6 +1686,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
break;
} else if (training_step == DP_TRAINING_1) {
/* link train_1 failed */
+ if (!dp_catalog_hpd_get_state_status(ctrl->catalog))
+ break; /* link cable unplugged */
+
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) {
@@ -1683,6 +1708,9 @@ 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_hpd_get_state_status(ctrl->catalog))
+ break; /* link cable unplugged */
+
rc = dp_ctrl_link_lane_down_shift(ctrl);
if (rc < 0) {
/* end with failure */
@@ -1703,6 +1731,8 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
*/
if (rc == 0) /* link train successfully */
dp_ctrl_push_idle(dp_ctrl);
+ else
+ rc = dp_ctrl_deinitialize_mainlink(ctrl);
return rc;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3eb0d428abf7..13b66266cd69 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -529,6 +529,11 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
if (ret) { /* link train failed */
hpd->hpd_high = 0;
dp->hpd_state = ST_DISCONNECTED;
+
+ if (ret == -ECONNRESET) { /* cable unplugged */
+ dp->core_initialized = false;
+ }
+
} else {
/* start sentinel checking in case of missing uevent */
dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
@@ -794,6 +799,11 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
dp_display = g_dp_display;
+ if (dp_display->power_on) {
+ DRM_DEBUG_DP("Link already setup, return\n");
+ return 0;
+ }
+
rc = dp_ctrl_on_stream(dp->ctrl);
if (!rc)
dp_display->power_on = true;
@@ -826,6 +836,9 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
dp_display = g_dp_display;
+ if (!dp_display->power_on)
+ return -EINVAL;
+
/* wait only if audio was enabled */
if (dp_display->audio_enabled) {
if (!wait_for_completion_timeout(&dp->audio_comp,
base-commit: fd4a29bed29b3d8f15942fdf77e7a0a52796d836
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo
2020-10-30 23:22 [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo Kuogee Hsieh
@ 2020-11-02 20:59 ` Stephen Boyd
2020-11-03 17:34 ` khsieh
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2020-11-02 20:59 UTC (permalink / raw)
To: Kuogee Hsieh, robdclark, sean
Cc: tanmay, abhinavk, aravindh, khsieh, rnayak, airlied, daniel,
linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Kuogee Hsieh (2020-10-30 16:22:53)
> DP compo phy have to be enable to start link training. When
> link training failed phy need to be disabled so that next
> link trainng can be proceed smoothly at next plug in. This
s/trainng/training/
> patch de initialize mainlink to disable phy if link training
s/de/de-/
> failed. This prevent system crash due to
> disp_cc_mdss_dp_link_intf_clk stuck at "off" state. This patch
> also perform checking power_on flag at dp_display_enable() and
> dp_display_disable() to avoid crashing when unplug cable while
> display is off.
>
> Fixes: fdaf9a5e3c15 (drm/msm/dp: fixes wrong connection state caused by failure of link train
>
Drop newline please.
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
Can you send this as a patch series? There were three patches sent near
each other and presumably they're related.
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 +++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++++++++
> 2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index cee161c8ecc6..904698dfc7f7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1468,6 +1468,29 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl)
> return ret;
> }
>
> +static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl)
> +{
> + struct dp_io *dp_io;
> + struct phy *phy;
> + int ret = 0;
Please drop this initialization to 0.
> +
> + dp_io = &ctrl->parser->io;
> + phy = dp_io->phy;
> +
> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
As it's overwritten here.
> + if (ret)
> + DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
> +
> + phy_power_off(phy);
> + phy_exit(phy);
> +
> + return -ECONNRESET;
Isn't this an error for networking connections getting reset? Really it
should return 0 because it didn't fail.
> +}
> +
> static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
> {
> int ret = 0;
> @@ -1648,8 +1671,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> if (rc)
> return rc;
>
> - while (--link_train_max_retries &&
> - !atomic_read(&ctrl->dp_ctrl.aborted)) {
> + while (--link_train_max_retries) {
> rc = dp_ctrl_reinitialize_mainlink(ctrl);
> if (rc) {
> DRM_ERROR("Failed to reinitialize mainlink. rc=%d\n",
> @@ -1664,6 +1686,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> break;
> } else if (training_step == DP_TRAINING_1) {
> /* link train_1 failed */
> + if (!dp_catalog_hpd_get_state_status(ctrl->catalog))
> + break; /* link cable unplugged */
> +
> 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) {
> @@ -1683,6 +1708,9 @@ 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_hpd_get_state_status(ctrl->catalog))
Maybe make a function called dp_catalog_link_disconnected()? Then the
comment isn't needed.
> + break; /* link cable unplugged */
> +
> rc = dp_ctrl_link_lane_down_shift(ctrl);
> if (rc < 0) {
> /* end with failure */
> @@ -1703,6 +1731,8 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
> */
> if (rc == 0) /* link train successfully */
> dp_ctrl_push_idle(dp_ctrl);
> + else
> + rc = dp_ctrl_deinitialize_mainlink(ctrl);
So if it fails we deinitialize and then return success? Shouldn't we
keep the error code from the link train attempt instead of overwrite it
with (most likely) zero? I see that it returns -ECONNRESET but that's
really odd and seeing this code here means you have to look at the
function to figure out that it's still returning an error code. Please
don't do that, just ignore the error code from this function.
>
> return rc;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3eb0d428abf7..13b66266cd69 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -529,6 +529,11 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
> if (ret) { /* link train failed */
> hpd->hpd_high = 0;
> dp->hpd_state = ST_DISCONNECTED;
> +
> + if (ret == -ECONNRESET) { /* cable unplugged */
> + dp->core_initialized = false;
> + }
Style: Drop braces on single line if statements.
> +
> } else {
> /* start sentinel checking in case of missing uevent */
> dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
> @@ -794,6 +799,11 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>
> dp_display = g_dp_display;
>
> + if (dp_display->power_on) {
> + DRM_DEBUG_DP("Link already setup, return\n");
> + return 0;
> + }
> +
> rc = dp_ctrl_on_stream(dp->ctrl);
> if (!rc)
> dp_display->power_on = true;
> @@ -826,6 +836,9 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>
> dp_display = g_dp_display;
>
> + if (!dp_display->power_on)
> + return -EINVAL;
> +
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> if (!wait_for_completion_timeout(&dp->audio_comp,
>
> base-commit: fd4a29bed29b3d8f15942fdf77e7a0a52796d836
What is this commit?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo
2020-11-02 20:59 ` Stephen Boyd
@ 2020-11-03 17:34 ` khsieh
0 siblings, 0 replies; 3+ messages in thread
From: khsieh @ 2020-11-03 17:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: robdclark, sean, tanmay, abhinavk, aravindh, rnayak, airlied,
daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel
On 2020-11-02 12:59, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2020-10-30 16:22:53)
>> DP compo phy have to be enable to start link training. When
>> link training failed phy need to be disabled so that next
>> link trainng can be proceed smoothly at next plug in. This
>
> s/trainng/training/
>
>> patch de initialize mainlink to disable phy if link training
>
> s/de/de-/
>
>> failed. This prevent system crash due to
>> disp_cc_mdss_dp_link_intf_clk stuck at "off" state. This patch
>> also perform checking power_on flag at dp_display_enable() and
>> dp_display_disable() to avoid crashing when unplug cable while
>> display is off.
>>
>> Fixes: fdaf9a5e3c15 (drm/msm/dp: fixes wrong connection state caused
>> by failure of link train
>>
>
> Drop newline please.
>
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>
> Can you send this as a patch series? There were three patches sent near
> each other and presumably they're related.
>
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34
>> +++++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++++++++
>> 2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index cee161c8ecc6..904698dfc7f7 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1468,6 +1468,29 @@ static int dp_ctrl_reinitialize_mainlink(struct
>> dp_ctrl_private *ctrl)
>> return ret;
>> }
>>
>> +static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private
>> *ctrl)
>> +{
>> + struct dp_io *dp_io;
>> + struct phy *phy;
>> + int ret = 0;
>
> Please drop this initialization to 0.
>
>> +
>> + dp_io = &ctrl->parser->io;
>> + phy = dp_io->phy;
>> +
>> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>> +
>> + dp_catalog_ctrl_reset(ctrl->catalog);
>> +
>> + ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
>
> As it's overwritten here.
>
>> + if (ret)
>> + DRM_ERROR("Failed to disable link clocks. ret=%d\n",
>> ret);
>> +
>> + phy_power_off(phy);
>> + phy_exit(phy);
>> +
>> + return -ECONNRESET;
>
> Isn't this an error for networking connections getting reset? Really it
> should return 0 because it didn't fail.
>
>> +}
>> +
>> static int dp_ctrl_link_maintenance(struct dp_ctrl_private *ctrl)
>> {
>> int ret = 0;
>> @@ -1648,8 +1671,7 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> if (rc)
>> return rc;
>>
>> - while (--link_train_max_retries &&
>> - !atomic_read(&ctrl->dp_ctrl.aborted)) {
>> + while (--link_train_max_retries) {
>> rc = dp_ctrl_reinitialize_mainlink(ctrl);
>> if (rc) {
>> DRM_ERROR("Failed to reinitialize mainlink.
>> rc=%d\n",
>> @@ -1664,6 +1686,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> break;
>> } else if (training_step == DP_TRAINING_1) {
>> /* link train_1 failed */
>> + if
>> (!dp_catalog_hpd_get_state_status(ctrl->catalog))
>> + break; /* link cable
>> unplugged */
>> +
>> 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)
>> {
>> @@ -1683,6 +1708,9 @@ 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_hpd_get_state_status(ctrl->catalog))
>
> Maybe make a function called dp_catalog_link_disconnected()? Then the
> comment isn't needed.
>
>> + break; /* link cable
>> unplugged */
>> +
>> rc = dp_ctrl_link_lane_down_shift(ctrl);
>> if (rc < 0) {
>> /* end with failure */
>> @@ -1703,6 +1731,8 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
>> */
>> if (rc == 0) /* link train successfully */
>> dp_ctrl_push_idle(dp_ctrl);
>> + else
>> + rc = dp_ctrl_deinitialize_mainlink(ctrl);
>
> So if it fails we deinitialize and then return success? Shouldn't we
> keep the error code from the link train attempt instead of overwrite it
> with (most likely) zero? I see that it returns -ECONNRESET but that's
> really odd and seeing this code here means you have to look at the
> function to figure out that it's still returning an error code. Please
> don't do that, just ignore the error code from this function.
>
There are two possible failure cases at plugin request, link training
failed and read dpcd/edid failed.
It does not need to enable phy/pll to perform aux read/write from/to
dpcd or edid.
on the other hand, phy/pll need to be enabled to perform link training.
If link training failed,
then phy/pll need to be disabled so that phy/pll can be enabled next
link training correctly.
Link training failed error has to be propagated back to the top caller
so that dp_display_host_init()
will be called again at next plugin request.
>>
>> return rc;
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 3eb0d428abf7..13b66266cd69 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -529,6 +529,11 @@ static int dp_hpd_plug_handle(struct
>> dp_display_private *dp, u32 data)
>> if (ret) { /* link train failed */
>> hpd->hpd_high = 0;
>> dp->hpd_state = ST_DISCONNECTED;
>> +
>> + if (ret == -ECONNRESET) { /* cable unplugged */
>> + dp->core_initialized = false;
>> + }
>
> Style: Drop braces on single line if statements.
>
>> +
>> } else {
>> /* start sentinel checking in case of missing uevent
>> */
>> dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
>> @@ -794,6 +799,11 @@ static int dp_display_enable(struct
>> dp_display_private *dp, u32 data)
>>
>> dp_display = g_dp_display;
>>
>> + if (dp_display->power_on) {
>> + DRM_DEBUG_DP("Link already setup, return\n");
>> + return 0;
>> + }
>> +
>> rc = dp_ctrl_on_stream(dp->ctrl);
>> if (!rc)
>> dp_display->power_on = true;
>> @@ -826,6 +836,9 @@ static int dp_display_disable(struct
>> dp_display_private *dp, u32 data)
>>
>> dp_display = g_dp_display;
>>
>> + if (!dp_display->power_on)
>> + return -EINVAL;
>> +
>> /* wait only if audio was enabled */
>> if (dp_display->audio_enabled) {
>> if (!wait_for_completion_timeout(&dp->audio_comp,
>>
>> base-commit: fd4a29bed29b3d8f15942fdf77e7a0a52796d836
>
> What is this commit?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-03 17:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-30 23:22 [PATCH] drm/msm/dp: deinitialize mainlink if link training failedo Kuogee Hsieh
2020-11-02 20:59 ` Stephen Boyd
2020-11-03 17:34 ` khsieh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).