[PATCH 1/2] drm/msm/dsi: move DSI host powerup to modeset time

[Dmitry Baryshkov]

On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote:
> > The DSI subsystem does not fully fall into the pre-enable/enable system
> > of callbacks, since typically DSI device bridge drivers expect to be
> > able to communicate with DSI devices at the pre-enable() callback. The
> > reason is that for some DSI hosts enabling the video stream would
> > prevent other drivers from sending DSI commands. For example see the
> > panel-bridge driver, which does drm_panel_prepare() from the
> > pre_enable() callback (which would be called before our pre_enable()
> > callback, resulting in panel preparation failures as the link is not yet
> > ready).
> >
> > Therewere several attempts to solve this issue, but currently the best
> > approach is to power up the DSI link from the mode_set() callback,
> > allowing next bridge/panel to use DSI transfers in the pre_enable()
> > time. Follow this approach.
> >
> Change looks okay. As per the programming guideline, we should set the
> VIDEO_MODE_EN register in the DSI controller followed by enabling the
> timing engine which will still happen even now because we will do it in
> modeset instead of the pre_enable().
> But, this can potentially increase the delay between VIDEO_MODE_EN
> and TIMING_ENGINE_EN. I dont see anything in the programming guide
> against this but since this is a change from the original flow, I would
> like to do one test before acking this. Can you please try adding a huge
> delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to
> make sure there are no issues? You can do that here:


Fine, I'll do the test as the time permits.

>
> int msm_dsi_host_enable(struct mipi_dsi_host *host)
> {
>      struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>
>      dsi_op_mode_config(msm_host,
>          !!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true);
>
>      msleep(300);
> }
>
>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > ---
> >   drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++--------
> >   1 file changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index 681ca74fe410..497719efb9e9 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector *connector)
> >       return msm_dsi_get_encoder(msm_dsi);
> >   }
> >
> > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
> > +static void dsi_mgr_bridge_power_on(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;
> > -     struct drm_panel *panel = msm_dsi->panel;
> >       struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX];
> >       bool is_bonded_dsi = IS_BONDED_DSI();
> >       int ret;
> > @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(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;
> > +     struct drm_panel *panel = msm_dsi->panel;
> > +     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;
> > +
> >       /* Always call panel functions once, because even for dual panels,
> >        * there is only one drm_panel instance.
> >        */
> > @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
> >       if (panel)
> >               drm_panel_unprepare(panel);
> >   panel_prep_fail:
> > -     msm_dsi_host_disable_irq(host);
> > -     if (is_bonded_dsi && msm_dsi1)
> > -             msm_dsi_host_disable_irq(msm_dsi1->host);
> >
> > -     if (is_bonded_dsi && msm_dsi1)
> > -             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;
> >   }
> >
> > @@ -573,6 +590,8 @@ 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);
> > +
> > +     dsi_mgr_bridge_power_on(bridge);
> >   }
> >
> >   static const struct drm_connector_funcs dsi_mgr_connector_funcs = {



-- 
With best wishes
Dmitry