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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    | 34 ++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_ctrl.h    |  1 +
>  drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++++++++++++----------
>  3 files changed, 68 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dcd0126..3f4cf6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1910,7 +1910,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>         return ret;
>  }
>
> -int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl)
>  {
>         struct dp_ctrl_private *ctrl;
>         struct dp_io *dp_io;
> @@ -1926,7 +1926,37 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>
>         dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>
> -       dp_catalog_ctrl_reset(ctrl->catalog);
> +       ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false);
> +       if (ret) {
> +               DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
> +       }
> +
> +       DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);

Please don't recast pointer prints with %x. Use %p to print pointers.

> +
> +       phy_power_off(phy);
> +
> +       DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
> +               (u32)(uintptr_t)phy, phy->init_count, phy->power_count);

It doesn't look good to be peeking inside struct phy. I wonder why that
isn't an opaque struct. Either way, please don't print the struct
members.

> +
> +       return ret;
> +}
> +
> +int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
> +{
> +       struct dp_ctrl_private *ctrl;
> +       struct dp_io *dp_io;
> +       struct phy *phy;
> +       int ret = 0;

Drop useless assignment please

> +
> +       if (!dp_ctrl)

How is this possible? Please remove useless checks.

> +               return -EINVAL;
> +
> +       ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +       dp_io = &ctrl->parser->io;
> +       phy = dp_io->phy;
> +
> +       dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false);
>
>         ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false);
>         if (ret)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 178b774..56bf7c5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -593,10 +593,16 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data)
>
>         mutex_lock(&dp->event_mutex);
>
> +       /*
> +        * main link had been setup but video is not ready yet
> +        * only tear down main link
> +        */
>         state = dp->hpd_state;
>         if (state == ST_CONNECT_PENDING) {
> -               dp->hpd_state = ST_CONNECTED;
>                 DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> +               dp_ctrl_off_link(dp->ctrl);
> +               dp_display_host_phy_exit(dp);
> +               dp->hpd_state = ST_DISCONNECTED;
>         }
>
>         mutex_unlock(&dp->event_mutex);
> @@ -645,6 +651,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>                 if (dp->link->sink_count == 0) {
>                         dp_display_host_phy_exit(dp);
>                 }
> +               dp_display_usbpd_disconnect_cb(&dp->pdev->dev);

The name of this function is very confusing. It would be nice to rename
it to something like dp_display_notify_disconnect() and skip

>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
>         }
> @@ -661,19 +668,19 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>                 return 0;
>         }
>
> -       dp->hpd_state = ST_DISCONNECT_PENDING;
> -
>         /* disable HPD plug interrupts */
>         dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>
> -       /*
> -        * We don't need separate work for disconnect as
> -        * connect/attention interrupts are disabled
> -        */

Why is this comment removed? Because a work is actually used? Why can't
we call dp_display_send_hpd_notification() directly?

>         dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
>
> -       /* start sentinel checking in case of missing uevent */
> -       dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> +       if (state == ST_DISPLAY_OFF) {
> +               dp->hpd_state = ST_DISCONNECTED;
> +

Drop extra newline please

> +       } else {
> +               /* start sentinel checking in case of missing uevent */
> +               dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
> +               dp->hpd_state = ST_DISCONNECT_PENDING;

It looks like dp_add_event() should check the event and change
dp->hpd_state sometimes. Then this code would be simply adding an event
and dp_add_event() would be changing the hpd_state while scheduling the
work.

> +       }
>
>         /* signal the disconnect event early to ensure proper teardown */
>         dp_display_handle_plugged_change(&dp->dp_display, false);
> @@ -695,10 +702,16 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data
>
>         mutex_lock(&dp->event_mutex);
>
> +       /*
> +        * main link had been set up and video is ready
> +        * tear down main link, video stream and phy

because disconnect event never came and we need to get back to a default
sane state?

> +        */
>         state =  dp->hpd_state;
>         if (state == ST_DISCONNECT_PENDING) {
> -               dp->hpd_state = ST_DISCONNECTED;
>                 DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> +               dp_ctrl_off(dp->ctrl);
> +               dp_display_host_phy_exit(dp);
> +               dp->hpd_state = ST_DISCONNECTED;
>         }
>
>         mutex_unlock(&dp->event_mutex);
> @@ -1571,6 +1584,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>
>         mutex_lock(&dp_display->event_mutex);
>
> +       state =  dp_display->hpd_state;

s/=  /= /

Drop extra space after '=' please.

> +       if (state == ST_DISCONNECTED) {
> +               mutex_unlock(&dp_display->event_mutex);
> +               return rc;
> +       }
> +
>         /* stop sentinel checking */
>         dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
>
> @@ -1588,8 +1607,6 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>                 return rc;
>         }
>
> -       state =  dp_display->hpd_state;
> -
>         if (state == ST_DISPLAY_OFF)
>                 dp_display_host_phy_init(dp_display);
>
> @@ -1638,13 +1655,18 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>         /* stop sentinel checking */
>         dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>
> +       state =  dp_display->hpd_state;

s/=  /= /

Drop extra space after '=' please.

> +       if (state == ST_DISCONNECTED || state == ST_DISPLAY_OFF) {
> +               mutex_unlock(&dp_display->event_mutex);
> +               return rc;
> +       }
> +
>         dp_display_disable(dp_display, 0);
>
>         rc = dp_display_unprepare(dp);
>         if (rc)
>                 DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>
> -       state =  dp_display->hpd_state;
>         if (state == ST_DISCONNECT_PENDING) {
>                 /* completed disconnection */
>                 dp_display->hpd_state = ST_DISCONNECTED;



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux