[PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

dp_hpd_plug_handle() is responsible for setting up main link and send
uevent to notify user space framework to start video stream. Similarly,
dp_hdp_unplug_handle is responsible to send uevent to notify user space
framework to stop video stream and then tear down main link.
However there are rare cases, such as in the middle of system suspending,
that uevent could not be delivered to user space framework. Therefore
some kind of recover mechanism armed by timer need to be in place in the
case of user space framework does not respond to uevent.

This patch have both dp_conenct_pending_timeout and
dp_disconnect_pending_timeout are used to stop video stream and tear down
main link and eventually restore DP driver state to known default
DISCONNECTED state in the case of timer fired due to framework does not
respond to uevent so that DP driver can recover itself gracefully at next
dongle unplug followed by plugin event.

Changes in v2:
-- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect

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    | 36 ++++++++++++++++++++-----
 drivers/gpu/drm/msm/dp/dp_ctrl.h    |  1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 54 ++++++++++++++++++++++++++++---------
 3 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index dcd0126..48990fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1910,15 +1910,12 @@ 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;
 	struct phy *phy;
-	int ret = 0;
-
-	if (!dp_ctrl)
-		return -EINVAL;
+	int ret;
 
 	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
 	dp_io = &ctrl->parser->io;
@@ -1926,7 +1923,34 @@ 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=%p init_count=%d power_on=%d\n",
+		phy, phy->init_count, phy->power_count);
+
+	phy_power_off(phy);
+
+	DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
+		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;
+
+	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..a6200a5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
 
 static int dp_display_usbpd_disconnect_cb(struct device *dev)
 {
+	return 0;
+}
+
+static void dp_display_notify_disconnect(struct device *dev)
+{
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
-
-	return 0;
 }
 
 static void dp_display_handle_video_request(struct dp_display_private *dp)
@@ -593,10 +596,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 +654,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_notify_disconnect(&dp->pdev->dev);
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -661,19 +671,22 @@ 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);
+	*/
+	dp_display_notify_disconnect(&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 +708,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 +1590,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 +1613,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 +1661,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

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Dmitry Baryshkov]

On 07/04/2022 00:28, Kuogee Hsieh wrote:
> dp_hpd_plug_handle() is responsible for setting up main link and send
> uevent to notify user space framework to start video stream. Similarly,
> dp_hdp_unplug_handle is responsible to send uevent to notify user space
> framework to stop video stream and then tear down main link.
> However there are rare cases, such as in the middle of system suspending,
> that uevent could not be delivered to user space framework. Therefore
> some kind of recover mechanism armed by timer need to be in place in the
> case of user space framework does not respond to uevent.

Hmm, how does userpsace 'respond' to the uevent? The driver should send 
hotplug notifications to userspace, but it must not expect any 
particular reaction. The userspace might be as simple, as fbdev 
emulation, but the driver still should function correctly.

