Message-ID: <31a0b303-64a4-8ac4-383d-6656f05a541b@quicinc.com>
Date:   Fri, 27 Jan 2023 10:53:59 -0800
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Douglas Anderson <dianders@...omium.org>,
        <dri-devel@...ts.freedesktop.org>, Rob Clark <robdclark@...il.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     ye xingchen <ye.xingchen@....com.cn>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Jonas Karlman <jonas@...boo.se>,
        David Airlie <airlied@...il.com>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Robert Foss <robert.foss@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Vinod Koul <vkoul@...nel.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        <linux-arm-msm@...r.kernel.org>, <freedreno@...ts.freedesktop.org>,
        Sean Paul <sean@...rly.run>, <linux-kernel@...r.kernel.org>
Subject: Re: [Freedreno] [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally
 powering up DSI hosts at modeset



On 1/13/2023 3:56 PM, Douglas Anderson wrote:
> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> time"), we moved powering up DSI hosts to modeset time. This wasn't
> because it was an elegant design, but there were no better options.
> 
> That commit actually ended up breaking ps8640, and thus was born
> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
> the old way of doing things. It turns out that ps8640 _really_ doesn't
> like its pre_enable() function to be called after
> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
> because I have any inside knowledge), it looks like the assertion of
> "RST#" in the ps8640 runtime resume handler seems like it's not
> allowed to happen after dsi_mgr_bridge_power_on()
> 
> Recently, Dave Stevenson's series landed allowing bridges some control
> over pre_enable ordering. The meaty commit for our purposes is commit
> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
> bridge init order"). As documented by that series, if a bridge doesn't
> set "pre_enable_prev_first" then we should use the old ordering.
> 
> Now that we have the commit ("drm/bridge: tc358762: Set
> pre_enable_prev_first") we can go back to the old ordering, which also
> allows us to remove the ps8640 special case.
> 
> One last note is that even without reverting commit 7d8e9a90509f
> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
> revert the ps8640 special case and try it out then it doesn't seem to
> fail anymore. I spent time bisecting / debugging this and it turns out
> to be mostly luck, so we still want this patch to make sure it's
> solid. Specifically the reason it sorta works these days is because
> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
> implemented means that we actually power the bridge chip up just a wee
> bit earlier and then the bridge happens to stay on because of
> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
> 
> Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Cc: Abhinav Kumar <quic_abhinavk@...cinc.com>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>

Why is the patch title showing 2/2? I am not seeing any 1/2 here.

> ---
> 
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++----------------------
>   1 file changed, 11 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 3a1417397283..5e6b8d423b96 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>   #define IS_SYNC_NEEDED()	(msm_dsim_glb.is_sync_needed)
>   #define IS_MASTER_DSI_LINK(id)	(msm_dsim_glb.master_dsi_link_id == id)
>   
> -#ifdef CONFIG_OF
> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
> -
> -	/*
> -	 * If the next bridge in the chain is the Parade ps8640 bridge chip
> -	 * then don't power on early since it seems to violate the expectations
> -	 * of the firmware that the bridge chip is running.
> -	 *
> -	 * NOTE: this is expected to be a temporary special case. It's expected
> -	 * that we'll eventually have a framework that allows the next level
> -	 * bridge to indicate whether it needs us to power on before it or
> -	 * after it. When that framework is in place then we'll use it and
> -	 * remove this special case.
> -	 */
> -	return !(next_bridge && next_bridge->of_node &&
> -		 of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
> -}
> -#else
> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
> -{
> -	return true;
> -}
> -#endif
> -
>   static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>   {
>   	return msm_dsim_glb.dsi[id];
> @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>   	}
>   }
>   
> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
> +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   {
>   	int id = dsi_mgr_bridge_get_id(bridge);
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>   	if (is_bonded_dsi && msm_dsi1)
>   		msm_dsi_host_enable_irq(msm_dsi1->host);
>   
> -	return;
> -
> -host1_on_fail:
> -	msm_dsi_host_power_off(host);
> -host_on_fail:
> -	dsi_mgr_phy_disable(id);
> -phy_en_fail:
> -	return;
> -}
> -
> -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
> -{
> -	int id = dsi_mgr_bridge_get_id(bridge);
> -	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> -	struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
> -	struct mipi_dsi_host *host = msm_dsi->host;
> -	bool is_bonded_dsi = IS_BONDED_DSI();
> -	int ret;
> -
> -	DBG("id=%d", id);
> -	if (!msm_dsi_device_connected(msm_dsi))
> -		return;
> -
>   	/* Do nothing with the host if it is slave-DSI in case of bonded DSI */
>   	if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>   		return;
>   
> -	if (!dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
> -
>   	ret = msm_dsi_host_enable(host);
>   	if (ret) {
>   		pr_err("%s: enable host %d failed, %d\n", __func__, id, ret);
> @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>   host1_en_fail:
>   	msm_dsi_host_disable(host);
>   host_en_fail:
> -
> +	msm_dsi_host_disable_irq(host);
> +	if (is_bonded_dsi && msm_dsi1) {
> +		msm_dsi_host_disable_irq(msm_dsi1->host);
> +		msm_dsi_host_power_off(msm_dsi1->host);
> +	}

In addition to Dmitry's comment of keeping the bridge_power_on() name,

this part of the change seems independent of the patch. This was missing 
cleanup for DSI1 (esp the disable_irq part).

So can we break it up into two parts.

1) Add missing cleanup for DSI1
2) Just get rid of dsi_mgr_power_on_early() and keep the call 
dsi_mgr_bridge_power_on() in dsi_mgr_bridge_pre_enable() unconditionally.

> +host1_on_fail:
> +	msm_dsi_host_power_off(host);
> +host_on_fail:
> +	dsi_mgr_phy_disable(id);
> +phy_en_fail:
>   	return;
>   }
>   
> @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>   	msm_dsi_host_set_display_mode(host, adjusted_mode);
>   	if (is_bonded_dsi && other_dsi)
>   		msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
> -
> -	if (dsi_mgr_power_on_early(bridge))
> -		dsi_mgr_bridge_power_on(bridge);
>   }
>   
>   static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,

Powered by blists - more mailing lists