* [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
@ 2022-04-05 18:17 ` Kuogee Hsieh
0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-04-05 18:17 UTC (permalink / raw)
To: robdclark, sean, swboyd, vkoul, daniel, airlied, agross,
dmitry.baryshkov, bjorn.andersson
Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
quic_khsieh, quic_aravindh, freedreno, linux-kernel
HPD plugin handle is responsible for setting up main link and depend on
user space frame work to start video stream. Similarly, HPD unplugged
handle is responsible for tearing down main link and depend on user space
frame work to stop video stream. Connect_pending_timeout and disconnect_
pending_timeout are fired after 5 seconds timer expired to tear down main
link and video stream and restore DP driver state into known default
DISCONNECTED state in the case of frame work does not response uevent
original from DP driver so that DP driver can recover gracefully.
The original connect_pending_timeout and disconnect_pending_timeout were
not implemented correctly. This patch enhance both timeout functions to
tear down main link and video stream correctly once timer is fired.
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
3 files changed, 68 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index dcd0126..3f4cf6d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
-int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
@@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
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);
+ }
+
+ DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
+ phy_power_off(phy);
+
+ DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
+ return ret;
+}
+
+int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+ struct dp_io *dp_io;
+ struct phy *phy;
+ int ret = 0;
+
+ if (!dp_ctrl)
+ return -EINVAL;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ dp_io = &ctrl->parser->io;
+ phy = dp_io->phy;
+
+ dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
if (ret)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,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_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);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
void dp_ctrl_isr(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 178b774..56bf7c5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been setup but video is not ready yet
+ * only tear down main link
+ */
state = dp->hpd_state;
if (state == ST_CONNECT_PENDING) {
- dp->hpd_state = ST_CONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off_link(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);
@@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
if (dp->link->sink_count == 0) {
dp_display_host_phy_exit(dp);
}
+ dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
return 0;
}
- dp->hpd_state = ST_DISCONNECT_PENDING;
-
/* disable HPD plug interrupts */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
- /*
- * We don't need separate work for disconnect as
- * connect/attention interrupts are disabled
- */
dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
- /* start sentinel checking in case of missing uevent */
- dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ if (state == ST_DISPLAY_OFF) {
+ dp->hpd_state = ST_DISCONNECTED;
+
+ } else {
+ /* start sentinel checking in case of missing uevent */
+ dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ dp->hpd_state = ST_DISCONNECT_PENDING;
+ }
/* signal the disconnect event early to ensure proper teardown */
dp_display_handle_plugged_change(&dp->dp_display, false);
@@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been set up and video is ready
+ * tear down main link, video stream and phy
+ */
state = dp->hpd_state;
if (state == ST_DISCONNECT_PENDING) {
- dp->hpd_state = ST_DISCONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);
@@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
+ state = dp_display->hpd_state;
+ if (state == ST_DISCONNECTED) {
+ mutex_unlock(&dp_display->event_mutex);
+ return rc;
+ }
+
/* stop sentinel checking */
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
@@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
- state = dp_display->hpd_state;
-
if (state == ST_DISPLAY_OFF)
dp_display_host_phy_init(dp_display);
@@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
/* stop sentinel checking */
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
+ state = dp_display->hpd_state;
+ if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
+ mutex_unlock(&dp_display->event_mutex);
+ return rc;
+ }
+
dp_display_disable(dp_display, 0);
rc = dp_display_unprepare(dp);
if (rc)
DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
- state = dp_display->hpd_state;
if (state == ST_DISCONNECT_PENDING) {
/* completed disconnection */
dp_display->hpd_state = ST_DISCONNECTED;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
@ 2022-04-05 18:17 ` Kuogee Hsieh
0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-04-05 18:17 UTC (permalink / raw)
To: robdclark, sean, swboyd, vkoul, daniel, airlied, agross,
dmitry.baryshkov, bjorn.andersson
Cc: quic_abhinavk, quic_aravindh, quic_khsieh, quic_sbillaka,
freedreno, dri-devel, linux-arm-msm, linux-kernel
HPD plugin handle is responsible for setting up main link and depend on
user space frame work to start video stream. Similarly, HPD unplugged
handle is responsible for tearing down main link and depend on user space
frame work to stop video stream. Connect_pending_timeout and disconnect_
pending_timeout are fired after 5 seconds timer expired to tear down main
link and video stream and restore DP driver state into known default
DISCONNECTED state in the case of frame work does not response uevent
original from DP driver so that DP driver can recover gracefully.
The original connect_pending_timeout and disconnect_pending_timeout were
not implemented correctly. This patch enhance both timeout functions to
tear down main link and video stream correctly once timer is fired.
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
3 files changed, 68 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index dcd0126..3f4cf6d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
-int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
+int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
@@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
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);
+ }
+
+ DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
+ phy_power_off(phy);
+
+ DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
+ return ret;
+}
+
+int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+ struct dp_io *dp_io;
+ struct phy *phy;
+ int ret = 0;
+
+ if (!dp_ctrl)
+ return -EINVAL;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ dp_io = &ctrl->parser->io;
+ phy = dp_io->phy;
+
+ dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
if (ret)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2433edb..ffafe17 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -22,6 +22,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_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);
void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
void dp_ctrl_isr(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 178b774..56bf7c5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been setup but video is not ready yet
+ * only tear down main link
+ */
state = dp->hpd_state;
if (state == ST_CONNECT_PENDING) {
- dp->hpd_state = ST_CONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off_link(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);
@@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
if (dp->link->sink_count == 0) {
dp_display_host_phy_exit(dp);
}
+ dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
return 0;
}
- dp->hpd_state = ST_DISCONNECT_PENDING;
-
/* disable HPD plug interrupts */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
- /*
- * We don't need separate work for disconnect as
- * connect/attention interrupts are disabled
- */
dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
- /* start sentinel checking in case of missing uevent */
- dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ if (state == ST_DISPLAY_OFF) {
+ dp->hpd_state = ST_DISCONNECTED;
+
+ } else {
+ /* start sentinel checking in case of missing uevent */
+ dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
+ dp->hpd_state = ST_DISCONNECT_PENDING;
+ }
/* signal the disconnect event early to ensure proper teardown */
dp_display_handle_plugged_change(&dp->dp_display, false);
@@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
mutex_lock(&dp->event_mutex);
+ /*
+ * main link had been set up and video is ready
+ * tear down main link, video stream and phy
+ */
state = dp->hpd_state;
if (state == ST_DISCONNECT_PENDING) {
- dp->hpd_state = ST_DISCONNECTED;
DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
+ dp_ctrl_off(dp->ctrl);
+ dp_display_host_phy_exit(dp);
+ dp->hpd_state = ST_DISCONNECTED;
}
mutex_unlock(&dp->event_mutex);
@@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
+ state = dp_display->hpd_state;
+ if (state == ST_DISCONNECTED) {
+ mutex_unlock(&dp_display->event_mutex);
+ return rc;
+ }
+
/* stop sentinel checking */
dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
@@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
return rc;
}
- state = dp_display->hpd_state;
-
if (state == ST_DISPLAY_OFF)
dp_display_host_phy_init(dp_display);
@@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
/* stop sentinel checking */
dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
+ state = dp_display->hpd_state;
+ if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
+ mutex_unlock(&dp_display->event_mutex);
+ return rc;
+ }
+
dp_display_disable(dp_display, 0);
rc = dp_display_unprepare(dp);
if (rc)
DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
- state = dp_display->hpd_state;
if (state == ST_DISCONNECT_PENDING) {
/* completed disconnection */
dp_display->hpd_state = ST_DISCONNECTED;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
2022-04-05 18:17 ` Kuogee Hsieh
@ 2022-04-05 19:48 ` Stephen Boyd
-1 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2022-04-05 19:48 UTC (permalink / raw)
To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
linux-kernel, quic_aravindh, freedreno
Quoting Kuogee Hsieh (2022-04-05 11:17:07)
> HPD plugin handle is responsible for setting up main link and depend on
Is "HPD plugin handle" a function? Can you use the function name?
> user space frame work to start video stream. Similarly, HPD unplugged
> handle is responsible for tearing down main link and depend on user space
> frame work to stop video stream. Connect_pending_timeout and disconnect_
Is 'Connect_pending_timeout' actually 'dp_connect_pending_timeout()'? If
so, it would be clearer if the function name is used.
> pending_timeout are fired after 5 seconds timer expired to tear down main
> link and video stream and restore DP driver state into known default
> DISCONNECTED state in the case of frame work does not response uevent
s/frame work/framework/
s/response/respond to/
> original from DP driver so that DP driver can recover gracefully.
This part of the sentence doesn't make sense to me.
>
> The original connect_pending_timeout and disconnect_pending_timeout were
> not implemented correctly. This patch enhance both timeout functions to
s/enhance/fixes/
> tear down main link and video stream correctly once timer is fired.
>
> Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>
Nitpick: Drop newline.
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
> 3 files changed, 68 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dcd0126..3f4cf6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
> return ret;
> }
>
> -int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>
> 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);
> + }
> +
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
Please don't recast pointer prints with %x. Use %p to print pointers.
> +
> + phy_power_off(phy);
> +
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
It doesn't look good to be peeking inside struct phy. I wonder why that
isn't an opaque struct. Either way, please don't print the struct
members.
> +
> + return ret;
> +}
> +
> +int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + struct dp_io *dp_io;
> + struct phy *phy;
> + int ret = 0;
Drop useless assignment please
> +
> + if (!dp_ctrl)
How is this possible? Please remove useless checks.
> + return -EINVAL;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> + dp_io = &ctrl->parser->io;
> + phy = dp_io->phy;
> +
> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>
> ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
> if (ret)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 178b774..56bf7c5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
>
> mutex_lock(&dp->event_mutex);
>
> + /*
> + * main link had been setup but video is not ready yet
> + * only tear down main link
> + */
> state = dp->hpd_state;
> if (state == ST_CONNECT_PENDING) {
> - dp->hpd_state = ST_CONNECTED;
> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> + dp_ctrl_off_link(dp->ctrl);
> + dp_display_host_phy_exit(dp);
> + dp->hpd_state = ST_DISCONNECTED;
> }
>
> mutex_unlock(&dp->event_mutex);
> @@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> if (dp->link->sink_count == 0) {
> dp_display_host_phy_exit(dp);
> }
> + dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
The name of this function is very confusing. It would be nice to rename
it to something like dp_display_notify_disconnect() and skip
> mutex_unlock(&dp->event_mutex);
> return 0;
> }
> @@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> return 0;
> }
>
> - dp->hpd_state = ST_DISCONNECT_PENDING;
> -
> /* disable HPD plug interrupts */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>
> - /*
> - * We don't need separate work for disconnect as
> - * connect/attention interrupts are disabled
> - */
Why is this comment removed? Because a work is actually used? Why can't
we call dp_display_send_hpd_notification() directly?
> dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
>
> - /* start sentinel checking in case of missing uevent */
> - dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> + if (state == ST_DISPLAY_OFF) {
> + dp->hpd_state = ST_DISCONNECTED;
> +
Drop extra newline please
> + } else {
> + /* start sentinel checking in case of missing uevent */
> + dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> + dp->hpd_state = ST_DISCONNECT_PENDING;
It looks like dp_add_event() should check the event and change
dp->hpd_state sometimes. Then this code would be simply adding an event
and dp_add_event() would be changing the hpd_state while scheduling the
work.
> + }
>
> /* signal the disconnect event early to ensure proper teardown */
> dp_display_handle_plugged_change(&dp->dp_display, false);
> @@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
>
> mutex_lock(&dp->event_mutex);
>
> + /*
> + * main link had been set up and video is ready
> + * tear down main link, video stream and phy
because disconnect event never came and we need to get back to a default
sane state?
> + */
> state = dp->hpd_state;
> if (state == ST_DISCONNECT_PENDING) {
> - dp->hpd_state = ST_DISCONNECTED;
> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> + dp_ctrl_off(dp->ctrl);
> + dp_display_host_phy_exit(dp);
> + dp->hpd_state = ST_DISCONNECTED;
> }
>
> mutex_unlock(&dp->event_mutex);
> @@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>
> mutex_lock(&dp_display->event_mutex);
>
> + state = dp_display->hpd_state;
s/= /= /
Drop extra space after '=' please.
> + if (state == ST_DISCONNECTED) {
> + mutex_unlock(&dp_display->event_mutex);
> + return rc;
> + }
> +
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
>
> @@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> - state = dp_display->hpd_state;
> -
> if (state == ST_DISPLAY_OFF)
> dp_display_host_phy_init(dp_display);
>
> @@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>
> + state = dp_display->hpd_state;
s/= /= /
Drop extra space after '=' please.
> + if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
> + mutex_unlock(&dp_display->event_mutex);
> + return rc;
> + }
> +
> dp_display_disable(dp_display, 0);
>
> rc = dp_display_unprepare(dp);
> if (rc)
> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>
> - state = dp_display->hpd_state;
> if (state == ST_DISCONNECT_PENDING) {
> /* completed disconnection */
> dp_display->hpd_state = ST_DISCONNECTED;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
@ 2022-04-05 19:48 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2022-04-05 19:48 UTC (permalink / raw)
To: Kuogee Hsieh, agross, airlied, bjorn.andersson, daniel,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
Quoting Kuogee Hsieh (2022-04-05 11:17:07)
> HPD plugin handle is responsible for setting up main link and depend on
Is "HPD plugin handle" a function? Can you use the function name?
> user space frame work to start video stream. Similarly, HPD unplugged
> handle is responsible for tearing down main link and depend on user space
> frame work to stop video stream. Connect_pending_timeout and disconnect_
Is 'Connect_pending_timeout' actually 'dp_connect_pending_timeout()'? If
so, it would be clearer if the function name is used.
> pending_timeout are fired after 5 seconds timer expired to tear down main
> link and video stream and restore DP driver state into known default
> DISCONNECTED state in the case of frame work does not response uevent
s/frame work/framework/
s/response/respond to/
> original from DP driver so that DP driver can recover gracefully.
This part of the sentence doesn't make sense to me.
>
> The original connect_pending_timeout and disconnect_pending_timeout were
> not implemented correctly. This patch enhance both timeout functions to
s/enhance/fixes/
> tear down main link and video stream correctly once timer is fired.
>
> Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>
Nitpick: Drop newline.
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
> 3 files changed, 68 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dcd0126..3f4cf6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
> return ret;
> }
>
> -int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> @@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>
> 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);
> + }
> +
> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
Please don't recast pointer prints with %x. Use %p to print pointers.
> +
> + phy_power_off(phy);
> +
> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
It doesn't look good to be peeking inside struct phy. I wonder why that
isn't an opaque struct. Either way, please don't print the struct
members.
> +
> + return ret;
> +}
> +
> +int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + struct dp_io *dp_io;
> + struct phy *phy;
> + int ret = 0;
Drop useless assignment please
> +
> + if (!dp_ctrl)
How is this possible? Please remove useless checks.
> + return -EINVAL;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> + dp_io = &ctrl->parser->io;
> + phy = dp_io->phy;
> +
> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>
> ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
> if (ret)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 178b774..56bf7c5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
>
> mutex_lock(&dp->event_mutex);
>
> + /*
> + * main link had been setup but video is not ready yet
> + * only tear down main link
> + */
> state = dp->hpd_state;
> if (state == ST_CONNECT_PENDING) {
> - dp->hpd_state = ST_CONNECTED;
> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> + dp_ctrl_off_link(dp->ctrl);
> + dp_display_host_phy_exit(dp);
> + dp->hpd_state = ST_DISCONNECTED;
> }
>
> mutex_unlock(&dp->event_mutex);
> @@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> if (dp->link->sink_count == 0) {
> dp_display_host_phy_exit(dp);
> }
> + dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
The name of this function is very confusing. It would be nice to rename
it to something like dp_display_notify_disconnect() and skip
> mutex_unlock(&dp->event_mutex);
> return 0;
> }
> @@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> return 0;
> }
>
> - dp->hpd_state = ST_DISCONNECT_PENDING;
> -
> /* disable HPD plug interrupts */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>
> - /*
> - * We don't need separate work for disconnect as
> - * connect/attention interrupts are disabled
> - */
Why is this comment removed? Because a work is actually used? Why can't
we call dp_display_send_hpd_notification() directly?
> dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
>
> - /* start sentinel checking in case of missing uevent */
> - dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> + if (state == ST_DISPLAY_OFF) {
> + dp->hpd_state = ST_DISCONNECTED;
> +
Drop extra newline please
> + } else {
> + /* start sentinel checking in case of missing uevent */
> + dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> + dp->hpd_state = ST_DISCONNECT_PENDING;
It looks like dp_add_event() should check the event and change
dp->hpd_state sometimes. Then this code would be simply adding an event
and dp_add_event() would be changing the hpd_state while scheduling the
work.
> + }
>
> /* signal the disconnect event early to ensure proper teardown */
> dp_display_handle_plugged_change(&dp->dp_display, false);
> @@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
>
> mutex_lock(&dp->event_mutex);
>
> + /*
> + * main link had been set up and video is ready
> + * tear down main link, video stream and phy
because disconnect event never came and we need to get back to a default
sane state?
> + */
> state = dp->hpd_state;
> if (state == ST_DISCONNECT_PENDING) {
> - dp->hpd_state = ST_DISCONNECTED;
> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> + dp_ctrl_off(dp->ctrl);
> + dp_display_host_phy_exit(dp);
> + dp->hpd_state = ST_DISCONNECTED;
> }
>
> mutex_unlock(&dp->event_mutex);
> @@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>
> mutex_lock(&dp_display->event_mutex);
>
> + state = dp_display->hpd_state;
s/= /= /
Drop extra space after '=' please.
> + if (state == ST_DISCONNECTED) {
> + mutex_unlock(&dp_display->event_mutex);
> + return rc;
> + }
> +
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
>
> @@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> return rc;
> }
>
> - state = dp_display->hpd_state;
> -
> if (state == ST_DISPLAY_OFF)
> dp_display_host_phy_init(dp_display);
>
> @@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>
> + state = dp_display->hpd_state;
s/= /= /
Drop extra space after '=' please.
> + if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
> + mutex_unlock(&dp_display->event_mutex);
> + return rc;
> + }
> +
> dp_display_disable(dp_display, 0);
>
> rc = dp_display_unprepare(dp);
> if (rc)
> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>
> - state = dp_display->hpd_state;
> if (state == ST_DISCONNECT_PENDING) {
> /* completed disconnection */
> dp_display->hpd_state = ST_DISCONNECTED;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
2022-04-05 19:48 ` Stephen Boyd
@ 2022-04-06 21:19 ` Kuogee Hsieh
-1 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-04-06 21:19 UTC (permalink / raw)
To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_sbillaka, linux-arm-msm, quic_abhinavk, dri-devel,
linux-kernel, quic_aravindh, freedreno
On 4/5/2022 12:48 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-04-05 11:17:07)
>> HPD plugin handle is responsible for setting up main link and depend on
> Is "HPD plugin handle" a function? Can you use the function name?
>
>> user space frame work to start video stream. Similarly, HPD unplugged
>> handle is responsible for tearing down main link and depend on user space
>> frame work to stop video stream. Connect_pending_timeout and disconnect_
> Is 'Connect_pending_timeout' actually 'dp_connect_pending_timeout()'? If
> so, it would be clearer if the function name is used.
>
>> pending_timeout are fired after 5 seconds timer expired to tear down main
>> link and video stream and restore DP driver state into known default
>> DISCONNECTED state in the case of frame work does not response uevent
> s/frame work/framework/
> s/response/respond to/
>
>> original from DP driver so that DP driver can recover gracefully.
> This part of the sentence doesn't make sense to me.
>
>> The original connect_pending_timeout and disconnect_pending_timeout were
>> not implemented correctly. This patch enhance both timeout functions to
> s/enhance/fixes/
>
>> tear down main link and video stream correctly once timer is fired.
>>
>> Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>>
> Nitpick: Drop newline.
>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
>> drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
>> 3 files changed, 68 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index dcd0126..3f4cf6d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>> return ret;
>> }
>>
>> -int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
>> {
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> @@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>>
>> 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);
>> + }
>> +
>> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> Please don't recast pointer prints with %x. Use %p to print pointers.
>
>> +
>> + phy_power_off(phy);
>> +
>> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> It doesn't look good to be peeking inside struct phy. I wonder why that
> isn't an opaque struct. Either way, please don't print the struct
> members.
both init_count and power_count are very important to debug "clock stuck
at off" issues.
Can we keep them here?
>> +
>> + return ret;
>> +}
>> +
>> +int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> +{
>> + struct dp_ctrl_private *ctrl;
>> + struct dp_io *dp_io;
>> + struct phy *phy;
>> + int ret = 0;
> Drop useless assignment please
>
>> +
>> + if (!dp_ctrl)
> How is this possible? Please remove useless checks.
>
>> + return -EINVAL;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> + dp_io = &ctrl->parser->io;
>> + phy = dp_io->phy;
>> +
>> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>>
>> ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
>> if (ret)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 178b774..56bf7c5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
>>
>> mutex_lock(&dp->event_mutex);
>>
>> + /*
>> + * main link had been setup but video is not ready yet
>> + * only tear down main link
>> + */
>> state = dp->hpd_state;
>> if (state == ST_CONNECT_PENDING) {
>> - dp->hpd_state = ST_CONNECTED;
>> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
>> + dp_ctrl_off_link(dp->ctrl);
>> + dp_display_host_phy_exit(dp);
>> + dp->hpd_state = ST_DISCONNECTED;
>> }
>>
>> mutex_unlock(&dp->event_mutex);
>> @@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> if (dp->link->sink_count == 0) {
>> dp_display_host_phy_exit(dp);
>> }
>> + dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
> The name of this function is very confusing. It would be nice to rename
> it to something like dp_display_notify_disconnect() and skip
>
>> mutex_unlock(&dp->event_mutex);
>> return 0;
>> }
>> @@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> return 0;
>> }
>>
>> - dp->hpd_state = ST_DISCONNECT_PENDING;
>> -
>> /* disable HPD plug interrupts */
>> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>>
>> - /*
>> - * We don't need separate work for disconnect as
>> - * connect/attention interrupts are disabled
>> - */
> Why is this comment removed? Because a work is actually used? Why can't
> we call dp_display_send_hpd_notification() directly?
There may have circular locking issue since we have hold event_mutex
already.
>
>> dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
>>
>> - /* start sentinel checking in case of missing uevent */
>> - dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>> + if (state == ST_DISPLAY_OFF) {
>> + dp->hpd_state = ST_DISCONNECTED;
>> +
> Drop extra newline please
>
>> + } else {
>> + /* start sentinel checking in case of missing uevent */
>> + dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>> + dp->hpd_state = ST_DISCONNECT_PENDING;
> It looks like dp_add_event() should check the event and change
> dp->hpd_state sometimes. Then this code would be simply adding an event
> and dp_add_event() would be changing the hpd_state while scheduling the
> work.
this will increase dp_add_event() burdens a lots since it has to looking
into each event.
Also not every hpd_state change go through dp_add_event.
>
>> + }
>>
>> /* signal the disconnect event early to ensure proper teardown */
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>> @@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
>>
>> mutex_lock(&dp->event_mutex);
>>
>> + /*
>> + * main link had been set up and video is ready
>> + * tear down main link, video stream and phy
> because disconnect event never came and we need to get back to a default
> sane state?
yes, the default state is DISCONNECTED, therefore we better stop video
and tear down main link.
>
>> + */
>> state = dp->hpd_state;
>> if (state == ST_DISCONNECT_PENDING) {
>> - dp->hpd_state = ST_DISCONNECTED;
>> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
>> + dp_ctrl_off(dp->ctrl);
>> + dp_display_host_phy_exit(dp);
>> + dp->hpd_state = ST_DISCONNECTED;
>> }
>>
>> mutex_unlock(&dp->event_mutex);
>> @@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>>
>> mutex_lock(&dp_display->event_mutex);
>>
>> + state = dp_display->hpd_state;
> s/= /= /
>
> Drop extra space after '=' please.
>
>> + if (state == ST_DISCONNECTED) {
>> + mutex_unlock(&dp_display->event_mutex);
>> + return rc;
>> + }
>> +
>> /* stop sentinel checking */
>> dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
>>
>> @@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>> return rc;
>> }
>>
>> - state = dp_display->hpd_state;
>> -
>> if (state == ST_DISPLAY_OFF)
>> dp_display_host_phy_init(dp_display);
>>
>> @@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>> /* stop sentinel checking */
>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>>
>> + state = dp_display->hpd_state;
> s/= /= /
>
> Drop extra space after '=' please.
>
>> + if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
>> + mutex_unlock(&dp_display->event_mutex);
>> + return rc;
>> + }
>> +
>> dp_display_disable(dp_display, 0);
>>
>> rc = dp_display_unprepare(dp);
>> if (rc)
>> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>>
>> - state = dp_display->hpd_state;
>> if (state == ST_DISCONNECT_PENDING) {
>> /* completed disconnection */
>> dp_display->hpd_state = ST_DISCONNECTED;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
@ 2022-04-06 21:19 ` Kuogee Hsieh
0 siblings, 0 replies; 6+ messages in thread
From: Kuogee Hsieh @ 2022-04-06 21:19 UTC (permalink / raw)
To: Stephen Boyd, agross, airlied, bjorn.andersson, daniel,
dmitry.baryshkov, robdclark, sean, vkoul
Cc: quic_abhinavk, quic_aravindh, quic_sbillaka, freedreno,
dri-devel, linux-arm-msm, linux-kernel
On 4/5/2022 12:48 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-04-05 11:17:07)
>> HPD plugin handle is responsible for setting up main link and depend on
> Is "HPD plugin handle" a function? Can you use the function name?
>
>> user space frame work to start video stream. Similarly, HPD unplugged
>> handle is responsible for tearing down main link and depend on user space
>> frame work to stop video stream. Connect_pending_timeout and disconnect_
> Is 'Connect_pending_timeout' actually 'dp_connect_pending_timeout()'? If
> so, it would be clearer if the function name is used.
>
>> pending_timeout are fired after 5 seconds timer expired to tear down main
>> link and video stream and restore DP driver state into known default
>> DISCONNECTED state in the case of frame work does not response uevent
> s/frame work/framework/
> s/response/respond to/
>
>> original from DP driver so that DP driver can recover gracefully.
> This part of the sentence doesn't make sense to me.
>
>> The original connect_pending_timeout and disconnect_pending_timeout were
>> not implemented correctly. This patch enhance both timeout functions to
> s/enhance/fixes/
>
>> tear down main link and video stream correctly once timer is fired.
>>
>> Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>>
> Nitpick: Drop newline.
>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 34 ++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
>> drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
>> 3 files changed, 68 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index dcd0126..3f4cf6d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>> return ret;
>> }
>>
>> -int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
>> {
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> @@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>>
>> 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);
>> + }
>> +
>> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> Please don't recast pointer prints with %x. Use %p to print pointers.
>
>> +
>> + phy_power_off(phy);
>> +
>> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
> It doesn't look good to be peeking inside struct phy. I wonder why that
> isn't an opaque struct. Either way, please don't print the struct
> members.
both init_count and power_count are very important to debug "clock stuck
at off" issues.
Can we keep them here?
>> +
>> + return ret;
>> +}
>> +
>> +int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> +{
>> + struct dp_ctrl_private *ctrl;
>> + struct dp_io *dp_io;
>> + struct phy *phy;
>> + int ret = 0;
> Drop useless assignment please
>
>> +
>> + if (!dp_ctrl)
> How is this possible? Please remove useless checks.
>
>> + return -EINVAL;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> + dp_io = &ctrl->parser->io;
>> + phy = dp_io->phy;
>> +
>> + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>>
>> ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
>> if (ret)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 178b774..56bf7c5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
>>
>> mutex_lock(&dp->event_mutex);
>>
>> + /*
>> + * main link had been setup but video is not ready yet
>> + * only tear down main link
>> + */
>> state = dp->hpd_state;
>> if (state == ST_CONNECT_PENDING) {
>> - dp->hpd_state = ST_CONNECTED;
>> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
>> + dp_ctrl_off_link(dp->ctrl);
>> + dp_display_host_phy_exit(dp);
>> + dp->hpd_state = ST_DISCONNECTED;
>> }
>>
>> mutex_unlock(&dp->event_mutex);
>> @@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> if (dp->link->sink_count == 0) {
>> dp_display_host_phy_exit(dp);
>> }
>> + dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
> The name of this function is very confusing. It would be nice to rename
> it to something like dp_display_notify_disconnect() and skip
>
>> mutex_unlock(&dp->event_mutex);
>> return 0;
>> }
>> @@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> return 0;
>> }
>>
>> - dp->hpd_state = ST_DISCONNECT_PENDING;
>> -
>> /* disable HPD plug interrupts */
>> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>>
>> - /*
>> - * We don't need separate work for disconnect as
>> - * connect/attention interrupts are disabled
>> - */
> Why is this comment removed? Because a work is actually used? Why can't
> we call dp_display_send_hpd_notification() directly?
There may have circular locking issue since we have hold event_mutex
already.
>
>> dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
>>
>> - /* start sentinel checking in case of missing uevent */
>> - dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>> + if (state == ST_DISPLAY_OFF) {
>> + dp->hpd_state = ST_DISCONNECTED;
>> +
> Drop extra newline please
>
>> + } else {
>> + /* start sentinel checking in case of missing uevent */
>> + dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>> + dp->hpd_state = ST_DISCONNECT_PENDING;
> It looks like dp_add_event() should check the event and change
> dp->hpd_state sometimes. Then this code would be simply adding an event
> and dp_add_event() would be changing the hpd_state while scheduling the
> work.
this will increase dp_add_event() burdens a lots since it has to looking
into each event.
Also not every hpd_state change go through dp_add_event.
>
>> + }
>>
>> /* signal the disconnect event early to ensure proper teardown */
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>> @@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
>>
>> mutex_lock(&dp->event_mutex);
>>
>> + /*
>> + * main link had been set up and video is ready
>> + * tear down main link, video stream and phy
> because disconnect event never came and we need to get back to a default
> sane state?
yes, the default state is DISCONNECTED, therefore we better stop video
and tear down main link.
>
>> + */
>> state = dp->hpd_state;
>> if (state == ST_DISCONNECT_PENDING) {
>> - dp->hpd_state = ST_DISCONNECTED;
>> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
>> + dp_ctrl_off(dp->ctrl);
>> + dp_display_host_phy_exit(dp);
>> + dp->hpd_state = ST_DISCONNECTED;
>> }
>>
>> mutex_unlock(&dp->event_mutex);
>> @@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>>
>> mutex_lock(&dp_display->event_mutex);
>>
>> + state = dp_display->hpd_state;
> s/= /= /
>
> Drop extra space after '=' please.
>
>> + if (state == ST_DISCONNECTED) {
>> + mutex_unlock(&dp_display->event_mutex);
>> + return rc;
>> + }
>> +
>> /* stop sentinel checking */
>> dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
>>
>> @@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>> return rc;
>> }
>>
>> - state = dp_display->hpd_state;
>> -
>> if (state == ST_DISPLAY_OFF)
>> dp_display_host_phy_init(dp_display);
>>
>> @@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>> /* stop sentinel checking */
>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>>
>> + state = dp_display->hpd_state;
> s/= /= /
>
> Drop extra space after '=' please.
>
>> + if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
>> + mutex_unlock(&dp_display->event_mutex);
>> + return rc;
>> + }
>> +
>> dp_display_disable(dp_display, 0);
>>
>> rc = dp_display_unprepare(dp);
>> if (rc)
>> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>>
>> - state = dp_display->hpd_state;
>> if (state == ST_DISCONNECT_PENDING) {
>> /* completed disconnection */
>> dp_display->hpd_state = ST_DISCONNECTED;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-06 21:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 18:17 [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle Kuogee Hsieh
2022-04-05 18:17 ` Kuogee Hsieh
2022-04-05 19:48 ` Stephen Boyd
2022-04-05 19:48 ` Stephen Boyd
2022-04-06 21:19 ` Kuogee Hsieh
2022-04-06 21:19 ` Kuogee Hsieh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.