[PATCH v8 0/3] Add display support for the MT8365-EVK board

[PATCH v8 0/3] Add display support for the MT8365-EVK board

[Alexandre Mergnat]

The purpose of this series is to add the display support for the mt8365-evk.

This is the list of HWs / IPs support added:
- Connectors (HW):
  - HDMI
  - MIPI DSI (Mobile Industry Processor Interface Display Serial Interface)
- HDMI bridge (it66121)
- DSI pannel (startek,kd070fhfid015)
- SoC display blocks (IP):
  - OVL0 (Overlay)
  - RDMA0 (Data Path Read DMA)
  - Color0
  - CCorr0 (Color Correction)
  - AAL0 (Adaptive Ambient Light)
  - GAMMA0
  - Dither0
  - DSI0 (Display Serial Interface)
  - RDMA1 (Data Path Read DMA)
  - DPI0 (Display Parallel Interface)

The Mediatek DSI, DPI and DRM drivers are also improved.

The series is rebased on top of Angelo's series [1] to
use the OF graphs support.

Update 03/20/25:
According to Angelo's comment, the DSI fix in the previous version
doesn't fix the root issue.
Then, the "drm/mediatek: dsi: Improves the DSI lane setup robustness"
as been replaced by two other patch to fix the DSI power on sequence
and remove custom power function.

Regards,
Alex

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
Changes in v8:
- Patch merged, then removed from the series:
  - dt-bindings: display: mediatek: dpi: add power-domains example
  - drm/mediatek: add MT8365 SoC support
  - arm64: dts: mediatek: add display blocks support for the MT8365 SoC
  - arm64: dts: mediatek: add display support for mt8365-evk
- Patch removed:
  - drm/mediatek: dsi: Improves the DSI lane setup robustness
- Patch added:
  - drm/panel: startek-kd070fhfid015: add another init step
  - drm/mediatek: dsi: remove custom init part
- Remove MTK custom init function for the DSI driver to avoid conflict
  with what the DRM APIs provide.
- Link to v7: https://lore.kernel.org/r/20231023-display-support-v7-0-6703f3e26831@baylibre.com

Changes in v7:
- Improve defconfig commit description
- Add comment in the DTS about pins clash with ethernet and HDMI (DPI0)
- Link to v6: https://lore.kernel.org/r/20231023-display-support-v6-0-c6af4f34f4d8@baylibre.com

Changes in v6:
- Fix DPI binding: remove the duplicated property (power-domains).
- Squash defconfig commit.
- Fix the property order in the DTS.
- Link to v5: https://lore.kernel.org/r/20231023-display-support-v5-0-3905f1e4b835@baylibre.com

Changes in v5:
- Patch merged, then removed from the series:
  - dt-bindings: display: mediatek: rdma: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: ovl: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: gamma: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: dpi: add compatible for MT8365
  - dt-bindings: display: mediatek: dsi: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: dither: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: color: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: ccorr: add compatible for MT8365 SoC
  - dt-bindings: display: mediatek: aal: add compatible for MT8365 SoC
- Enable STARTEK KD070FHFID015 panel in the defconfig.
- Rebase on top of 6.13-rc6.
- Link to v4: https://lore.kernel.org/all/20231023-display-support-v4-0-ed82eb168fb1@baylibre.com

Changes in v4:
- Patch merged, then removed from the series:
  - dt-bindings: display: mediatek: dpi: add power-domains property
  - dt-bindings: pwm: mediatek,pwm-disp: add compatible for mt8365 SoC
  - clk: mediatek: mt8365-mm: fix DPI0 parent
- Remove mediatek,mt8365-dpi compatible from mtk_drm_drv.c because it
  use the mt8192's data. It's a miss.
- Add MT8365 OF graphs support, remove the hardcoded display path and
  rebase on top of Angelo's series [1].
- Link to v3: https://lore.kernel.org/r/20231023-display-support-v3-0-53388f3ed34b@baylibre.com

Changes in v3:
- Drop "drm/mediatek: add mt8365 dpi support" because it's the same
  config as mt8192 SoC
- Drop "dt-bindings: pwm: mediatek,pwm-disp: add power-domains property"
  because an equivalent patch has been merge already.
- Add DPI clock fix in a separate commit.
- Improve DTS(I) readability.
- Link to v2: https://lore.kernel.org/r/20231023-display-support-v2-0-33ce8864b227@baylibre.com

Changes in v2:
- s/binding/compatible/ in commit messages/titles.
- Improve commit messages as Conor suggest.
- pwm-disp: Set power domain property for MT8365. This one is optionnal
  and can be used for other SoC.
- Fix mediatek,dsi.yaml issue.
- Remove the extra clock in the DPI node/driver and fix the dpi clock
  parenting to be consistent with the DPI clock assignement.
- Link to v1: https://lore.kernel.org/r/20231023-display-support-v1-0-5c860ed5c33b@baylibre.com

[1] https://lore.kernel.org/lkml/20240516081104.83458-1-angelogioacchino.delregno@collabora.com/

---
Alexandre Mergnat (3):
      arm64: defconfig: enable display support for mt8365-evk
      drm/panel: startek-kd070fhfid015: add another init step
      drm/mediatek: dsi: remove custom init part

 arch/arm64/configs/defconfig                       |  2 ++
 drivers/gpu/drm/mediatek/mtk_ddp_comp.c            |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_drv.h            |  2 --
 drivers/gpu/drm/mediatek/mtk_dsi.c                 | 16 --------------
 .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
 5 files changed, 17 insertions(+), 30 deletions(-)
---
base-commit: 72e8ab284ff34785ec292f79610de5fcf3825b32
change-id: 20231023-display-support-c6418b30e419

Best regards,
-- 
Alexandre Mergnat <amergnat@baylibre.com>

[PATCH v8 1/3] arm64: defconfig: enable display support for mt8365-evk

[Alexandre Mergnat]