> This patch have both dp_conenct_pending_timeout and
> dp_disconnect_pending_timeout are used to stop video stream and tear down
> main link and eventually restore DP driver state to known default
> DISCONNECTED state in the case of timer fired due to framework does not
> respond to uevent so that DP driver can recover itself gracefully at next
> dongle unplug followed by plugin event.
> 
> Changes in v2:
> -- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect
> 
> 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    | 36 ++++++++++++++++++++-----
>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  1 +
>   drivers/gpu/drm/msm/dp/dp_display.c | 54 ++++++++++++++++++++++++++++---------
>   3 files changed, 72 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dcd0126..48990fb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1910,15 +1910,12 @@ 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;
>   	struct phy *phy;
> -	int ret = 0;
> -
> -	if (!dp_ctrl)
> -		return -EINVAL;
> +	int ret;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   	dp_io = &ctrl->parser->io;
> @@ -1926,7 +1923,34 @@ 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=%p init_count=%d power_on=%d\n",
> +		phy, phy->init_count, phy->power_count);
> +
> +	phy_power_off(phy);
> +
> +	DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
> +		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;
> +
> +	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..a6200a5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>   
>   static int dp_display_usbpd_disconnect_cb(struct device *dev)
>   {
> +	return 0;
> +}
> +
> +static void dp_display_notify_disconnect(struct device *dev)
> +{
>   	struct dp_display_private *dp = dev_get_dp_display_private(dev);
>   
>   	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> -
> -	return 0;
>   }
>   
>   static void dp_display_handle_video_request(struct dp_display_private *dp)
> @@ -593,10 +596,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 +654,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_notify_disconnect(&dp->pdev->dev);
>   		mutex_unlock(&dp->event_mutex);
>   		return 0;
>   	}
> @@ -661,19 +671,22 @@ 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);
> +	*/
> +	dp_display_notify_disconnect(&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 +708,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 +1590,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 +1613,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 +1661,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;


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
> On 07/04/2022 00:28, Kuogee Hsieh wrote:
>> dp_hpd_plug_handle() is responsible for setting up main link and send
>> uevent to notify user space framework to start video stream. Similarly,
>> dp_hdp_unplug_handle is responsible to send uevent to notify user space
>> framework to stop video stream and then tear down main link.
>> However there are rare cases, such as in the middle of system 
>> suspending,
>> that uevent could not be delivered to user space framework. Therefore
>> some kind of recover mechanism armed by timer need to be in place in the
>> case of user space framework does not respond to uevent.
>
> Hmm, how does userpsace 'respond' to the uevent? The driver should 
> send hotplug notifications to userspace, but it must not expect any 
> particular reaction. The userspace might be as simple, as fbdev 
> emulation, but the driver still should function correctly.

yes, driver is function correctly by setting up main link. but it does 
not know which resolution to display.

It send hotplug notification through uevent to framework after main link 
is ready.

Framework  is responsible to set up MDP timing engine to start video stream.


However it does not know which


>
>> This patch have both dp_conenct_pending_timeout and
>> dp_disconnect_pending_timeout are used to stop video stream and tear 
>> down
>> main link and eventually restore DP driver state to known default
>> DISCONNECTED state in the case of timer fired due to framework does not
>> respond to uevent so that DP driver can recover itself gracefully at 
>> next
>> dongle unplug followed by plugin event.
>>
>> Changes in v2:
>> -- replace dp_display_usbpd_disconnect_cb with 
>> dp_display_notify_disconnect
>>
>> 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    | 36 ++++++++++++++++++++-----
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  1 +
>>   drivers/gpu/drm/msm/dp/dp_display.c | 54 
>> ++++++++++++++++++++++++++++---------
>>   3 files changed, 72 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index dcd0126..48990fb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1910,15 +1910,12 @@ 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;
>>       struct phy *phy;
>> -    int ret = 0;
>> -
>> -    if (!dp_ctrl)
>> -        return -EINVAL;
>> +    int ret;
>>         ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>       dp_io = &ctrl->parser->io;
>> @@ -1926,7 +1923,34 @@ 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=%p init_count=%d power_on=%d\n",
>> +        phy, phy->init_count, phy->power_count);
>> +
>> +    phy_power_off(phy);
>> +
>> +    DRM_DEBUG_DP("After, phy=%p init_count=%d power_on=%d\n",
>> +        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;
>> +
>> +    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..a6200a5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct 
>> device *dev)
>>     static int dp_display_usbpd_disconnect_cb(struct device *dev)
>>   {
>> +    return 0;
>> +}
>> +
>> +static void dp_display_notify_disconnect(struct device *dev)
>> +{
>>       struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>         dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>> -
>> -    return 0;
>>   }
>>     static void dp_display_handle_video_request(struct 
>> dp_display_private *dp)
>> @@ -593,10 +596,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 +654,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_notify_disconnect(&dp->pdev->dev);
>>           mutex_unlock(&dp->event_mutex);
>>           return 0;
>>       }
>> @@ -661,19 +671,22 @@ 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);
>> +    */
>> +    dp_display_notify_disconnect(&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 +708,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 +1590,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 +1613,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 +1661,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;
>
>

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Dmitry Baryshkov]

