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

[Dmitry Baryshkov]

On 18/01/2022 23:03, Dmitry Baryshkov wrote:
> 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.

I did the tests, the display pipeline works as expected.

Let's get this in, it allows using other DSI-controlled bridges.

> 
>>
>> 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