Re: [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset

[Doug Anderson]

Hi,

On Thu, Jan 26, 2023 at 9:49 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Sat, 14 Jan 2023 at 01:56, Douglas Anderson <dianders@xxxxxxxxxxxx> 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().
>
> I had a small comment on your patch, but then I was distracted and
> forgot to send it. See below.
>
> >
> > Cc: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> >
> >  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)
>
> Can you please keep the dsi_mgr_bridge_power_on() as is and just
> remove the now-legacy dsi_mgr_power_on_early().

By this, I assume you mean keep the function separate but still remove
the call to it from "modeset" and unconditionally call it from
dsi_mgr_bridge_pre_enable(), right?


> >  {
> >         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);
> > +       }
> > +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,
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
>
>
> --
> With best wishes
> Dmitry