Enable the DRM HDMI connector support and the MIPI-DSI display
Startek KD070FHFID015 panel to have HDMI and DSI display working
on the mt8365-evk board.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c62831e615863..1e2963a13500b 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -897,9 +897,11 @@ CONFIG_DRM_PANEL_NOVATEK_NT36672E=m
 CONFIG_DRM_PANEL_RAYDIUM_RM67191=m
 CONFIG_DRM_PANEL_SAMSUNG_ATNA33XC20=m
 CONFIG_DRM_PANEL_SITRONIX_ST7703=m
+CONFIG_DRM_PANEL_STARTEK_KD070FHFID015=m
 CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA=m
 CONFIG_DRM_PANEL_VISIONOX_VTDR6130=m
 CONFIG_DRM_FSL_LDB=m
+CONFIG_DRM_DISPLAY_CONNECTOR=m
 CONFIG_DRM_LONTIUM_LT8912B=m
 CONFIG_DRM_LONTIUM_LT9611=m
 CONFIG_DRM_LONTIUM_LT9611UXC=m

-- 
2.25.1

[PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[Alexandre Mergnat]

Currently, the panel set power, set gpio and enable the display link
in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
panel_bridge_atomic_pre_enable, pointed by
drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
atomic_pre_enable must not enable the display link

Since the DSI driver is properly inited by the DRM, the panel try to
communicate with the panel before DSI is powered on.

To solve that, use stk_panel_enable to enable the display link because
it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
on the DSI.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
index c0c95355b7435..bc3c4038bf4f5 100644
--- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
+++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
@@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
 	gpiod_set_value(stk->enable_gpio, 1);
 	mdelay(20);
 	gpiod_set_value(stk->reset_gpio, 1);
-	mdelay(10);
-	ret = stk_panel_init(stk);
-	if (ret < 0)
-		goto poweroff;
-
-	ret = stk_panel_on(stk);
-	if (ret < 0)
-		goto poweroff;
 
 	return 0;
 
-poweroff:
-	regulator_disable(stk->supplies[POWER].consumer);
 iovccoff:
 	regulator_disable(stk->supplies[IOVCC].consumer);
 	gpiod_set_value(stk->reset_gpio, 0);
@@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
 	return ret;
 }
 
