On Tue, 2022-03-22 at 17:23 +0800, xinlei.lee wrote:
On Thu, 2022-03-17 at 20:02 +0800, Rex-BC Chen wrote:
Hello Xinlei,
On Thu, 2022-03-17 at 15:53 +0800, xinlei.lee@mediatek.com wrote:
From: Jitao Shi jitao.shi@mediatek.com
In order to match the changes of "Use the drm_panel_bridge API", the poweron/poweroff of dsi is extracted from enable/disable and defined as new funcs (pre_enable/post_disable).
Fixes: 2dd8075d2185 ("drm/mediatek: mtk_dsi: Use the drm_panel_bridge API")
Signed-off-by: Jitao Shi jitao.shi@mediatek.com Signed-off-by: Xinlei Lee xinlei.lee@mediatek.com
drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++++-------
--
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 262c027d8c2f..e33caaca11a7 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -679,16 +679,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi) if (--dsi->refcount != 0) return;
- /*
* mtk_dsi_stop() and mtk_dsi_start() is asymmetric, since* mtk_dsi_stop() should be called aftermtk_drm_crtc_atomic_disable(),
* which needs irq for vblank, and mtk_dsi_stop() will disableirq.
* mtk_dsi_start() needs to be called inmtk_output_dsi_enable(),
* after dsi is fully set.*/- mtk_dsi_stop(dsi);
- mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500); mtk_dsi_reset_engine(dsi); mtk_dsi_lane0_ulp_mode_enter(dsi); mtk_dsi_clk_ulp_mode_enter(dsi);
@@ -703,17 +693,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
static void mtk_output_dsi_enable(struct mtk_dsi *dsi) {
int ret;
if (dsi->enabled) return;
ret = mtk_dsi_poweron(dsi);
if (ret < 0) {
DRM_ERROR("failed to power on dsi\n");return;}
mtk_dsi_set_mode(dsi); mtk_dsi_clk_hs_mode(dsi, 1);
@@ -727,7 +709,16 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi) if (!dsi->enabled) return;
- mtk_dsi_poweroff(dsi);
- /*
* mtk_dsi_stop() and mtk_dsi_start() is asymmetric, sinceWhy they are asymmetric?
* mtk_dsi_stop() should be called aftermtk_drm_crtc_atomic_disable(),
* which needs irq for vblank, and mtk_dsi_stop() will disableirq.
* mtk_dsi_start() needs to be called inmtk_output_dsi_enable(),
* after dsi is fully set.*/mtk_dsi_stop(dsi);
mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
dsi->enabled = false;
} @@ -765,10 +756,26 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge) mtk_output_dsi_enable(dsi); }
+static void mtk_dsi_bridge_pre_enable(struct drm_bridge *bridge) +{
- struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- mtk_dsi_poweron(dsi);
Should you handle the error of mtk_dsi_poweron? If you failed to mtk_dsi_bridge_pre_enable and do mtk_dsi_bridge_enable, what will happend?
+}
+static void mtk_dsi_bridge_post_disable(struct drm_bridge *bridge) +{
- struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- mtk_dsi_poweroff(dsi);
If you failed to mtk_dsi_bridge_disable and you do mtk_dsi_bridge_post_disable, what will happend? Do you need to handle this?
BRs, Rex
+}
static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = { .attach = mtk_dsi_bridge_attach, .disable = mtk_dsi_bridge_disable, .enable = mtk_dsi_bridge_enable,
- .pre_enable = mtk_dsi_bridge_pre_enable,
- .post_disable = mtk_dsi_bridge_post_disable, .mode_set = mtk_dsi_bridge_mode_set,
};
Hi Rex:
Thanks for your review!
1.Why they are asymmetric? =>My understanding mtk_dsi_stop() and mtk_dsi_start() is to make dsi switch from LP11 and HS mode.DSI has two working modes: If it is cmd mode, the data sent is sent by LP11, and dsi_start is just a signal. In this mode, dsi_stop is not required after sending cmd. If it is video mode, because the data needs to be sent in HS mode, dsi_start is required to make dsi enter HS mode from LP11. After suspend, drm will call dsi_disable, and call dsi_stop to make dsi return from HS mode to LP11 state. Therefore mtk_dsi_stop() and mtk_dsi_start() are asymmetric. For example, in the dsi_host_transfer function, only dsi_stop has no dsi_start operation.
Because the return type of pre_enable & post_disable in common code is void type. If there is an error, it will be processed in poweron/poweroff, and the error message will be printed. Do you mean that pre_enable & post_disable needs to accept the poweron/poweroff error return value and then print the error log?
If pre_enable fails, there is only a problem with the dsi module, and it does not affect the execution of other modules and enable funcs under drm. Same goes for post_disable & disable.
Best Regrds! xinlei
Hello Xinlei,
about failure for mtk_dsi_poweron(), it is all becuase of error setting for clock. If we do not set clock correctly and not enable DSI, after set DSI start, will it cause any issue? (maybe bus hang or something.)
Because in original drivers, if mtk_dsi_poweron failed, the DSI will not start.
IMO, it also needs some error message for mtk_dsi_bridge_pre_enable() and mtk_dsi_bridge_post_disable().
BRs, Rex