On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
