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

[Abhinav Kumar]

On 2/11/2022 11:37 AM, Dmitry Baryshkov wrote:
> 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.

Alright, sounds good,

Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> 
>>
>>>
>>> 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 = {
>>
>>
>>
> 
>