+static int stk_panel_enable(struct drm_panel *panel)
+{
+	struct stk_panel *stk = to_stk_panel(panel);
+	int ret;
+
+	ret = stk_panel_init(stk);
+	if (ret < 0)
+		return ret;
+
+	ret = stk_panel_on(stk);
+
+	return ret;
+}
+
 static const struct drm_display_mode default_mode = {
 		.clock = 163204,
 		.hdisplay = 1200,
@@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
 }
 
 static const struct drm_panel_funcs stk_panel_funcs = {
+	.enable = stk_panel_enable,
 	.unprepare = stk_panel_unprepare,
 	.prepare = stk_panel_prepare,
 	.get_modes = stk_panel_get_modes,

-- 
2.25.1

[PATCH v8 3/3] drm/mediatek: dsi: remove custom init part

[Alexandre Mergnat]

To be aligned with the DRM framework and avoid DSI power being driven
by two different entities, remove the custom function and keep the DRM
API to initialize the DSI.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/gpu/drm/mediatek/mtk_ddp_comp.c |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_drv.h |  2 --
 drivers/gpu/drm/mediatek/mtk_dsi.c      | 16 ----------------
 3 files changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
index edc6417639e64..d86eed0d279d3 100644
--- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
@@ -317,8 +317,6 @@ static const struct mtk_ddp_comp_funcs ddp_dsc = {
 };
 
 static const struct mtk_ddp_comp_funcs ddp_dsi = {
-	.start = mtk_dsi_ddp_start,
-	.stop = mtk_dsi_ddp_stop,
 	.encoder_index = mtk_dsi_encoder_index,
 };
 
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 04217a36939cd..5657854fa2f9e 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -47,8 +47,6 @@ void mtk_dpi_start(struct device *dev);
 void mtk_dpi_stop(struct device *dev);
 unsigned int mtk_dpi_encoder_index(struct device *dev);
 
-void mtk_dsi_ddp_start(struct device *dev);
-void mtk_dsi_ddp_stop(struct device *dev);
 unsigned int mtk_dsi_encoder_index(struct device *dev);
 
 int mtk_gamma_clk_enable(struct device *dev);
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index e61b9bc68e9a3..b813b49340420 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -787,7 +787,6 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
 {
 	if (dsi->enabled)
 		return;
-
 	mtk_dsi_lane_ready(dsi);
 	mtk_dsi_set_mode(dsi);
 	mtk_dsi_clk_hs_mode(dsi, 1);
@@ -893,20 +892,6 @@ static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
 	.mode_set = mtk_dsi_bridge_mode_set,
 };
 
-void mtk_dsi_ddp_start(struct device *dev)
-{
-	struct mtk_dsi *dsi = dev_get_drvdata(dev);
-
-	mtk_dsi_poweron(dsi);
-}
-
-void mtk_dsi_ddp_stop(struct device *dev)
-{
-	struct mtk_dsi *dsi = dev_get_drvdata(dev);
-
-	mtk_dsi_poweroff(dsi);
-}
-
 static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
 {
 	int ret;
@@ -1243,7 +1228,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
 	}
 
 	init_waitqueue_head(&dsi->irq_wait_queue);
-
 	platform_set_drvdata(pdev, dsi);
 
 	dsi->bridge.funcs = &mtk_dsi_bridge_funcs;

-- 
2.25.1

Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[AngeloGioacchino Del Regno]

Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
> Currently, the panel set power, set gpio and enable the display link
> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
> panel_bridge_atomic_pre_enable, pointed by
> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
> atomic_pre_enable must not enable the display link
> 
> Since the DSI driver is properly inited by the DRM, the panel try to
> communicate with the panel before DSI is powered on.
> 

The panel driver shall still be able to send commands in the .prepare() callback
and if this is not happening anymore... well, there's a problem!

> To solve that, use stk_panel_enable to enable the display link because
> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
> on the DSI.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>   1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
> index c0c95355b7435..bc3c4038bf4f5 100644
> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>   	gpiod_set_value(stk->enable_gpio, 1);
>   	mdelay(20);
>   	gpiod_set_value(stk->reset_gpio, 1);
> -	mdelay(10);
> -	ret = stk_panel_init(stk);
> -	if (ret < 0)
> -		goto poweroff;

Also, you're moving both init and set_display_on to the enable callback...
this is suboptimal.

You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
should have a .enable() callback that calls DISP ON, a .disable() callback that
calls DISP OFF, and .unprepare() that turns everything off.

Cheers,
Angelo

> -
> -	ret = stk_panel_on(stk);
> -	if (ret < 0)
> -		goto poweroff;
>   
>   	return 0;
>   
> -poweroff:
> -	regulator_disable(stk->supplies[POWER].consumer);
>   iovccoff:
>   	regulator_disable(stk->supplies[IOVCC].consumer);
>   	gpiod_set_value(stk->reset_gpio, 0);
> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>   	return ret;
>   }
>   
> +static int stk_panel_enable(struct drm_panel *panel)
> +{
> +	struct stk_panel *stk = to_stk_panel(panel);
> +	int ret;
> +
> +	ret = stk_panel_init(stk);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = stk_panel_on(stk);
> +
> +	return ret;
> +}
> +
>   static const struct drm_display_mode default_mode = {
>   		.clock = 163204,
>   		.hdisplay = 1200,
> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>   }
>   
>   static const struct drm_panel_funcs stk_panel_funcs = {
> +	.enable = stk_panel_enable,
>   	.unprepare = stk_panel_unprepare,
>   	.prepare = stk_panel_prepare,
>   	.get_modes = stk_panel_get_modes,
> 

Re: [PATCH v8 3/3] drm/mediatek: dsi: remove custom init part

[CK Hu (胡俊光)]

On Thu, 2025-03-20 at 09:48 +0100, Alexandre Mergnat wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> To be aligned with the DRM framework and avoid DSI power being driven
> by two different entities, remove the custom function and keep the DRM
> API to initialize the DSI.

In MT8173, need this custom init so dsi could work correctly.
I'm not sure it's software problem or hardware problem.
If it's hardware problem, this custom init could not be removed.
If it's software problem, it may be fixed in current version.
But I don't know it's fixed or not.
So I need someone to test this patch in MT8173 platform.
Otherwise, I could not apply this patch.

Regards,
CK

> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_ddp_comp.c |  2 --
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h |  2 --
>  drivers/gpu/drm/mediatek/mtk_dsi.c      | 16 ----------------
>  3 files changed, 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> index edc6417639e64..d86eed0d279d3 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> @@ -317,8 +317,6 @@ static const struct mtk_ddp_comp_funcs ddp_dsc = {
>  };
> 
>  static const struct mtk_ddp_comp_funcs ddp_dsi = {
> -       .start = mtk_dsi_ddp_start,
> -       .stop = mtk_dsi_ddp_stop,
>         .encoder_index = mtk_dsi_encoder_index,
>  };
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 04217a36939cd..5657854fa2f9e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -47,8 +47,6 @@ void mtk_dpi_start(struct device *dev);
>  void mtk_dpi_stop(struct device *dev);
>  unsigned int mtk_dpi_encoder_index(struct device *dev);
> 
> -void mtk_dsi_ddp_start(struct device *dev);
> -void mtk_dsi_ddp_stop(struct device *dev);
>  unsigned int mtk_dsi_encoder_index(struct device *dev);
> 
>  int mtk_gamma_clk_enable(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index e61b9bc68e9a3..b813b49340420 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -787,7 +787,6 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
>  {
>         if (dsi->enabled)
>                 return;
> -
>         mtk_dsi_lane_ready(dsi);
>         mtk_dsi_set_mode(dsi);
>         mtk_dsi_clk_hs_mode(dsi, 1);
> @@ -893,20 +892,6 @@ static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
>         .mode_set = mtk_dsi_bridge_mode_set,
>  };
> 
> -void mtk_dsi_ddp_start(struct device *dev)
> -{
> -       struct mtk_dsi *dsi = dev_get_drvdata(dev);
> -
> -       mtk_dsi_poweron(dsi);
> -}
> -
> -void mtk_dsi_ddp_stop(struct device *dev)
> -{
> -       struct mtk_dsi *dsi = dev_get_drvdata(dev);
> -
> -       mtk_dsi_poweroff(dsi);
> -}
> -
>  static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
>  {
>         int ret;
> @@ -1243,7 +1228,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>         }
> 
>         init_waitqueue_head(&dsi->irq_wait_queue);
> -
>         platform_set_drvdata(pdev, dsi);
> 
>         dsi->bridge.funcs = &mtk_dsi_bridge_funcs;
> 
> --
> 2.25.1
> 

Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[Alexandre Mergnat]

Hi Angelo,
Thanks for the fast feedback :)

On 20/03/2025 13:37, AngeloGioacchino Del Regno wrote:
> Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
>> Currently, the panel set power, set gpio and enable the display link
>> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
>> panel_bridge_atomic_pre_enable, pointed by
>> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
>> atomic_pre_enable must not enable the display link
>>
>> Since the DSI driver is properly inited by the DRM, the panel try to
>> communicate with the panel before DSI is powered on.
>>
> 
> The panel driver shall still be able to send commands in the .prepare() callback
> and if this is not happening anymore... well, there's a problem!

Sorry I don't think so, according to that def:
	/**
	 * @pre_enable:
	 *
	 * This callback should enable the bridge. It is called right before
	 * the preceding element in the display pipe is enabled. If the
	 * preceding element is a bridge this means it's called before that
	 * bridge's @pre_enable function. If the preceding element is a
	 * &drm_encoder it's called right before the encoder's
	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
	 * &drm_encoder_helper_funcs.dpms hook.
	 *
	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
	 * will not yet be running when this callback is called. The bridge must
	 * not enable the display link feeding the next bridge in the chain (if
	 * there is one) when this callback is called.
	 *
	 * The @pre_enable callback is optional.
	 *
	 * NOTE:
	 *
	 * This is deprecated, do not use!
	 * New drivers shall use &drm_bridge_funcs.atomic_pre_enable.
	 */
	void (*pre_enable)(struct drm_bridge *bridge);

	/**
	 * @enable:
	 *
	 * This callback should enable the bridge. It is called right after
	 * the preceding element in the display pipe is enabled. If the
	 * preceding element is a bridge this means it's called after that
	 * bridge's @enable function. If the preceding element is a
	 * &drm_encoder it's called right after the encoder's
	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
	 * &drm_encoder_helper_funcs.dpms hook.
	 *
	 * The bridge can assume that the display pipe (i.e. clocks and timing
	 * signals) feeding it is running when this callback is called. This
	 * callback must enable the display link feeding the next bridge in the
	 * chain if there is one.
	 *
	 * The @enable callback is optional.
	 *
	 * NOTE:
	 *
	 * This is deprecated, do not use!
	 * New drivers shall use &drm_bridge_funcs.atomic_enable.
	 */
	void (*enable)(struct drm_bridge *bridge);

=> "The bridge must not enable the display link feeding the next bridge in the
=> chain (if there is one) when this callback is called."

Additionally, you ask for something impossible because here is the init order
fixed by the framework:

[   10.753139] panel_bridge_atomic_pre_enable
[   10.963505] mtk_dsi_bridge_atomic_pre_enable
[   10.963518] mtk_dsi_bridge_atomic_enable
[   10.963527] panel_bridge_atomic_enable
[   10.963532] drm_panel_enable

If panel want to use the DSI link in panel_bridge_atomic_pre_enable, nothing
will happen and  you will get a timeout.

So, IMHO, this patch make sense.

> 
>> To solve that, use stk_panel_enable to enable the display link because
>> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
>> on the DSI.
>>
>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>> ---
>>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c 
>> b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>> index c0c95355b7435..bc3c4038bf4f5 100644
>> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>       gpiod_set_value(stk->enable_gpio, 1);
>>       mdelay(20);
>>       gpiod_set_value(stk->reset_gpio, 1);
>> -    mdelay(10);
>> -    ret = stk_panel_init(stk);
>> -    if (ret < 0)
>> -        goto poweroff;
> 
> Also, you're moving both init and set_display_on to the enable callback...
> this is suboptimal.
> 
> You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
> should have a .enable() callback that calls DISP ON, a .disable() callback that
> calls DISP OFF, and .unprepare() that turns everything off.

This is not what I understand from the pre_enable's definition above, and also
the function call order by the framework. :)

