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

Kuogee Hsieh quic_khsieh at quicinc.com
Wed Apr 6 21:19:29 UTC 2022


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 at 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;


More information about the dri-devel mailing list