On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
> > On 07/04/2022 00:28, Kuogee Hsieh wrote:
> >> dp_hpd_plug_handle() is responsible for setting up main link and send
> >> uevent to notify user space framework to start video stream. Similarly,
> >> dp_hdp_unplug_handle is responsible to send uevent to notify user space
> >> framework to stop video stream and then tear down main link.
> >> However there are rare cases, such as in the middle of system
> >> suspending,
> >> that uevent could not be delivered to user space framework. Therefore
> >> some kind of recover mechanism armed by timer need to be in place in the
> >> case of user space framework does not respond to uevent.
> >
> > Hmm, how does userpsace 'respond' to the uevent? The driver should
> > send hotplug notifications to userspace, but it must not expect any
> > particular reaction. The userspace might be as simple, as fbdev
> > emulation, but the driver still should function correctly.
>
> yes, driver is function correctly by setting up main link. but it does
> not know which resolution to display.
>
> It send hotplug notification through uevent to framework after main link
> is ready.
>
> Framework  is responsible to set up MDP timing engine to start video stream.
>
>
> However it does not know which

It's of no concern to the driver. It is completely the userspace
problem. After resuming, it should reread available video output
properties. The display could have been changed while the system is
suspended.
From your description I still do not understand why you need the
'recovery' mechanism.

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Stephen Boyd]

The subject is still misleading. It is fixing something. It may be
enhancing it as well but it is clearly fixing it first.

Quoting Kuogee Hsieh (2022-04-06 14:28:13)
> dp_hpd_plug_handle() is responsible for setting up main link and send
> uevent to notify user space framework to start video stream. Similarly,
> dp_hdp_unplug_handle is responsible to send uevent to notify user space
> framework to stop video stream and then tear down main link.
> However there are rare cases, such as in the middle of system suspending,
> that uevent could not be delivered to user space framework. Therefore
> some kind of recover mechanism armed by timer need to be in place in the
> case of user space framework does not respond to uevent.
>
> This patch have both dp_conenct_pending_timeout and
> dp_disconnect_pending_timeout are used to stop video stream and tear down
> main link and eventually restore DP driver state to known default
> DISCONNECTED state in the case of timer fired due to framework does not
> respond to uevent so that DP driver can recover itself gracefully at next
> dongle unplug followed by plugin event.
>
> Changes in v2:
> -- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect

I'd prefer this part to be a different patch. It can come after the fix
to ease backporting.

Also, is there any response to Dmitry's question yet? I haven't seen
anything.

> 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..a6200a5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>
>  static int dp_display_usbpd_disconnect_cb(struct device *dev)

We shouldn't need to keep around an empty function.

>  {
> +       return 0;
> +}
> +
> +static void dp_display_notify_disconnect(struct device *dev)
> +{
>         struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
>         dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> -
> -       return 0;
>  }
>
>  static void dp_display_handle_video_request(struct dp_display_private *dp)
> @@ -593,10 +596,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 +654,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_notify_disconnect(&dp->pdev->dev);
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
>         }
> @@ -661,19 +671,22 @@ 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);
> +       */

This comment end is wrong. It should be unchanged.

> +       dp_display_notify_disconnect(&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 +708,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);

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/13/2022 5:02 PM, Stephen Boyd wrote:
> The subject is still misleading. It is fixing something. It may be
> enhancing it as well but it is clearly fixing it first.
>
> Quoting Kuogee Hsieh (2022-04-06 14:28:13)
>> dp_hpd_plug_handle() is responsible for setting up main link and send
>> uevent to notify user space framework to start video stream. Similarly,
>> dp_hdp_unplug_handle is responsible to send uevent to notify user space
>> framework to stop video stream and then tear down main link.
>> However there are rare cases, such as in the middle of system suspending,
>> that uevent could not be delivered to user space framework. Therefore
>> some kind of recover mechanism armed by timer need to be in place in the
>> case of user space framework does not respond to uevent.
>>
>> This patch have both dp_conenct_pending_timeout and
>> dp_disconnect_pending_timeout are used to stop video stream and tear down
>> main link and eventually restore DP driver state to known default
>> DISCONNECTED state in the case of timer fired due to framework does not
>> respond to uevent so that DP driver can recover itself gracefully at next
>> dongle unplug followed by plugin event.
>>
>> Changes in v2:
>> -- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect
> I'd prefer this part to be a different patch. It can come after the fix
> to ease backporting.
>
> Also, is there any response to Dmitry's question yet? I haven't seen
> anything.

