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