> 
> Cheers,
> Angelo
> 
>> -
>> -    ret = stk_panel_on(stk);
>> -    if (ret < 0)
>> -        goto poweroff;
>>       return 0;
>> -poweroff:
>> -    regulator_disable(stk->supplies[POWER].consumer);
>>   iovccoff:
>>       regulator_disable(stk->supplies[IOVCC].consumer);
>>       gpiod_set_value(stk->reset_gpio, 0);
>> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>       return ret;
>>   }
>> +static int stk_panel_enable(struct drm_panel *panel)
>> +{
>> +    struct stk_panel *stk = to_stk_panel(panel);
>> +    int ret;
>> +
>> +    ret = stk_panel_init(stk);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = stk_panel_on(stk);
>> +
>> +    return ret;
>> +}
>> +
>>   static const struct drm_display_mode default_mode = {
>>           .clock = 163204,
>>           .hdisplay = 1200,
>> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>>   }
>>   static const struct drm_panel_funcs stk_panel_funcs = {
>> +    .enable = stk_panel_enable,
>>       .unprepare = stk_panel_unprepare,
>>       .prepare = stk_panel_prepare,
>>       .get_modes = stk_panel_get_modes,
>>
> 
> 
> 

-- 
Regards,
Alexandre

Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[Alexandre Mergnat]

Hi Angelo,

Gentle ping

Let me shortly summarize my problem: I see the panel driver sending commands to the display before 
it is ready. My approach to prevent that is to delay sending commands until bridge enable. Your 
concern was that during the panel's .prepare() the panel driver should already be able to send 
commands through the bridge. Can you please clarify what you think should be the approach to fix that?

Regards,
Alexandre

On 21/03/2025 10:19, Alexandre Mergnat wrote:
> Hi Angelo,
> Thanks for the fast feedback :)
> 
> On 20/03/2025 13:37, AngeloGioacchino Del Regno wrote:
>> Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
>>> Currently, the panel set power, set gpio and enable the display link
>>> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
>>> panel_bridge_atomic_pre_enable, pointed by
>>> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
>>> atomic_pre_enable must not enable the display link
>>>
>>> Since the DSI driver is properly inited by the DRM, the panel try to
>>> communicate with the panel before DSI is powered on.
>>>
>>
>> The panel driver shall still be able to send commands in the .prepare() callback
>> and if this is not happening anymore... well, there's a problem!
> 
> Sorry I don't think so, according to that def:
>      /**
>       * @pre_enable:
>       *
>       * This callback should enable the bridge. It is called right before
>       * the preceding element in the display pipe is enabled. If the
>       * preceding element is a bridge this means it's called before that
>       * bridge's @pre_enable function. If the preceding element is a
>       * &drm_encoder it's called right before the encoder's
>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>       * &drm_encoder_helper_funcs.dpms hook.
>       *
>       * The display pipe (i.e. clocks and timing signals) feeding this bridge
>       * will not yet be running when this callback is called. The bridge must
>       * not enable the display link feeding the next bridge in the chain (if
>       * there is one) when this callback is called.
>       *
>       * The @pre_enable callback is optional.
>       *
>       * NOTE:
>       *
>       * This is deprecated, do not use!
>       * New drivers shall use &drm_bridge_funcs.atomic_pre_enable.
>       */
>      void (*pre_enable)(struct drm_bridge *bridge);
> 
>      /**
>       * @enable:
>       *
>       * This callback should enable the bridge. It is called right after
>       * the preceding element in the display pipe is enabled. If the
>       * preceding element is a bridge this means it's called after that
>       * bridge's @enable function. If the preceding element is a
>       * &drm_encoder it's called right after the encoder's
>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>       * &drm_encoder_helper_funcs.dpms hook.
>       *
>       * The bridge can assume that the display pipe (i.e. clocks and timing
>       * signals) feeding it is running when this callback is called. This
>       * callback must enable the display link feeding the next bridge in the
>       * chain if there is one.
>       *
>       * The @enable callback is optional.
>       *
>       * NOTE:
>       *
>       * This is deprecated, do not use!
>       * New drivers shall use &drm_bridge_funcs.atomic_enable.
>       */
>      void (*enable)(struct drm_bridge *bridge);
> 
> => "The bridge must not enable the display link feeding the next bridge in the
> => chain (if there is one) when this callback is called."
> 
> Additionally, you ask for something impossible because here is the init order
> fixed by the framework:
> 
> [   10.753139] panel_bridge_atomic_pre_enable
> [   10.963505] mtk_dsi_bridge_atomic_pre_enable
> [   10.963518] mtk_dsi_bridge_atomic_enable
> [   10.963527] panel_bridge_atomic_enable
> [   10.963532] drm_panel_enable
> 
> If panel want to use the DSI link in panel_bridge_atomic_pre_enable, nothing
> will happen and  you will get a timeout.
> 
> So, IMHO, this patch make sense.
> 
>>
>>> To solve that, use stk_panel_enable to enable the display link because
>>> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
>>> on the DSI.
>>>
>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>> ---
>>>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c 
>>> b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>> index c0c95355b7435..bc3c4038bf4f5 100644
>>> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>       gpiod_set_value(stk->enable_gpio, 1);
>>>       mdelay(20);
>>>       gpiod_set_value(stk->reset_gpio, 1);
>>> -    mdelay(10);
>>> -    ret = stk_panel_init(stk);
>>> -    if (ret < 0)
>>> -        goto poweroff;
>>
>> Also, you're moving both init and set_display_on to the enable callback...
>> this is suboptimal.
>>
>> You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
>> should have a .enable() callback that calls DISP ON, a .disable() callback that
>> calls DISP OFF, and .unprepare() that turns everything off.
> 
> This is not what I understand from the pre_enable's definition above, and also
> the function call order by the framework. :)
> 
>>
>> Cheers,
>> Angelo
>>
>>> -
>>> -    ret = stk_panel_on(stk);
>>> -    if (ret < 0)
>>> -        goto poweroff;
>>>       return 0;
>>> -poweroff:
>>> -    regulator_disable(stk->supplies[POWER].consumer);
>>>   iovccoff:
>>>       regulator_disable(stk->supplies[IOVCC].consumer);
>>>       gpiod_set_value(stk->reset_gpio, 0);
>>> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>       return ret;
>>>   }
>>> +static int stk_panel_enable(struct drm_panel *panel)
>>> +{
>>> +    struct stk_panel *stk = to_stk_panel(panel);
>>> +    int ret;
>>> +
>>> +    ret = stk_panel_init(stk);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    ret = stk_panel_on(stk);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static const struct drm_display_mode default_mode = {
>>>           .clock = 163204,
>>>           .hdisplay = 1200,
>>> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>>>   }
>>>   static const struct drm_panel_funcs stk_panel_funcs = {
>>> +    .enable = stk_panel_enable,
>>>       .unprepare = stk_panel_unprepare,
>>>       .prepare = stk_panel_prepare,
>>>       .get_modes = stk_panel_get_modes,
>>>
>>
>>
>>
> 

Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[AngeloGioacchino Del Regno]

Il 15/04/25 16:13, Alexandre Mergnat ha scritto:
> Hi Angelo,
> 
> Gentle ping
> 
> Let me shortly summarize my problem: I see the panel driver sending commands to the 
> display before it is ready. My approach to prevent that is to delay sending 
> commands until bridge enable. Your concern was that during the panel's .prepare() 
> the panel driver should already be able to send commands through the bridge. Can 
> you please clarify what you think should be the approach to fix that?
> 

Please don't top post.

Anyway - sorry but I missed your reply, that wasn't intentional - thanks for the
ping (or I wouldn't have replied, duh!).

What is not ready? The Startek display or the MediaTek display controller?

The display controller shall be able to send commands when the *panel*'s .prepare()
callback gets executed - if not, there's something wrong at the display controller
side (driver).

You're probably getting confused by the bridge en/disable callbacks, btw... please
check include/drm/drm_panel.h, struct drm_panel_funcs.

In short, the panel's prepare() should be used for whatever setup is required by
the panel to become available to *receive the video transmission* from the display
controller: this implies that if the panel needs DSI commands for setup, this is
allowed and it's a perfectly fine case.

So, if you are unable to "turn the panel on and wait for it to become ready" in
the panel's .prepare() callback, there's something wrong either in your panel
driver, on in the display controller (the DSI driver) instead.

Since this wasn't happening before your mtk_dsi cleanup, this probably means that
the cleanup is done wrong - and that removing the .start/.stop custom callbacks
from that driver needs you to do something more than just that in order to avoid
regressions.

Unfortunately, I'm pretty busy these days, otherwise I would've gladly made some
research to try and give you some more hints.. but eh :-)

Cheers,
Angelo

> Regards,
> Alexandre
> 
> On 21/03/2025 10:19, Alexandre Mergnat wrote:
>> Hi Angelo,
>> Thanks for the fast feedback :)
>>
>> On 20/03/2025 13:37, AngeloGioacchino Del Regno wrote:
>>> Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
>>>> Currently, the panel set power, set gpio and enable the display link
>>>> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
>>>> panel_bridge_atomic_pre_enable, pointed by
>>>> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
>>>> atomic_pre_enable must not enable the display link
>>>>
>>>> Since the DSI driver is properly inited by the DRM, the panel try to
>>>> communicate with the panel before DSI is powered on.
>>>>
>>>
>>> The panel driver shall still be able to send commands in the .prepare() callback
>>> and if this is not happening anymore... well, there's a problem!
>>
>> Sorry I don't think so, according to that def:
>>      /**
>>       * @pre_enable:
>>       *
>>       * This callback should enable the bridge. It is called right before
>>       * the preceding element in the display pipe is enabled. If the
>>       * preceding element is a bridge this means it's called before that
>>       * bridge's @pre_enable function. If the preceding element is a
>>       * &drm_encoder it's called right before the encoder's
>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>       * &drm_encoder_helper_funcs.dpms hook.
>>       *
>>       * The display pipe (i.e. clocks and timing signals) feeding this bridge
>>       * will not yet be running when this callback is called. The bridge must
>>       * not enable the display link feeding the next bridge in the chain (if
>>       * there is one) when this callback is called.
>>       *
>>       * The @pre_enable callback is optional.
>>       *
>>       * NOTE:
>>       *
>>       * This is deprecated, do not use!
>>       * New drivers shall use &drm_bridge_funcs.atomic_pre_enable.
>>       */
>>      void (*pre_enable)(struct drm_bridge *bridge);
>>
>>      /**
>>       * @enable:
>>       *
>>       * This callback should enable the bridge. It is called right after
>>       * the preceding element in the display pipe is enabled. If the
>>       * preceding element is a bridge this means it's called after that
>>       * bridge's @enable function. If the preceding element is a
>>       * &drm_encoder it's called right after the encoder's
>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>       * &drm_encoder_helper_funcs.dpms hook.
>>       *
>>       * The bridge can assume that the display pipe (i.e. clocks and timing
>>       * signals) feeding it is running when this callback is called. This
>>       * callback must enable the display link feeding the next bridge in the
>>       * chain if there is one.
>>       *
>>       * The @enable callback is optional.
>>       *
>>       * NOTE:
>>       *
>>       * This is deprecated, do not use!
>>       * New drivers shall use &drm_bridge_funcs.atomic_enable.
>>       */
>>      void (*enable)(struct drm_bridge *bridge);
>>
>> => "The bridge must not enable the display link feeding the next bridge in the
>> => chain (if there is one) when this callback is called."
>>
>> Additionally, you ask for something impossible because here is the init order
>> fixed by the framework:
>>
>> [   10.753139] panel_bridge_atomic_pre_enable
>> [   10.963505] mtk_dsi_bridge_atomic_pre_enable
>> [   10.963518] mtk_dsi_bridge_atomic_enable
>> [   10.963527] panel_bridge_atomic_enable
>> [   10.963532] drm_panel_enable
>>
>> If panel want to use the DSI link in panel_bridge_atomic_pre_enable, nothing
>> will happen and  you will get a timeout.
>>
>> So, IMHO, this patch make sense.
>>
>>>
>>>> To solve that, use stk_panel_enable to enable the display link because
>>>> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
>>>> on the DSI.
>>>>
>>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>> ---
>>>>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/ 
>>>> drm/panel/panel-startek-kd070fhfid015.c
>>>> index c0c95355b7435..bc3c4038bf4f5 100644
>>>> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>       gpiod_set_value(stk->enable_gpio, 1);
>>>>       mdelay(20);
>>>>       gpiod_set_value(stk->reset_gpio, 1);
>>>> -    mdelay(10);
>>>> -    ret = stk_panel_init(stk);
>>>> -    if (ret < 0)
>>>> -        goto poweroff;
>>>
>>> Also, you're moving both init and set_display_on to the enable callback...
>>> this is suboptimal.
>>>
>>> You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
>>> should have a .enable() callback that calls DISP ON, a .disable() callback that
>>> calls DISP OFF, and .unprepare() that turns everything off.
>>
>> This is not what I understand from the pre_enable's definition above, and also
>> the function call order by the framework. :)
>>
>>>
>>> Cheers,
>>> Angelo
>>>
>>>> -
>>>> -    ret = stk_panel_on(stk);
>>>> -    if (ret < 0)
>>>> -        goto poweroff;
>>>>       return 0;
>>>> -poweroff:
>>>> -    regulator_disable(stk->supplies[POWER].consumer);
>>>>   iovccoff:
>>>>       regulator_disable(stk->supplies[IOVCC].consumer);
>>>>       gpiod_set_value(stk->reset_gpio, 0);
>>>> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>       return ret;
>>>>   }
>>>> +static int stk_panel_enable(struct drm_panel *panel)
>>>> +{
>>>> +    struct stk_panel *stk = to_stk_panel(panel);
>>>> +    int ret;
>>>> +
>>>> +    ret = stk_panel_init(stk);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    ret = stk_panel_on(stk);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>   static const struct drm_display_mode default_mode = {
>>>>           .clock = 163204,
>>>>           .hdisplay = 1200,
>>>> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>>>>   }
>>>>   static const struct drm_panel_funcs stk_panel_funcs = {
>>>> +    .enable = stk_panel_enable,
>>>>       .unprepare = stk_panel_unprepare,
>>>>       .prepare = stk_panel_prepare,
>>>>       .get_modes = stk_panel_get_modes,
>>>>
>>>
>>>
>>>
>>
> 

Re: [PATCH v8 1/3] arm64: defconfig: enable display support for mt8365-evk

[Alexandre Mergnat]

Hello

On 20/03/2025 09:48, Alexandre Mergnat wrote:
> Enable the DRM HDMI connector support and the MIPI-DSI display
> Startek KD070FHFID015 panel to have HDMI and DSI display working
> on the mt8365-evk board.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/configs/defconfig | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index c62831e615863..1e2963a13500b 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -897,9 +897,11 @@ CONFIG_DRM_PANEL_NOVATEK_NT36672E=m
>   CONFIG_DRM_PANEL_RAYDIUM_RM67191=m
>   CONFIG_DRM_PANEL_SAMSUNG_ATNA33XC20=m
>   CONFIG_DRM_PANEL_SITRONIX_ST7703=m
> +CONFIG_DRM_PANEL_STARTEK_KD070FHFID015=m
>   CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA=m
>   CONFIG_DRM_PANEL_VISIONOX_VTDR6130=m
>   CONFIG_DRM_FSL_LDB=m
> +CONFIG_DRM_DISPLAY_CONNECTOR=m
>   CONFIG_DRM_LONTIUM_LT8912B=m
>   CONFIG_DRM_LONTIUM_LT9611=m
>   CONFIG_DRM_LONTIUM_LT9611UXC=m
> 

Gentle ping.
Is that patch can be applied please ? The remaining patches in this serie
do cleanup only, then shouldn't block this one.

-- 
Regards,
Alexandre

Re: [PATCH v8 2/3] drm/panel: startek-kd070fhfid015: add another init step

[Alexandre Mergnat]

Hi Angelo,

On 15/04/2025 16:46, AngeloGioacchino Del Regno wrote:
> Il 15/04/25 16:13, Alexandre Mergnat ha scritto:
>> Hi Angelo,
>>
>> Gentle ping
>>
>> Let me shortly summarize my problem: I see the panel driver sending commands to the display before 
>> it is ready. My approach to prevent that is to delay sending commands until bridge enable. Your 
>> concern was that during the panel's .prepare() the panel driver should already be able to send 
>> commands through the bridge. Can you please clarify what you think should be the approach to fix 
>> that?
>>
> 
> Please don't top post.
> 
> Anyway - sorry but I missed your reply, that wasn't intentional - thanks for the
> ping (or I wouldn't have replied, duh!).
> 
> What is not ready? The Startek display or the MediaTek display controller?
> 

MediaTek display controller (DSI)

> The display controller shall be able to send commands when the *panel*'s .prepare()
> callback gets executed - if not, there's something wrong at the display controller
> side (driver).
> 

It's explained at the end.

> You're probably getting confused by the bridge en/disable callbacks, btw... please
> check include/drm/drm_panel.h, struct drm_panel_funcs.
> 

panel_bridge_atomic_pre_enable => drm_panel_prepare => (drm_panel_funcs.prepare) stk_panel_prepare
panel_bridge_atomic_enable => drm_panel_enable => (drm_panel_funcs.enable) stk_panel_enable

The bridge en/disable callbacks call panel and DSI enable/disable callbacks, they are linked.

> In short, the panel's prepare() should be used for whatever setup is required by
> the panel to become available to *receive the video transmission* from the display
> controller: this implies that if the panel needs DSI commands for setup, this is
> allowed and it's a perfectly fine case.
> 
> So, if you are unable to "turn the panel on and wait for it to become ready" in
> the panel's .prepare() callback, there's something wrong either in your panel
> driver, on in the display controller (the DSI driver) instead.
> 
> Since this wasn't happening before your mtk_dsi cleanup, this probably means that
> the cleanup is done wrong - and that removing the .start/.stop custom callbacks
> from that driver needs you to do something more than just that in order to avoid
> regressions.
> 

Here the current call order:
[   13.715959] mtk_dsi_ddp_start ( => dsi poweron)
[   13.716797] stk_panel_prepare ( => panel poweron + enable)
[   13.939473] mtk_dsi_bridge_atomic_pre_enable ( => dsi poweron)
[   13.939488] mtk_output_dsi_enable ( => dsi enable)

As you can see, dsi poweron is called twice. According to your comment [1] asking me to remove
custom init function in favor of DRM API call, I've removed "mtk_dsi_ddp_start". Since I don't
find any API to poweron DSI before stk_panel_prepare call, it has been split to do enable
sequence after DSI poweron+enable, because it requiere mipi dsi interface enabled to do
panel enable.

Patched solution:
[   14.164136] stk_panel_prepare ( => panel poweron)
[   14.213300] mtk_dsi_bridge_atomic_pre_enable ( => dsi poweron)
[   14.213623] mtk_output_dsi_enable ( => dsi enable)
[   14.215116] stk_panel_enable ( => panel enable)

The prepare/enable order is fixed by the DRM framework [2]

We still misaligned about the panel's prepare() should be, but even if I try to implement
whatever setup is required by the panel to become available to *receive the video
transmission* from the display controller, the DRM init order doesn't allow it.
I can move stk_panel_prepare content into stk_panel_enable if you prefer, but it's less clean
IMHO because I like to have a first callback for HW/power setup, and a second callback for SW
setup, which fit with bridge callback descriptions.
I don't see a better way to cleanup custom init, and my apologies for your time if I missed something.

[1]: https://lore.kernel.org/all/c2154240-efa1-4c73-aabe-74e938a75af1@collabora.com/
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_bridge.c#n769

> Unfortunately, I'm pretty busy these days, otherwise I would've gladly made some
> research to try and give you some more hints.. but eh :-)
> 
> Cheers,
> Angelo
> 
>> Regards,
>> Alexandre
>>
>> On 21/03/2025 10:19, Alexandre Mergnat wrote:
>>> Hi Angelo,
>>> Thanks for the fast feedback :)
>>>
>>> On 20/03/2025 13:37, AngeloGioacchino Del Regno wrote:
>>>> Il 20/03/25 09:48, Alexandre Mergnat ha scritto:
>>>>> Currently, the panel set power, set gpio and enable the display link
>>>>> in stk_panel_prepare, pointed by drm_panel_funcs.prepare, called by
>>>>> panel_bridge_atomic_pre_enable, pointed by
>>>>> drm_bridge_funcs.atomic_pre_enable. According to the drm_bridge.h,
>>>>> atomic_pre_enable must not enable the display link
>>>>>
>>>>> Since the DSI driver is properly inited by the DRM, the panel try to
>>>>> communicate with the panel before DSI is powered on.
>>>>>
>>>>
>>>> The panel driver shall still be able to send commands in the .prepare() callback
>>>> and if this is not happening anymore... well, there's a problem!
>>>
>>> Sorry I don't think so, according to that def:
>>>      /**
>>>       * @pre_enable:
>>>       *
>>>       * This callback should enable the bridge. It is called right before
>>>       * the preceding element in the display pipe is enabled. If the
>>>       * preceding element is a bridge this means it's called before that
>>>       * bridge's @pre_enable function. If the preceding element is a
>>>       * &drm_encoder it's called right before the encoder's
>>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>>       * &drm_encoder_helper_funcs.dpms hook.
>>>       *
>>>       * The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>       * will not yet be running when this callback is called. The bridge must
>>>       * not enable the display link feeding the next bridge in the chain (if
>>>       * there is one) when this callback is called.
>>>       *
>>>       * The @pre_enable callback is optional.
>>>       *
>>>       * NOTE:
>>>       *
>>>       * This is deprecated, do not use!
>>>       * New drivers shall use &drm_bridge_funcs.atomic_pre_enable.
>>>       */
>>>      void (*pre_enable)(struct drm_bridge *bridge);
>>>
>>>      /**
>>>       * @enable:
>>>       *
>>>       * This callback should enable the bridge. It is called right after
>>>       * the preceding element in the display pipe is enabled. If the
>>>       * preceding element is a bridge this means it's called after that
>>>       * bridge's @enable function. If the preceding element is a
>>>       * &drm_encoder it's called right after the encoder's
>>>       * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
>>>       * &drm_encoder_helper_funcs.dpms hook.
>>>       *
>>>       * The bridge can assume that the display pipe (i.e. clocks and timing
>>>       * signals) feeding it is running when this callback is called. This
>>>       * callback must enable the display link feeding the next bridge in the
>>>       * chain if there is one.
>>>       *
>>>       * The @enable callback is optional.
>>>       *
>>>       * NOTE:
>>>       *
>>>       * This is deprecated, do not use!
>>>       * New drivers shall use &drm_bridge_funcs.atomic_enable.
>>>       */
>>>      void (*enable)(struct drm_bridge *bridge);
>>>
>>> => "The bridge must not enable the display link feeding the next bridge in the
>>> => chain (if there is one) when this callback is called."
>>>
>>> Additionally, you ask for something impossible because here is the init order
>>> fixed by the framework:
>>>
>>> [   10.753139] panel_bridge_atomic_pre_enable
>>> [   10.963505] mtk_dsi_bridge_atomic_pre_enable
>>> [   10.963518] mtk_dsi_bridge_atomic_enable
>>> [   10.963527] panel_bridge_atomic_enable
>>> [   10.963532] drm_panel_enable
>>>
>>> If panel want to use the DSI link in panel_bridge_atomic_pre_enable, nothing
>>> will happen and  you will get a timeout.
>>>
>>> So, IMHO, this patch make sense.
>>>
>>>>
>>>>> To solve that, use stk_panel_enable to enable the display link because
>>>>> it's called after the mtk_dsi_bridge_atomic_pre_enable which is power
>>>>> on the DSI.
>>>>>
>>>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>>> ---
>>>>>   .../gpu/drm/panel/panel-startek-kd070fhfid015.c    | 25 +++++++++++++---------
>>>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/ 
>>>>> drm/panel/panel-startek-kd070fhfid015.c
>>>>> index c0c95355b7435..bc3c4038bf4f5 100644
>>>>> --- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>>> +++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
>>>>> @@ -135,19 +135,9 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>>       gpiod_set_value(stk->enable_gpio, 1);
>>>>>       mdelay(20);
>>>>>       gpiod_set_value(stk->reset_gpio, 1);
>>>>> -    mdelay(10);
>>>>> -    ret = stk_panel_init(stk);
>>>>> -    if (ret < 0)
>>>>> -        goto poweroff;
>>>>
>>>> Also, you're moving both init and set_display_on to the enable callback...
>>>> this is suboptimal.
>>>>
>>>> You should do the DrIC setup in .prepare() (can include SLEEP OUT), and then you
>>>> should have a .enable() callback that calls DISP ON, a .disable() callback that
>>>> calls DISP OFF, and .unprepare() that turns everything off.
>>>
>>> This is not what I understand from the pre_enable's definition above, and also
>>> the function call order by the framework. :)
>>>
>>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>> -
>>>>> -    ret = stk_panel_on(stk);
>>>>> -    if (ret < 0)
>>>>> -        goto poweroff;
>>>>>       return 0;
>>>>> -poweroff:
>>>>> -    regulator_disable(stk->supplies[POWER].consumer);
>>>>>   iovccoff:
>>>>>       regulator_disable(stk->supplies[IOVCC].consumer);
>>>>>       gpiod_set_value(stk->reset_gpio, 0);
>>>>> @@ -156,6 +146,20 @@ static int stk_panel_prepare(struct drm_panel *panel)
>>>>>       return ret;
>>>>>   }
>>>>> +static int stk_panel_enable(struct drm_panel *panel)
>>>>> +{
>>>>> +    struct stk_panel *stk = to_stk_panel(panel);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = stk_panel_init(stk);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = stk_panel_on(stk);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>>   static const struct drm_display_mode default_mode = {
>>>>>           .clock = 163204,
>>>>>           .hdisplay = 1200,
>>>>> @@ -239,6 +243,7 @@ drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
>>>>>   }
>>>>>   static const struct drm_panel_funcs stk_panel_funcs = {
>>>>> +    .enable = stk_panel_enable,
>>>>>       .unprepare = stk_panel_unprepare,
>>>>>       .prepare = stk_panel_prepare,
>>>>>       .get_modes = stk_panel_get_modes,
>>>>>
>>>>
>>>>
>>>>
>>>
>>
> 

-- 
Regards,
Alexandre