Sorry, since our internal review does not like this approach.

I will upload new patch for review soon.

>> 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..a6200a5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>>
>>   static int dp_display_usbpd_disconnect_cb(struct device *dev)
> We shouldn't need to keep around an empty function.
>
>>   {
>> +       return 0;
>> +}
>> +
>> +static void dp_display_notify_disconnect(struct device *dev)
>> +{
>>          struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>
>>          dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>> -
>> -       return 0;
>>   }
>>
>>   static void dp_display_handle_video_request(struct dp_display_private *dp)
>> @@ -593,10 +596,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 +654,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_notify_disconnect(&dp->pdev->dev);
>>                  mutex_unlock(&dp->event_mutex);
>>                  return 0;
>>          }
>> @@ -661,19 +671,22 @@ 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);
>> +       */
> This comment end is wrong. It should be unchanged.
>
>> +       dp_display_notify_disconnect(&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 +708,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);

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-04-14 09:34:55)
>
> On 4/13/2022 5:02 PM, Stephen Boyd wrote:
> > The subject is still misleading. It is fixing something. It may be
> > enhancing it as well but it is clearly fixing it first.
> >
[...]
> > I'd prefer this part to be a different patch. It can come after the fix
> > to ease backporting.
> >
> > Also, is there any response to Dmitry's question yet? I haven't seen
> > anything.
>
> Sorry, since our internal review does not like this approach.

The internal review shouldn't prevent you from responding to code review
on the mailing list.

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote:
> On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
>>> On 07/04/2022 00:28, Kuogee Hsieh wrote:
>>>> dp_hpd_plug_handle() is responsible for setting up main link and send
>>>> uevent to notify user space framework to start video stream. Similarly,
>>>> dp_hdp_unplug_handle is responsible to send uevent to notify user space
>>>> framework to stop video stream and then tear down main link.
>>>> However there are rare cases, such as in the middle of system
>>>> suspending,
>>>> that uevent could not be delivered to user space framework. Therefore
>>>> some kind of recover mechanism armed by timer need to be in place in the
>>>> case of user space framework does not respond to uevent.
>>> Hmm, how does userpsace 'respond' to the uevent? The driver should
>>> send hotplug notifications to userspace, but it must not expect any
>>> particular reaction. The userspace might be as simple, as fbdev
>>> emulation, but the driver still should function correctly.
>> yes, driver is function correctly by setting up main link. but it does
>> not know which resolution to display.
>>
>> It send hotplug notification through uevent to framework after main link
>> is ready.
>>
>> Framework  is responsible to set up MDP timing engine to start video stream.
>>
>>
>> However it does not know which
> It's of no concern to the driver. It is completely the userspace
> problem. After resuming, it should reread available video output
> properties. The display could have been changed while the system is
> suspended.
>  From your description I still do not understand why you need the
> 'recovery' mechanism.

I mean "recovery" means  dp driver may leave stay at link ready 
mistakenly after dongle unplugged due to missing framework action to 
tear down main link so that next dongle plug in will not work.

Currently it was armed with timeout function and it will fired if 
framework did not call msm_dp_display_disable() after 5 second.

Anyway, we think this approach is not good. therefore it is replaced 
with  below patch.

drm/msm/dp: tear down main link at unplug handle immediately

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Dmitry Baryshkov]

