Re: [PATCH v6, 2/4] drm/mediatek: Separate poweron/poweroff from enable/disable and define new funcs

On Wed, 2022-05-18 at 11:39 +0800, CK Hu wrote:

Hi, Xinlei:

On Thu, 2022-05-12 at 14:30 +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 (atomic_pre_enable/atomic_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 Reviewed-by: AngeloGioacchino Del Regno < angelogioacchino.delregno@collabora.com> Reviewed-by: Rex-BC Chen rex-bc.chen@mediatek.com

---

drivers/gpu/drm/mediatek/mtk_dsi.c | 53 +++++++++++++++++++-------

-- 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index f880136cec09..d9a6b928dba8 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -691,16 +691,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 after
  

mtk_drm_crtc_atomic_disable(),

• * which needs irq for vblank, and mtk_dsi_stop() will disable
  

irq.

• * mtk_dsi_start() needs to be called in
  

mtk_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);

@@ -715,17 +705,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);

@@ -739,7 +721,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, since
  

• * mtk_dsi_stop() should be called after
  

mtk_drm_crtc_atomic_disable(),

• * which needs irq for vblank, and mtk_dsi_stop() will disable
  

irq.

• * mtk_dsi_start() needs to be called in
  

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

} @@ -776,13 +767,37 @@ static void mtk_dsi_bridge_atomic_enable(struct drm_bridge *bridge, { struct mtk_dsi *dsi = bridge_to_dsi(bridge);

• if (dsi->refcount == 0)

• return;
  

This seems not related to pre_enable()/post_disable(). So separate this to another patch.

Regards, CK

...

• mtk_output_dsi_enable(dsi);

}

+static void mtk_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,

• 			     struct drm_bridge_state
  

*old_bridge_state) +{

• struct mtk_dsi *dsi = bridge_to_dsi(bridge);
• int ret;
• ret = mtk_dsi_poweron(dsi);
• if (ret < 0)

• DRM_ERROR("failed to power on dsi\n");
  

+}

+static void mtk_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,

• 			       struct drm_bridge_state
  

*old_bridge_state) +{

• struct mtk_dsi *dsi = bridge_to_dsi(bridge);
• mtk_dsi_poweroff(dsi);

+}

static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = { .attach = mtk_dsi_bridge_attach, .atomic_disable = mtk_dsi_bridge_atomic_disable, .atomic_enable = mtk_dsi_bridge_atomic_enable,

• .atomic_pre_enable = mtk_dsi_bridge_atomic_pre_enable,
• .atomic_post_disable = mtk_dsi_bridge_atomic_post_disable, .mode_set = mtk_dsi_bridge_mode_set,

};

Hi CK: Thanks for your review.

The modification here is related to pre_enable()/post_disable(). Because in the previous dsi_enable function, dsi_poweron will be executed first and then the dsi register will be operated. But the current modification will move dsi_poweron to the pre_enable function, and the pre_enable is of void type, when dsi_poweron fails, an error log will be printed, but the dsi register will be operated in dsi_enable later , it may cause bus hang.

Therefore, dsi_poweron is judged in the dsi_enable function to avoid the above situation. I'll add that description in the comments on the next edition.

Best Regards! xinlei