Messages in this thread | | | Date | Wed, 6 Apr 2022 14:19:29 -0700 | Subject | Re: [PATCH] drm/msm/dp: enhance both connect and disconnect pending_timeout handle | From | Kuogee Hsieh <> |
| |
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;
| |