On 15/04/2022 01:06, Kuogee Hsieh wrote:
> 
> On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote:
>> On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>> wrote:
>>>
>>> On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
>>>> On 07/04/2022 00:28, Kuogee Hsieh wrote:
>>>>> dp_hpd_plug_handle() is responsible for setting up main link and send
>>>>> uevent to notify user space framework to start video stream. 
>>>>> Similarly,
>>>>> dp_hdp_unplug_handle is responsible to send uevent to notify user 
>>>>> space
>>>>> framework to stop video stream and then tear down main link.
>>>>> However there are rare cases, such as in the middle of system
>>>>> suspending,
>>>>> that uevent could not be delivered to user space framework. Therefore
>>>>> some kind of recover mechanism armed by timer need to be in place 
>>>>> in the
>>>>> case of user space framework does not respond to uevent.
>>>> Hmm, how does userpsace 'respond' to the uevent? The driver should
>>>> send hotplug notifications to userspace, but it must not expect any
>>>> particular reaction. The userspace might be as simple, as fbdev
>>>> emulation, but the driver still should function correctly.
>>> yes, driver is function correctly by setting up main link. but it does
>>> not know which resolution to display.
>>>
>>> It send hotplug notification through uevent to framework after main link
>>> is ready.
>>>
>>> Framework  is responsible to set up MDP timing engine to start video 
>>> stream.
>>>
>>>
>>> However it does not know which
>> It's of no concern to the driver. It is completely the userspace
>> problem. After resuming, it should reread available video output
>> properties. The display could have been changed while the system is
>> suspended.
>>  From your description I still do not understand why you need the
>> 'recovery' mechanism.
> 
> I mean "recovery" means  dp driver may leave stay at link ready 
> mistakenly after dongle unplugged due to missing framework action to 
> tear down main link so that next dongle plug in will not work.
> 
> Currently it was armed with timeout function and it will fired if 
> framework did not call msm_dp_display_disable() after 5 second.

framework = userspace?

Is my understanding correct? Currently DP driver sends a notification to 
userspace that DP is unplugged, waits for userspace to disable DP 
output, and only after that it will shutdown the link. Is this a correct 
description?

If so, then yes, your change is correct. I mean that the kernel 
shouldn't wait for clients to stop using video pipeline if the sink gets 
unplugged. Instead it should tear the video stream down and let the DRM 
client cope with that.

I'm not sure how should the driver react if the client doesn't disable 
the output, but then the sink gets reattached?
And a bit more interesting question: how should the driver behave if the 
new sink is not compatible with the existing video stream.

> 
> Anyway, we think this approach is not good. therefore it is replaced 
> with  below patch.
> 
> drm/msm/dp: tear down main link at unplug handle immediately


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/14/2022 4:34 PM, Dmitry Baryshkov wrote:
> On 15/04/2022 01:06, Kuogee Hsieh wrote:
>>
>> On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote:
>>> On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>>> wrote:
>>>>
>>>> On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
>>>>> On 07/04/2022 00:28, Kuogee Hsieh wrote:
>>>>>> dp_hpd_plug_handle() is responsible for setting up main link and 
>>>>>> send
>>>>>> uevent to notify user space framework to start video stream. 
>>>>>> Similarly,
>>>>>> dp_hdp_unplug_handle is responsible to send uevent to notify user 
>>>>>> space
>>>>>> framework to stop video stream and then tear down main link.
>>>>>> However there are rare cases, such as in the middle of system
>>>>>> suspending,
>>>>>> that uevent could not be delivered to user space framework. 
>>>>>> Therefore
>>>>>> some kind of recover mechanism armed by timer need to be in place 
>>>>>> in the
>>>>>> case of user space framework does not respond to uevent.
>>>>> Hmm, how does userpsace 'respond' to the uevent? The driver should
>>>>> send hotplug notifications to userspace, but it must not expect any
>>>>> particular reaction. The userspace might be as simple, as fbdev
>>>>> emulation, but the driver still should function correctly.
>>>> yes, driver is function correctly by setting up main link. but it does
>>>> not know which resolution to display.
>>>>
>>>> It send hotplug notification through uevent to framework after main 
>>>> link
>>>> is ready.
>>>>
>>>> Framework  is responsible to set up MDP timing engine to start 
>>>> video stream.
>>>>
>>>>
>>>> However it does not know which
>>> It's of no concern to the driver. It is completely the userspace
>>> problem. After resuming, it should reread available video output
>>> properties. The display could have been changed while the system is
>>> suspended.
>>>  From your description I still do not understand why you need the
>>> 'recovery' mechanism.
>>
>> I mean "recovery" means  dp driver may leave stay at link ready 
>> mistakenly after dongle unplugged due to missing framework action to 
>> tear down main link so that next dongle plug in will not work.
>>
>> Currently it was armed with timeout function and it will fired if 
>> framework did not call msm_dp_display_disable() after 5 second.
>
> framework = userspace?
>
> Is my understanding correct? Currently DP driver sends a notification 
> to userspace that DP is unplugged, waits for userspace to disable DP 
> output, and only after that it will shutdown the link. Is this a 
> correct description?
Yes, you are correct, connection need to be tear down from top to bottom.
>
> If so, then yes, your change is correct. I mean that the kernel 
> shouldn't wait for clients to stop using video pipeline if the sink 
> gets unplugged. Instead it should tear the video stream down and let 
> the DRM client cope with that.
>
> I'm not sure how should the driver react if the client doesn't disable 
> the output, but then the sink gets reattached?

I do not know that either.

But it should not happen as long as framework response to uevent.

> And a bit more interesting question: how should the driver behave if 
> the new sink is not compatible with the existing video stream.

When dongle plug/replug in, driver will always read sink's edid.

Therefore resolution database should be updated accordingly.

>
>>
>> Anyway, we think this approach is not good. therefore it is replaced 
>> with  below patch.
>>
>> drm/msm/dp: tear down main link at unplug handle immediately
>
>

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Stephen Boyd]

Quoting Kuogee Hsieh (2022-04-15 17:06:48)
>
> On 4/14/2022 4:34 PM, Dmitry Baryshkov wrote:
> >
> > I'm not sure how should the driver react if the client doesn't disable
> > the output, but then the sink gets reattached?
>
> I do not know that either.
>
> But it should not happen as long as framework response to uevent.

In talking with seanpaul@ it sounds like we can update the link status
to BAD with drm_connector_set_link_status_property() and then put it
back to GOOD when the link is re-established. This way userspace will
know it needs to retry the modeset. Does that help here?

Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/20/2022 3:58 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-04-15 17:06:48)
>> On 4/14/2022 4:34 PM, Dmitry Baryshkov wrote:
>>> I'm not sure how should the driver react if the client doesn't disable
>>> the output, but then the sink gets reattached?
>> I do not know that either.
>>
>> But it should not happen as long as framework response to uevent.
> In talking with seanpaul@ it sounds like we can update the link status
> to BAD with drm_connector_set_link_status_property() and then put it
> back to GOOD when the link is re-established. This way userspace will
> know it needs to retry the modeset. Does that help here?

To setup connection, dp driver have to enable phy and do link training 
then transfer dp controller to video ready state.

After that, dp driver signal framework to set up frame buffer 
fetching/layer mixer and start timing engine at final step to have video 
stream start transmitting to panel.

Do opposite way to tear down connection since dp driver can not reset dp 
controller and disable phy before timing engine has been stopped. 
Otherwise vsync timeout or buffer underrun/overrun may happen.

this means user space framework need to stop timing engine (stop frame 
buffer fetching/layer mixer) first and then stop video ready state of dp 
controller and then disable phy. (note, there may have something else at 
drm stack need to be teared down but i do not know details)

In this case, since framework is not response to uevent to stop timing 
engine,  dp controller still in video ready state and phy still enabled 
even dongle had been removed already.

At this point, I am not sure what dp driver should do if dongle re 
plugged back in.

Tear down dp mainlink while timing engine is still running and re setup 
dp mainlink?

However, I think this scenario most likely will not happen since if 
framework responses uevent to setup connection earlier it should be 
there to response uevent to tear down connection.