Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 31 Jan 2023 15:32:12 -0800 | Subject | Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset | From | Abhinav Kumar <> |
| |
On 1/31/2023 2:18 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@raspberrypi.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable() > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +-------------------------- > 1 file changed, 1 insertion(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 1bbac72dad35..2197a54b9b96 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]; > @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > 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; >
Why are these two checks removed?
> ret = dsi_mgr_phy_enable(id, phy_shared_timings); > if (ret) > @@ -327,8 +295,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > return; > > - if (!dsi_mgr_power_on_early(bridge)) > - dsi_mgr_bridge_power_on(bridge); > + dsi_mgr_bridge_power_on(bridge); > > ret = msm_dsi_host_enable(host); > if (ret) { > @@ -438,9 +405,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,
| ![\](/images/icornerr.gif) |