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

From: Stephen Boyd
Date: Tue Apr 05 2022 - 21:31:31 EST

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;