| Message ID | 1649280493-4393-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle | expand |
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; > >
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.
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);
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.
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
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
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 > >
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?
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.
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;
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(-)