[PATCH v4 00/11] drm/bridge: cdns-dsi: Fix the color-shift issue

[PATCH v4 00/11] drm/bridge: cdns-dsi: Fix the color-shift issue

[Aradhya Bhatia]

Hello all,

This series provides some crucial fixes and improvements for the Cadence's DSI
TX (cdns-dsi) controller found commonly in Texas Instruments' J7 family of SoCs
as well as in AM62P.

Along with that, this series aims to fix the color-shift issue that has been
going on with the DSI controller. This controller requires to be enabled before
the previous entity enables its stream[0]. It's a strict requirement which, if
not followed, causes the colors to "shift" on the display. The fix happens in
2 steps.

    1. The bridge pre_enable calls have been shifted before the crtc_enable and
       the bridge post_disable calls have been shifted after the crtc_disable.
       This has been done as per the definition of bridge pre_enable.

       "The display pipe (i.e. clocks and timing signals) feeding this bridge
       will not yet be running when this callback is called".

       Since CRTC is also a source feeding the bridge, it should not be enabled
       before the bridges in the pipeline are pre_enabled.

       The sequence of enable after this patch will look like:

	        bridge[n]_pre_enable
	        ...
	        bridge[1]_pre_enable

	        crtc_enable
	        encoder_enable

	        bridge[1]_enable
	        ...
	        bridge[n]_enable

       and vice-versa for the bridge chain disable sequence.


    2. The cdns-dsi enable / disable sequences have now been moved to pre_enable
       and post_disable sequences. This is the only way to have cdns-dsi drivers
       be up and ready before the previous entity is enables its streaming.

The DSI also spec requires the Clock and Data Lanes be ready before the DSI TX
enables its stream[0]. A patch has been added to make the code wait for that to
happen. Going ahead with further DSI (and DSS configuration), while the lanes
are not ready, has been found to be another reason for shift in colors.

These patches have been tested with J721E based BeagleboneAI64 along with a
RaspberryPi 7" DSI panel. The extra patches can be found in the
"next_dsi-v4-tests" branch of my github fork[1] for anyone who would like to
test them.

Thanks,
Aradhya


[0]: Section 12.6.5.7.3: "Start-up Procedure" [For DSI TX controller]
     in TDA4VM Technical Reference Manual https://www.ti.com/lit/zip/spruil1

[1]: https://github.com/aradhya07/linux-ab/tree/next_dsi-v4-tests

Change Log:

  - Changes in v4:
    - Add new patch, "drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()",
      to update to an auto-managed way of finding next bridge in the chain.
    - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized variable" and
      add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that properly
      de-initializes the Phy and maintains the initialization state.
    - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO" to explain
      the HW concerns better.
    - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.

  - Changes in v3:
    - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix OF node
      pointer".
    - Add a new helper API to figure out DSI host input pixel format
      in patch "drm/mipi-dsi: Add helper to find input format".
    - Use a common function for bridge pre-enable and enable, and bridge disable
      and post-disable, to avoid code duplication.
    - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add it in v2).
    - Add R-b tag from Dmitry Baryshkov for patch 8/10.

  - Changes in v2:
    - Drop patch "drm/tidss: Add CRTC mode_fixup"
    - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4 separate ones
    - Drop support for early_enable/late_disable APIs and instead re-order the
      pre_enable / post_disable APIs to be called before / after crtc_enable /
      crtc_disable.
    - Drop support for early_enable/late_disable in cdns-dsi and use
      pre_enable/post_disable APIs instead to do bridge enable/disable.


Previous versions:

v1: https://lore.kernel.org/all/20240511153051.1355825-1-a-bhatia1@ti.com/
v2: https://lore.kernel.org/all/20240530093621.1925863-1-a-bhatia1@ti.com/
v3: https://lore.kernel.org/all/20240617105311.1587489-1-a-bhatia1@ti.com/

Aradhya Bhatia (11):
  drm/bridge: cdns-dsi: Fix OF node pointer
  drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()
  drm/bridge: cdns-dsi: Fix Phy _init() and _exit()
  drm/bridge: cdns-dsi: Fix the link and phy init order
  drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()
  drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready
  drm/bridge: cdns-dsi: Reset the DCS write FIFO
  drm/mipi-dsi: Add helper to find input format
  drm/bridge: cdns-dsi: Support atomic bridge APIs
  drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
  drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 105 ++++++-----
 .../gpu/drm/bridge/cadence/cdns-dsi-core.h    |   2 -
 drivers/gpu/drm/drm_atomic_helper.c           | 165 ++++++++++++------
 drivers/gpu/drm/drm_mipi_dsi.c                |  37 ++++
 include/drm/drm_atomic_helper.h               |   7 +
 include/drm/drm_mipi_dsi.h                    |   1 +
 6 files changed, 209 insertions(+), 108 deletions(-)


base-commit: b992b79ca8bc336fa8e2c80990b5af80ed8f36fd
-- 
2.34.1

[PATCH v4 01/11] drm/bridge: cdns-dsi: Fix OF node pointer

[Aradhya Bhatia]

Fix the OF node pointer passed to the of_drm_find_bridge() call to find
the next bridge in the display chain.

To find the next bridge in the pipeline, we need to pass "np" - the OF
node pointer of the next entity in the devicetree chain. Passing
"of_node" to of_drm_find_bridge will make the function try to fetch the
bridge for the cdns-dsi which is not what's required.

Fix that.

Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 7457d38622b0..b016f2ba06bb 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -952,7 +952,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 		bridge = drm_panel_bridge_add_typed(panel,
 						    DRM_MODE_CONNECTOR_DSI);
 	} else {
-		bridge = of_drm_find_bridge(dev->dev.of_node);
+		bridge = of_drm_find_bridge(np);
 		if (!bridge)
 			bridge = ERR_PTR(-EINVAL);
 	}
-- 
2.34.1

Re: [PATCH v4 01/11] drm/bridge: cdns-dsi: Fix OF node pointer

[Tomi Valkeinen]

Hi,

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Fix the OF node pointer passed to the of_drm_find_bridge() call to find
> the next bridge in the display chain.
> 
> To find the next bridge in the pipeline, we need to pass "np" - the OF
> node pointer of the next entity in the devicetree chain. Passing
> "of_node" to of_drm_find_bridge will make the function try to fetch the
> bridge for the cdns-dsi which is not what's required.
> 
> Fix that.

The code looks fine, but I'd write the subject and desc from a different 
perspective. The subject could be something like "Fix connecting to a 
sink bridge", and the desc could first say that connecting the sink to a 
DSI panel works, but connecting to a bridge fails, as wrong OF node is 
passed to of_drm_find_bridge().

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 7457d38622b0..b016f2ba06bb 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -952,7 +952,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
>   		bridge = drm_panel_bridge_add_typed(panel,
>   						    DRM_MODE_CONNECTOR_DSI);
>   	} else {
> -		bridge = of_drm_find_bridge(dev->dev.of_node);
> +		bridge = of_drm_find_bridge(np);
>   		if (!bridge)
>   			bridge = ERR_PTR(-EINVAL);
>   	}

[PATCH v4 02/11] drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()

[Aradhya Bhatia]

Instead of manually finding the next bridge/panel, and maintaining the
panel-bridge (in-case the next entity is a panel), switch to using the
automatically managing devm_drm_of_get_bridge() API.

Drop the drm_panel support completely from the driver while at it.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 28 ++-----------------
 .../gpu/drm/bridge/cadence/cdns-dsi-core.h    |  2 --
 2 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b016f2ba06bb..5159c3f0853e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -920,8 +920,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 	struct cdns_dsi_output *output = &dsi->output;
 	struct cdns_dsi_input *input = &dsi->input;
 	struct drm_bridge *bridge;
-	struct drm_panel *panel;
-	struct device_node *np;
 	int ret;
 
 	/*
@@ -939,26 +937,10 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 	/*
 	 * The host <-> device link might be described using an OF-graph
 	 * representation, in this case we extract the device of_node from
-	 * this representation, otherwise we use dsidev->dev.of_node which
-	 * should have been filled by the core.
+	 * this representation.
 	 */
-	np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
-				      dev->channel);
-	if (!np)
-		np = of_node_get(dev->dev.of_node);
-
-	panel = of_drm_find_panel(np);
-	if (!IS_ERR(panel)) {
-		bridge = drm_panel_bridge_add_typed(panel,
-						    DRM_MODE_CONNECTOR_DSI);
-	} else {
-		bridge = of_drm_find_bridge(np);
-		if (!bridge)
-			bridge = ERR_PTR(-EINVAL);
-	}
-
-	of_node_put(np);
-
+	bridge = devm_drm_of_get_bridge(dsi->base.dev, dsi->base.dev->of_node,
+					DSI_OUTPUT_PORT, dev->channel);
 	if (IS_ERR(bridge)) {
 		ret = PTR_ERR(bridge);
 		dev_err(host->dev, "failed to add DSI device %s (err = %d)",
@@ -968,7 +950,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 
 	output->dev = dev;
 	output->bridge = bridge;
-	output->panel = panel;
 
 	/*
 	 * The DSI output has been properly configured, we can now safely
@@ -984,12 +965,9 @@ static int cdns_dsi_detach(struct mipi_dsi_host *host,
 			   struct mipi_dsi_device *dev)
 {
 	struct cdns_dsi *dsi = to_cdns_dsi(host);
-	struct cdns_dsi_output *output = &dsi->output;
 	struct cdns_dsi_input *input = &dsi->input;
 
 	drm_bridge_remove(&input->bridge);
-	if (output->panel)
-		drm_panel_bridge_remove(output->bridge);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
index ca7ea2da635c..5db5dbbbcaad 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
@@ -10,7 +10,6 @@
 
 #include <drm/drm_bridge.h>
 #include <drm/drm_mipi_dsi.h>
-#include <drm/drm_panel.h>
 
 #include <linux/bits.h>
 #include <linux/completion.h>
@@ -21,7 +20,6 @@ struct reset_control;
 
 struct cdns_dsi_output {
 	struct mipi_dsi_device *dev;
-	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 	union phy_configure_opts phy_opts;
 };
-- 
2.34.1

Re: [PATCH v4 02/11] drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Instead of manually finding the next bridge/panel, and maintaining the
> panel-bridge (in-case the next entity is a panel), switch to using the
> automatically managing devm_drm_of_get_bridge() API.
> 
> Drop the drm_panel support completely from the driver while at it.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> ---
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 28 ++-----------------
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.h    |  2 --
>   2 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index b016f2ba06bb..5159c3f0853e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -920,8 +920,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
>   	struct cdns_dsi_output *output = &dsi->output;
>   	struct cdns_dsi_input *input = &dsi->input;
>   	struct drm_bridge *bridge;
> -	struct drm_panel *panel;
> -	struct device_node *np;
>   	int ret;
>   
>   	/*
> @@ -939,26 +937,10 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
>   	/*
>   	 * The host <-> device link might be described using an OF-graph
>   	 * representation, in this case we extract the device of_node from
> -	 * this representation, otherwise we use dsidev->dev.of_node which
> -	 * should have been filled by the core.
> +	 * this representation.
>   	 */
> -	np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
> -				      dev->channel);
> -	if (!np)
> -		np = of_node_get(dev->dev.of_node);
> -
> -	panel = of_drm_find_panel(np);
> -	if (!IS_ERR(panel)) {
> -		bridge = drm_panel_bridge_add_typed(panel,
> -						    DRM_MODE_CONNECTOR_DSI);
> -	} else {
> -		bridge = of_drm_find_bridge(np);
> -		if (!bridge)
> -			bridge = ERR_PTR(-EINVAL);
> -	}
> -
> -	of_node_put(np);
> -
> +	bridge = devm_drm_of_get_bridge(dsi->base.dev, dsi->base.dev->of_node,
> +					DSI_OUTPUT_PORT, dev->channel);
>   	if (IS_ERR(bridge)) {
>   		ret = PTR_ERR(bridge);
>   		dev_err(host->dev, "failed to add DSI device %s (err = %d)",
> @@ -968,7 +950,6 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
>   
>   	output->dev = dev;
>   	output->bridge = bridge;
> -	output->panel = panel;
>   
>   	/*
>   	 * The DSI output has been properly configured, we can now safely
> @@ -984,12 +965,9 @@ static int cdns_dsi_detach(struct mipi_dsi_host *host,
>   			   struct mipi_dsi_device *dev)
>   {
>   	struct cdns_dsi *dsi = to_cdns_dsi(host);
> -	struct cdns_dsi_output *output = &dsi->output;
>   	struct cdns_dsi_input *input = &dsi->input;
>   
>   	drm_bridge_remove(&input->bridge);
> -	if (output->panel)
> -		drm_panel_bridge_remove(output->bridge);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
> index ca7ea2da635c..5db5dbbbcaad 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.h
> @@ -10,7 +10,6 @@
>   
>   #include <drm/drm_bridge.h>
>   #include <drm/drm_mipi_dsi.h>
> -#include <drm/drm_panel.h>
>   
>   #include <linux/bits.h>
>   #include <linux/completion.h>
> @@ -21,7 +20,6 @@ struct reset_control;
>   
>   struct cdns_dsi_output {
>   	struct mipi_dsi_device *dev;
> -	struct drm_panel *panel;
>   	struct drm_bridge *bridge;
>   	union phy_configure_opts phy_opts;
>   };

[PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()

[Aradhya Bhatia]

Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
during the _suspend().

Also power-off the Phy from bridge_disable.

Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 5159c3f0853e..d89c32bae2b9 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 	if (dsi->platform_ops && dsi->platform_ops->disable)
 		dsi->platform_ops->disable(dsi);
 
+	phy_power_off(dsi->dphy);
+	dsi->link_initialized = false;
+	dsi->phy_initialized = false;
+
 	pm_runtime_put(dsi->base.dev);
 }
 
@@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 	       DPHY_CMN_PDN | DPHY_PLL_PDN,
 	       dsi->regs + MCTL_DPHY_CFG0);
 
-	phy_init(dsi->dphy);
 	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
 	phy_configure(dsi->dphy, &output->phy_opts);
 	phy_power_on(dsi->dphy);
@@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev)
 	clk_prepare_enable(dsi->dsi_p_clk);
 	clk_prepare_enable(dsi->dsi_sys_clk);
 
+	phy_init(dsi->dphy);
+
 	return 0;
 }
 
@@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
 {
 	struct cdns_dsi *dsi = dev_get_drvdata(dev);
 
+	phy_exit(dsi->dphy);
+
 	clk_disable_unprepare(dsi->dsi_sys_clk);
 	clk_disable_unprepare(dsi->dsi_p_clk);
 	reset_control_assert(dsi->dsi_p_rst);
-	dsi->link_initialized = false;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()

[Tomi Valkeinen]

Hi,

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
> during the _suspend().
> 
> Also power-off the Phy from bridge_disable.
> 
> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 5159c3f0853e..d89c32bae2b9 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
>   	if (dsi->platform_ops && dsi->platform_ops->disable)
>   		dsi->platform_ops->disable(dsi);
>   
> +	phy_power_off(dsi->dphy);
> +	dsi->link_initialized = false;
> +	dsi->phy_initialized = false;
> +
>   	pm_runtime_put(dsi->base.dev);
>   }
>   
> @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>   	       DPHY_CMN_PDN | DPHY_PLL_PDN,
>   	       dsi->regs + MCTL_DPHY_CFG0);
>   
> -	phy_init(dsi->dphy);
>   	phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
>   	phy_configure(dsi->dphy, &output->phy_opts);
>   	phy_power_on(dsi->dphy);
> @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev)
>   	clk_prepare_enable(dsi->dsi_p_clk);
>   	clk_prepare_enable(dsi->dsi_sys_clk);
>   
> +	phy_init(dsi->dphy);
> +
>   	return 0;
>   }
>   
> @@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
>   {
>   	struct cdns_dsi *dsi = dev_get_drvdata(dev);
>   
> +	phy_exit(dsi->dphy);
> +
>   	clk_disable_unprepare(dsi->dsi_sys_clk);
>   	clk_disable_unprepare(dsi->dsi_p_clk);
>   	reset_control_assert(dsi->dsi_p_rst);
> -	dsi->link_initialized = false;
>   	return 0;
>   }
>   

So with this patch, phy_init/exit will be called in the resume/suspend 
functions. That looks fine.

But the phy_power_on/phy_power_off looks odd to me. Here you add 
phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But 
phy_power_on() is called in cdns_dsi_hs_init(), and that is called in 
cdns_dsi_bridge_enable() (which sounds fine), but also in 
cdns_dsi_bridge_pre_enable().

So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable() 
is extra, as it effectively does nothing (it exists right away if 
dsi->phy_initialized == true)?

  Tomi

Re: [PATCH v4 03/11] drm/bridge: cdns-dsi: Fix Phy _init() and _exit()

[Aradhya Bhatia]

Hi Tomi,

Thanks for reviewing the patches!

On 26/06/24 15:55, Tomi Valkeinen wrote:
> Hi,
> 
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>> Initialize the Phy during the cdns-dsi _resume(), and de-initialize it
>> during the _suspend().
>>
>> Also power-off the Phy from bridge_disable.
>>
>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 5159c3f0853e..d89c32bae2b9 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct
>> drm_bridge *bridge)
>>       if (dsi->platform_ops && dsi->platform_ops->disable)
>>           dsi->platform_ops->disable(dsi);
>>   +    phy_power_off(dsi->dphy);
>> +    dsi->link_initialized = false;
>> +    dsi->phy_initialized = false;
>> +
>>       pm_runtime_put(dsi->base.dev);
>>   }
>>   @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>>              DPHY_CMN_PDN | DPHY_PLL_PDN,
>>              dsi->regs + MCTL_DPHY_CFG0);
>>   -    phy_init(dsi->dphy);
>>       phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
>>       phy_configure(dsi->dphy, &output->phy_opts);
>>       phy_power_on(dsi->dphy);
>> @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct
>> device *dev)
>>       clk_prepare_enable(dsi->dsi_p_clk);
>>       clk_prepare_enable(dsi->dsi_sys_clk);
>>   +    phy_init(dsi->dphy);
>> +
>>       return 0;
>>   }
>>   @@ -1127,10 +1132,11 @@ static int __maybe_unused
>> cdns_dsi_suspend(struct device *dev)
>>   {
>>       struct cdns_dsi *dsi = dev_get_drvdata(dev);
>>   +    phy_exit(dsi->dphy);
>> +
>>       clk_disable_unprepare(dsi->dsi_sys_clk);
>>       clk_disable_unprepare(dsi->dsi_p_clk);
>>       reset_control_assert(dsi->dsi_p_rst);
>> -    dsi->link_initialized = false;
>>       return 0;
>>   }
>>   
> 
> So with this patch, phy_init/exit will be called in the resume/suspend
> functions. That looks fine.
> 
> But the phy_power_on/phy_power_off looks odd to me. Here you add
> phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But
> phy_power_on() is called in cdns_dsi_hs_init(), and that is called in
> cdns_dsi_bridge_enable() (which sounds fine), but also in
> cdns_dsi_bridge_pre_enable().
> 
> So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable()
> is extra, as it effectively does nothing (it exists right away if
> dsi->phy_initialized == true)?

That's right. When cdns_dsi_hs_init() is called from
cdns_dsi_bridge_enable(), it is simply expected to return since
phy_initialized is true.

I am not aware about the exact reasoning behind this, but this gets
addressed when I convert the _bridge_enable() to _bridge_pre_enable()
and drop the older _bridge_pre_enable() entirely.


Regards
Aradhya

[PATCH v4 04/11] drm/bridge: cdns-dsi: Fix the link and phy init order

[Aradhya Bhatia]

The order of init of DSI link and DSI phy is wrong. The DSI link needs
to be configured before the DSI phy is getting configured. Otherwise,
the D-Phy is unable to lock in on the incoming PLL Reference clock[0].

Fix the order of inits.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index d89c32bae2b9..03a5af52ec0b 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -778,8 +778,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 
 	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
 
-	cdns_dsi_hs_init(dsi);
 	cdns_dsi_init_link(dsi);
+	cdns_dsi_hs_init(dsi);
 
 	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
 	       dsi->regs + VID_HSIZE1);
-- 
2.34.1

Re: [PATCH v4 04/11] drm/bridge: cdns-dsi: Fix the link and phy init order

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> The order of init of DSI link and DSI phy is wrong. The DSI link needs
> to be configured before the DSI phy is getting configured. Otherwise,
> the D-Phy is unable to lock in on the incoming PLL Reference clock[0].
> 
> Fix the order of inits.
> 
> [0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
>       TRM Link: http://www.ti.com/lit/pdf/spruil1
> 
> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index d89c32bae2b9..03a5af52ec0b 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -778,8 +778,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>   
>   	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
>   
> -	cdns_dsi_hs_init(dsi);
>   	cdns_dsi_init_link(dsi);
> +	cdns_dsi_hs_init(dsi);
>   
>   	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
>   	       dsi->regs + VID_HSIZE1);

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

[PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()

[Aradhya Bhatia]

Allow the D-Phy config checks to use mode->clock instead of
mode->crtc_clock during mode_valid checks, like everywhere else in the
driver.

Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 03a5af52ec0b..426f77092341 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+	phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock : mode->crtc_clock) * 1000,
 					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					 nlanes, phy_cfg);
 
-- 
2.34.1

Re: [PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Allow the D-Phy config checks to use mode->clock instead of
> mode->crtc_clock during mode_valid checks, like everywhere else in the
> driver.
> 
> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 03a5af52ec0b..426f77092341 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>   	if (ret)
>   		return ret;
>   
> -	phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
> +	phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock : mode->crtc_clock) * 1000,
>   					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
>   					 nlanes, phy_cfg);
>   

I think this is fine as a fix.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

However... The code looks a bit messy. Maybe the first one is something 
that could be addressed in this series.

- Return value of phy_mipi_dphy_get_default_config() is not checked

- Using the non-crtc and crtc versions of the timings this way looks 
bad, but that's not a problem of the driver. It would be better to have 
a struct that contains the timings, and struct drm_display_mode would 
contain two instances of that struct. The driver code could then just 
pick the correct instance, instead of making the choice for each and 
every field. This would be an interesting coccinelle project ;)

- Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd. 
Everything should already have been checked. In fact, at the check phase 
the resulting config values could have been stored somewhere, so that 
they're ready for use by cdns_dsi_bridge_enable(). But this rises the 
question if the non-crtc and crtc timings can actually be different, and 
if they are... doesn't it break everything if at the check phase we use 
the non-crtc ones, but at enable phase we use crtc ones?

Ah, I see, this is with non-atomic. Maybe after you switch to atomic 
callbacks, atomic_check could be used so that there's no need for the 
WARN_ON_ONCE() in enable callback.

  Tomi

Re: [PATCH v4 05/11] drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()

[Aradhya Bhatia]

On 26/06/24 16:17, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>> Allow the D-Phy config checks to use mode->clock instead of
>> mode->crtc_clock during mode_valid checks, like everywhere else in the
>> driver.
>>
>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 03a5af52ec0b..426f77092341 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>       if (ret)
>>           return ret;
>>   -    phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>> +    phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock
>> : mode->crtc_clock) * 1000,
>>                        mipi_dsi_pixel_format_to_bpp(output->dev->format),
>>                        nlanes, phy_cfg);
>>   
> 
> I think this is fine as a fix.
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> However... The code looks a bit messy. Maybe the first one is something
> that could be addressed in this series.
> 
> - Return value of phy_mipi_dphy_get_default_config() is not checked

Sure, I can fix that.

> 
> - Using the non-crtc and crtc versions of the timings this way looks
> bad, but that's not a problem of the driver. It would be better to have
> a struct that contains the timings, and struct drm_display_mode would
> contain two instances of that struct. The driver code could then just
> pick the correct instance, instead of making the choice for each and
> every field. This would be an interesting coccinelle project ;)
> 
> - Calling cdns_dsi_check_conf() in cdns_dsi_bridge_enable() is odd.
> Everything should already have been checked. In fact, at the check phase
> the resulting config values could have been stored somewhere, so that
> they're ready for use by cdns_dsi_bridge_enable(). But this rises the
> question if the non-crtc and crtc timings can actually be different, and
> if they are... doesn't it break everything if at the check phase we use
> the non-crtc ones, but at enable phase we use crtc ones?

It'd appear that it does. I don't fully understand why the driver uses
non-crtc_* timing parameters during the check phase, only to use the
crtc_* timing parameters during _enable().

Since with tidss, both the sets are same, I haven't had to think too
much about this! =)

What is the ideal way that this should get addressed though? If we have
an agreeable resolution then maybe I can fix that as well.

> 
> Ah, I see, this is with non-atomic. Maybe after you switch to atomic
> callbacks, atomic_check could be used so that there's no need for the
> WARN_ON_ONCE() in enable callback.
> 

Yes, I think this would be better. We can use atomic_check() to verify
the crtc_* timing parameters, while the already existing mode_valid()
can continue checking the non-crtc_* ones.

I will add this change when I am adding other atomic_* APIs later in the
series.


Regards
Aradhya

[PATCH v4 06/11] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready

[Aradhya Bhatia]

Once the DSI Link and DSI Phy are initialized, the code needs to wait
for Clk and Data Lanes to be ready, before continuing configuration.
This is in accordance with the DSI Start-up procedure, found in the
Technical Reference Manual of Texas Instrument's J721E SoC[0] which
houses this DSI TX controller.

If the previous bridge (or crtc/encoder) are configured pre-maturely,
the input signal FIFO gets corrupt. This introduces a color-shift on the
display.

Allow the driver to wait for the clk and data lanes to get ready during
DSI enable.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Tested-by: Dominik Haller <d.haller@phytec.de>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 426f77092341..126e4bccd868 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -764,7 +764,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
 	struct cdns_dsi_cfg dsi_cfg;
-	u32 tmp, reg_wakeup, div;
+	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -781,6 +781,17 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 
+	/*
+	 * Now that the DSI Link and DSI Phy are initialized,
+	 * wait for the CLK and Data Lanes to be ready.
+	 */
+	tmp = CLK_LANE_RDY;
+	for (int i = 0; i < nlanes; i++)
+		tmp |= DATA_LANE_RDY(i);
+
+	WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+					status & tmp, 100, 0));
+
 	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
 	       dsi->regs + VID_HSIZE1);
 	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
-- 
2.34.1

Re: [PATCH v4 06/11] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Once the DSI Link and DSI Phy are initialized, the code needs to wait
> for Clk and Data Lanes to be ready, before continuing configuration.
> This is in accordance with the DSI Start-up procedure, found in the
> Technical Reference Manual of Texas Instrument's J721E SoC[0] which
> houses this DSI TX controller.
> 
> If the previous bridge (or crtc/encoder) are configured pre-maturely,
> the input signal FIFO gets corrupt. This introduces a color-shift on the
> display.
> 
> Allow the driver to wait for the clk and data lanes to get ready during
> DSI enable.
> 
> [0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
>       TRM Link: http://www.ti.com/lit/pdf/spruil1
> 
> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Tested-by: Dominik Haller <d.haller@phytec.de>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 426f77092341..126e4bccd868 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -764,7 +764,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>   	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>   	unsigned long tx_byte_period;
>   	struct cdns_dsi_cfg dsi_cfg;
> -	u32 tmp, reg_wakeup, div;
> +	u32 tmp, reg_wakeup, div, status;
>   	int nlanes;
>   
>   	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> @@ -781,6 +781,17 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>   	cdns_dsi_init_link(dsi);
>   	cdns_dsi_hs_init(dsi);
>   
> +	/*
> +	 * Now that the DSI Link and DSI Phy are initialized,
> +	 * wait for the CLK and Data Lanes to be ready.
> +	 */
> +	tmp = CLK_LANE_RDY;
> +	for (int i = 0; i < nlanes; i++)
> +		tmp |= DATA_LANE_RDY(i);
> +
> +	WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
> +					status & tmp, 100, 0));

I think an error print is more suitable than WARN_ON_ONCE(). Other than 
that:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> +
>   	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
>   	       dsi->regs + VID_HSIZE1);
>   	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),

[PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO

[Aradhya Bhatia]

If any normal DCS write command has already been transmitted prior to
transmitting any Zero-Parameter DCS command, then it is necessary to
clear the TX FIFO by resetting it. Otherwise, the FIFO points to another
location, and the DCS command transmits unnecessary data causing the
panel to not work[0].

Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule,
before any DCS packet is transmitted to the DSI peripheral.

[0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical
     Reference Manual: https://www.ti.com/lit/zip/spruil1

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 126e4bccd868..cad0c1478ef0 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
 
 	cdns_dsi_init_link(dsi);
 
+	/* Reset the DCS Write FIFO */
+	writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST);
+
 	ret = mipi_dsi_create_packet(&packet, msg);
 	if (ret)
 		goto out;
-- 
2.34.1

Re: [PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> If any normal DCS write command has already been transmitted prior to
> transmitting any Zero-Parameter DCS command, then it is necessary to
> clear the TX FIFO by resetting it. Otherwise, the FIFO points to another
> location, and the DCS command transmits unnecessary data causing the
> panel to not work[0].
> 
> Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule,
> before any DCS packet is transmitted to the DSI peripheral.
> 
> [0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical
>       Reference Manual: https://www.ti.com/lit/zip/spruil1

Hmm so if I read the doc right, it says: if sending zero-parameter dcs 
command, clear the FIFO and write zero to direct_cmd_wrdat.

Your patch seems to always clear the FIFO, not only for zero-parameter 
commands. Is that a problem (I don't think so, but...)?

Also, is the direct_cmd_wrdat written at all when sending zero-parameter 
dcs command?

  Tomi

> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 126e4bccd868..cad0c1478ef0 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
>   
>   	cdns_dsi_init_link(dsi);
>   
> +	/* Reset the DCS Write FIFO */
> +	writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST);
> +
>   	ret = mipi_dsi_create_packet(&packet, msg);
>   	if (ret)
>   		goto out;

Re: [PATCH v4 07/11] drm/bridge: cdns-dsi: Reset the DCS write FIFO

[Aradhya Bhatia]

On 26/06/24 16:33, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>> If any normal DCS write command has already been transmitted prior to
>> transmitting any Zero-Parameter DCS command, then it is necessary to
>> clear the TX FIFO by resetting it. Otherwise, the FIFO points to another
>> location, and the DCS command transmits unnecessary data causing the
>> panel to not work[0].
>>
>> Allow the DCS Write FIFO in the cdns-dsi controller to reset as a rule,
>> before any DCS packet is transmitted to the DSI peripheral.
>>
>> [0]: Section 12.6.5.7.5.2: "Command Mode Settings" in TDA4VM Technical
>>       Reference Manual: https://www.ti.com/lit/zip/spruil1
> 
> Hmm so if I read the doc right, it says: if sending zero-parameter dcs
> command, clear the FIFO and write zero to direct_cmd_wrdat.

That's right.

> 
> Your patch seems to always clear the FIFO, not only for zero-parameter
> commands. Is that a problem (I don't think so, but...)?
> 

My patch does clear the FIFO every time.

While there is no documentation that says its harmless, I have tested
the patches with RPi Panel (which doesn't seem to have any
zero-parameter commands in the driver) - and so far it seems to have
worked fine.


> Also, is the direct_cmd_wrdat written at all when sending zero-parameter
> dcs command?
> 

At the moment, no.

Apparently there are 2 types of "Zero parameter" commands.

There is,

a) "MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM" - which has absolutely no
parameter that needs to be sent, and there is,

b) "MIPI_DSI_DCS_SHORT_WRITE" - which has a 1-byte command value that
needs to be transmitted.

(Macros referred from mipi_display.h)

In the J721E TRM[0], there is a table[1]  which classifies the
"MIPI_DSI_DCS_SHORT_WRITE" - the command with 1-byte command parameter -
as a "zero parameter" command.

For a "MIPI_DSI_DCS_SHORT_WRITE" command, we are still writing the
1-byte command data into the FIFO.

However, in the other section which talks about resetting the FIFO[2],
it is mentioned that, for "zero parameter" commands, the FIFO needs to
be reset and then 0x00 needs to be written to the FIFO.

The second step cannot be done for "MIPI_DSI_DCS_SHORT_WRITE" commands
because we want to write the 1 byte command parameter instead of 0x00
into the FIFO.

So, the only logical conclusion is that, the FIFO reset is only required
for _truly_ zero parameter commands, which is the
"MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM" command.

So, I am planning to change this patch to do 2 things, under the
condition that there are absolutely no data bytes that require
transmission.

a. Reset the FIFO.
b. Write 0x00 to the FIFO.


Regards
Aradhya

[0]: J721E TRM: https://www.ti.com/lit/zip/spruil1
[1]: Table: 12-1933: "DSI Main Settings Register Description".
[2]: Section 12.6.5.7.5.2: "Command Mode Settings"


> 
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 126e4bccd868..cad0c1478ef0 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -1018,6 +1018,9 @@ static ssize_t cdns_dsi_transfer(struct
>> mipi_dsi_host *host,
>>         cdns_dsi_init_link(dsi);
>>   +    /* Reset the DCS Write FIFO */
>> +    writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST);
>> >>       ret = mipi_dsi_create_packet(&packet, msg);
>>       if (ret)
>>           goto out;
> 
> 

[PATCH v4 08/11] drm/mipi-dsi: Add helper to find input format

[Aradhya Bhatia]

Add a helper API that can be used by the DSI hosts to find the required
input bus format for the given output dsi pixel format.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 37 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index a471c46f5ca6..937aa16dfcf6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -36,6 +36,8 @@
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_print.h>
 
+#include <linux/media-bus-format.h>
+
 #include <video/mipi_display.h>
 
 /**
@@ -866,6 +868,41 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 }
 EXPORT_SYMBOL(mipi_dsi_generic_read);
 
+/**
+ * drm_mipi_dsi_get_input_bus_fmt() - Get the required MEDIA_BUS_FMT_* based
+ *				      input pixel format for a given DSI output
+ *				      pixel format
+ * @dsi_format: pixel format that a DSI host needs to output
+ *
+ * Various DSI hosts can use this function during their
+ * &drm_bridge_funcs.atomic_get_input_bus_fmts operation to ascertain
+ * the MEDIA_BUS_FMT_* pixel format required as input.
+ *
+ * RETURNS:
+ * a 32-bit MEDIA_BUS_FMT_* value on success or 0 in case of failure.
+ */
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format)
+{
+	switch (dsi_format) {
+	case MIPI_DSI_FMT_RGB888:
+		return MEDIA_BUS_FMT_RGB888_1X24;
+
+	case MIPI_DSI_FMT_RGB666:
+		return MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+
+	case MIPI_DSI_FMT_RGB666_PACKED:
+		return MEDIA_BUS_FMT_RGB666_1X18;
+
+	case MIPI_DSI_FMT_RGB565:
+		return MEDIA_BUS_FMT_RGB565_1X16;
+
+	default:
+		/* Unsupported DSI Format */
+		return 0;
+	}
+}
+EXPORT_SYMBOL(drm_mipi_dsi_get_input_bus_fmt);
+
 /**
  * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
  * @dsi: DSI peripheral device
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 71d121aeef24..78a2c7d9eefb 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -290,6 +290,7 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
 				  const void *payload, size_t size);
 ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 			      size_t num_params, void *data, size_t size);
+u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format);
 
 #define mipi_dsi_msleep(ctx, delay)	\
 	do {				\
-- 
2.34.1

Re: [PATCH v4 08/11] drm/mipi-dsi: Add helper to find input format

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Add a helper API that can be used by the DSI hosts to find the required
> input bus format for the given output dsi pixel format.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/drm_mipi_dsi.c | 37 ++++++++++++++++++++++++++++++++++
>   include/drm/drm_mipi_dsi.h     |  1 +
>   2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index a471c46f5ca6..937aa16dfcf6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -36,6 +36,8 @@
>   #include <drm/drm_mipi_dsi.h>
>   #include <drm/drm_print.h>
>   
> +#include <linux/media-bus-format.h>
> +
>   #include <video/mipi_display.h>
>   
>   /**
> @@ -866,6 +868,41 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
>   }
>   EXPORT_SYMBOL(mipi_dsi_generic_read);
>   
> +/**
> + * drm_mipi_dsi_get_input_bus_fmt() - Get the required MEDIA_BUS_FMT_* based
> + *				      input pixel format for a given DSI output
> + *				      pixel format
> + * @dsi_format: pixel format that a DSI host needs to output
> + *
> + * Various DSI hosts can use this function during their
> + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation to ascertain
> + * the MEDIA_BUS_FMT_* pixel format required as input.
> + *
> + * RETURNS:
> + * a 32-bit MEDIA_BUS_FMT_* value on success or 0 in case of failure.
> + */
> +u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format)
> +{
> +	switch (dsi_format) {
> +	case MIPI_DSI_FMT_RGB888:
> +		return MEDIA_BUS_FMT_RGB888_1X24;
> +
> +	case MIPI_DSI_FMT_RGB666:
> +		return MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> +
> +	case MIPI_DSI_FMT_RGB666_PACKED:
> +		return MEDIA_BUS_FMT_RGB666_1X18;
> +
> +	case MIPI_DSI_FMT_RGB565:
> +		return MEDIA_BUS_FMT_RGB565_1X16;
> +
> +	default:
> +		/* Unsupported DSI Format */
> +		return 0;
> +	}
> +}
> +EXPORT_SYMBOL(drm_mipi_dsi_get_input_bus_fmt);
> +
>   /**
>    * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
>    * @dsi: DSI peripheral device
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 71d121aeef24..78a2c7d9eefb 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -290,6 +290,7 @@ void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
>   				  const void *payload, size_t size);
>   ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
>   			      size_t num_params, void *data, size_t size);
> +u32 drm_mipi_dsi_get_input_bus_fmt(enum mipi_dsi_pixel_format dsi_format);
>   
>   #define mipi_dsi_msleep(ctx, delay)	\
>   	do {				\

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

[PATCH v4 09/11] drm/bridge: cdns-dsi: Support atomic bridge APIs

[Aradhya Bhatia]

Change the existing (and deprecated) bridge hooks, to the bridge
atomic APIs.

Add drm helpers for duplicate_state, destroy_state, and bridge_reset
bridge hooks.

Further add support for the input format negotiation hook.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 51 ++++++++++++++++---
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index cad0c1478ef0..c9697818308e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,7 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -679,7 +680,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -755,7 +757,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -906,7 +909,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -918,13 +922,44 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
 	cdns_dsi_hs_init(dsi);
 }
 
+static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
+					       struct drm_bridge_state *bridge_state,
+					       struct drm_crtc_state *crtc_state,
+					       struct drm_connector_state *conn_state,
+					       u32 output_fmt,
+					       unsigned int *num_input_fmts)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input_to_dsi(input);
+	struct cdns_dsi_output *output = &dsi->output;
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	input_fmts[0] = drm_mipi_dsi_get_input_bus_fmt(output->dev->format);
+	if (!input_fmts[0])
+		return NULL;
+
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.disable = cdns_dsi_bridge_disable,
-	.pre_enable = cdns_dsi_bridge_pre_enable,
-	.enable = cdns_dsi_bridge_enable,
-	.post_disable = cdns_dsi_bridge_post_disable,
+	.atomic_disable = cdns_dsi_bridge_atomic_disable,
+	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
+	.atomic_enable = cdns_dsi_bridge_atomic_enable,
+	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
 };
 
 static int cdns_dsi_attach(struct mipi_dsi_host *host,
-- 
2.34.1

Re: [PATCH v4 09/11] drm/bridge: cdns-dsi: Support atomic bridge APIs

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Change the existing (and deprecated) bridge hooks, to the bridge
> atomic APIs.
> 
> Add drm helpers for duplicate_state, destroy_state, and bridge_reset
> bridge hooks.
> 
> Further add support for the input format negotiation hook.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> ---
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 51 ++++++++++++++++---
>   1 file changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index cad0c1478ef0..c9697818308e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -655,7 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>   	return MODE_OK;
>   }
>   
> -static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
> +					   struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -679,7 +680,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
>   	pm_runtime_put(dsi->base.dev);
>   }
>   
> -static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -755,7 +757,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
>   	dsi->link_initialized = true;
>   }
>   
> -static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -906,7 +909,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>   	writel(tmp, dsi->regs + MCTL_MAIN_EN);
>   }
>   
> -static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -918,13 +922,44 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
>   	cdns_dsi_hs_init(dsi);
>   }
>   
> +static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
> +					       struct drm_bridge_state *bridge_state,
> +					       struct drm_crtc_state *crtc_state,
> +					       struct drm_connector_state *conn_state,
> +					       u32 output_fmt,
> +					       unsigned int *num_input_fmts)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi *dsi = input_to_dsi(input);
> +	struct cdns_dsi_output *output = &dsi->output;
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	input_fmts[0] = drm_mipi_dsi_get_input_bus_fmt(output->dev->format);
> +	if (!input_fmts[0])
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +
> +	return input_fmts;
> +}
> +
>   static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.attach = cdns_dsi_bridge_attach,
>   	.mode_valid = cdns_dsi_bridge_mode_valid,
> -	.disable = cdns_dsi_bridge_disable,
> -	.pre_enable = cdns_dsi_bridge_pre_enable,
> -	.enable = cdns_dsi_bridge_enable,
> -	.post_disable = cdns_dsi_bridge_post_disable,
> +	.atomic_disable = cdns_dsi_bridge_atomic_disable,
> +	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
> +	.atomic_enable = cdns_dsi_bridge_atomic_enable,
> +	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
>   };
>   
>   static int cdns_dsi_attach(struct mipi_dsi_host *host,

[PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Aradhya Bhatia]

Move the bridge pre_enable call before crtc enable, and the bridge
post_disable call after the crtc disable.

The sequence of enable after this patch will look like:

	bridge[n]_pre_enable
	...
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	...
	bridge[n]__enable

and vice-versa for the bridge chain disable sequence.

The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".

Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled. Fix that by
re-ordering the sequence of bridge pre_enable and bridge post_disable.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
 include/drm/drm_atomic_helper.h     |   7 ++
 2 files changed, 114 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fb97b51b38f1..e8ad08634f58 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -74,6 +74,7 @@
  * also shares the &struct drm_plane_helper_funcs function table with the plane
  * helpers.
  */
+
 static void
 drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 				struct drm_plane_state *old_plane_state,
@@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
+			    enum bridge_chain_operation_type op_type)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
-	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
@@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (WARN_ON(!encoder))
 			continue;
 
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
-			       encoder->base.id, encoder->name);
-
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call disable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_disable(bridge, old_state);
 
-		/* Right function depends upon target state. */
-		if (funcs) {
-			if (funcs->atomic_disable)
-				funcs->atomic_disable(encoder, old_state);
-			else if (new_conn_state->crtc && funcs->prepare)
-				funcs->prepare(encoder);
-			else if (funcs->disable)
-				funcs->disable(encoder);
-			else if (funcs->dpms)
-				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
-		}
+		switch (op_type) {
+		case DRM_ENCODER_BRIDGE_DISABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
+
+			drm_atomic_bridge_chain_disable(bridge, old_state);
+
+			/* Right function depends upon target state. */
+			if (funcs) {
+				if (funcs->atomic_disable)
+					funcs->atomic_disable(encoder, old_state);
+				else if (new_conn_state->crtc && funcs->prepare)
+					funcs->prepare(encoder);
+				else if (funcs->disable)
+					funcs->disable(encoder);
+				else if (funcs->dpms)
+					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+			}
+
+			break;
+
+		case DRM_BRIDGE_POST_DISABLE:
+			drm_atomic_bridge_chain_post_disable(bridge, old_state);
 
-		drm_atomic_bridge_chain_post_disable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
+			break;
+		}
 	}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	int i;
+
+	disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
@@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (ret == 0)
 			drm_crtc_vblank_put(crtc);
 	}
+
+	disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
 }
 
 /**
@@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
+static void
+enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
+			   enum bridge_chain_operation_type op_type)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		const struct drm_encoder_helper_funcs *funcs;
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		if (!new_conn_state->best_encoder)
+			continue;
+
+		if (!new_conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
+			continue;
+
+		encoder = new_conn_state->best_encoder;
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call enable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+
+		switch (op_type) {
+		case DRM_BRIDGE_PRE_ENABLE:
+			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+			break;
+
+		case DRM_ENCODER_BRIDGE_ENABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
+
+			if (funcs) {
+				if (funcs->atomic_enable)
+					funcs->atomic_enable(encoder, old_state);
+				else if (funcs->enable)
+					funcs->enable(encoder);
+				else if (funcs->commit)
+					funcs->commit(encoder);
+			}
+
+			drm_atomic_bridge_chain_enable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
+			break;
+		}
+	}
+}
+
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
  * @dev: DRM device
@@ -1465,10 +1549,10 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
-	struct drm_connector *connector;
-	struct drm_connector_state *new_conn_state;
 	int i;
 
+	enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
+
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
@@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		}
 	}
 
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
-		const struct drm_encoder_helper_funcs *funcs;
-		struct drm_encoder *encoder;
-		struct drm_bridge *bridge;
-
-		if (!new_conn_state->best_encoder)
-			continue;
-
-		if (!new_conn_state->crtc->state->active ||
-		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
-			continue;
-
-		encoder = new_conn_state->best_encoder;
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
-			       encoder->base.id, encoder->name);
-
-		/*
-		 * Each encoder has at most one connector (since we always steal
-		 * it away), so we won't call enable hooks twice.
-		 */
-		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
-
-		if (funcs) {
-			if (funcs->atomic_enable)
-				funcs->atomic_enable(encoder, old_state);
-			else if (funcs->enable)
-				funcs->enable(encoder);
-			else if (funcs->commit)
-				funcs->commit(encoder);
-		}
-
-		drm_atomic_bridge_chain_enable(bridge, old_state);
-	}
+	enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9aa0a05aa072..b45a175a9f8a 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -43,6 +43,13 @@
  */
 #define DRM_PLANE_NO_SCALING (1<<16)
 
+enum bridge_chain_operation_type {
+	DRM_BRIDGE_PRE_ENABLE,
+	DRM_BRIDGE_POST_DISABLE,
+	DRM_ENCODER_BRIDGE_ENABLE,
+	DRM_ENCODER_BRIDGE_DISABLE,
+};
+
 struct drm_atomic_state;
 struct drm_private_obj;
 struct drm_private_state;
-- 
2.34.1

Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> Move the bridge pre_enable call before crtc enable, and the bridge
> post_disable call after the crtc disable.
> 
> The sequence of enable after this patch will look like:
> 
> 	bridge[n]_pre_enable
> 	...
> 	bridge[1]_pre_enable
> 
> 	crtc_enable
> 	encoder_enable
> 
> 	bridge[1]_enable
> 	...
> 	bridge[n]__enable
> 
> and vice-versa for the bridge chain disable sequence.
> 
> The definition of bridge pre_enable hook says that,
> "The display pipe (i.e. clocks and timing signals) feeding this bridge
> will not yet be running when this callback is called".
> 
> Since CRTC is also a source feeding the bridge, it should not be enabled
> before the bridges in the pipeline are pre_enabled. Fix that by
> re-ordering the sequence of bridge pre_enable and bridge post_disable.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
>   include/drm/drm_atomic_helper.h     |   7 ++
>   2 files changed, 114 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fb97b51b38f1..e8ad08634f58 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -74,6 +74,7 @@
>    * also shares the &struct drm_plane_helper_funcs function table with the plane
>    * helpers.
>    */
> +

Extra change.

>   static void
>   drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>   				struct drm_plane_state *old_plane_state,
> @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>   }
>   
>   static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> +			    enum bridge_chain_operation_type op_type)
>   {
>   	struct drm_connector *connector;
>   	struct drm_connector_state *old_conn_state, *new_conn_state;
> -	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>   	int i;
>   
> @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>   		if (WARN_ON(!encoder))
>   			continue;
>   
> -		funcs = encoder->helper_private;
> -
> -		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> -			       encoder->base.id, encoder->name);
> -
>   		/*
>   		 * Each encoder has at most one connector (since we always steal
>   		 * it away), so we won't call disable hooks twice.
>   		 */
>   		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		drm_atomic_bridge_chain_disable(bridge, old_state);
>   
> -		/* Right function depends upon target state. */
> -		if (funcs) {
> -			if (funcs->atomic_disable)
> -				funcs->atomic_disable(encoder, old_state);
> -			else if (new_conn_state->crtc && funcs->prepare)
> -				funcs->prepare(encoder);
> -			else if (funcs->disable)
> -				funcs->disable(encoder);
> -			else if (funcs->dpms)
> -				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> -		}
> +		switch (op_type) {
> +		case DRM_ENCODER_BRIDGE_DISABLE:
> +			funcs = encoder->helper_private;
> +
> +			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> +				       encoder->base.id, encoder->name);
> +
> +			drm_atomic_bridge_chain_disable(bridge, old_state);
> +
> +			/* Right function depends upon target state. */
> +			if (funcs) {
> +				if (funcs->atomic_disable)
> +					funcs->atomic_disable(encoder, old_state);
> +				else if (new_conn_state->crtc && funcs->prepare)
> +					funcs->prepare(encoder);
> +				else if (funcs->disable)
> +					funcs->disable(encoder);
> +				else if (funcs->dpms)
> +					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +			}
> +
> +			break;
> +
> +		case DRM_BRIDGE_POST_DISABLE:
> +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
>   
> -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
> +			break;
> +
> +		default:
> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> +			break;
> +		}
>   	}
> +}
> +
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	int i;
> +
> +	disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
>   
>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>   		const struct drm_crtc_helper_funcs *funcs;
> @@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>   		if (ret == 0)
>   			drm_crtc_vblank_put(crtc);
>   	}
> +
> +	disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
>   }
>   
>   /**
> @@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>   	}
>   }
>   
> +static void
> +enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> +			   enum bridge_chain_operation_type op_type)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> +		const struct drm_encoder_helper_funcs *funcs;
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		if (!new_conn_state->best_encoder)
> +			continue;
> +
> +		if (!new_conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = new_conn_state->best_encoder;
> +
> +		/*
> +		 * Each encoder has at most one connector (since we always steal
> +		 * it away), so we won't call enable hooks twice.
> +		 */
> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +
> +		switch (op_type) {
> +		case DRM_BRIDGE_PRE_ENABLE:
> +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> +			break;
> +
> +		case DRM_ENCODER_BRIDGE_ENABLE:
> +			funcs = encoder->helper_private;
> +
> +			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> +				       encoder->base.id, encoder->name);
> +
> +			if (funcs) {
> +				if (funcs->atomic_enable)
> +					funcs->atomic_enable(encoder, old_state);
> +				else if (funcs->enable)
> +					funcs->enable(encoder);
> +				else if (funcs->commit)
> +					funcs->commit(encoder);
> +			}
> +
> +			drm_atomic_bridge_chain_enable(bridge, old_state);
> +			break;
> +
> +		default:
> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> +			break;
> +		}
> +	}
> +}
> +
>   /**
>    * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
>    * @dev: DRM device
> @@ -1465,10 +1549,10 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state;
>   	struct drm_crtc_state *new_crtc_state;
> -	struct drm_connector *connector;
> -	struct drm_connector_state *new_conn_state;
>   	int i;
>   
> +	enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
> +
>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>   		const struct drm_crtc_helper_funcs *funcs;
>   
> @@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   		}
>   	}
>   
> -	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> -		const struct drm_encoder_helper_funcs *funcs;
> -		struct drm_encoder *encoder;
> -		struct drm_bridge *bridge;
> -
> -		if (!new_conn_state->best_encoder)
> -			continue;
> -
> -		if (!new_conn_state->crtc->state->active ||
> -		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> -			continue;
> -
> -		encoder = new_conn_state->best_encoder;
> -		funcs = encoder->helper_private;
> -
> -		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> -			       encoder->base.id, encoder->name);
> -
> -		/*
> -		 * Each encoder has at most one connector (since we always steal
> -		 * it away), so we won't call enable hooks twice.
> -		 */
> -		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> -
> -		if (funcs) {
> -			if (funcs->atomic_enable)
> -				funcs->atomic_enable(encoder, old_state);
> -			else if (funcs->enable)
> -				funcs->enable(encoder);
> -			else if (funcs->commit)
> -				funcs->commit(encoder);
> -		}
> -
> -		drm_atomic_bridge_chain_enable(bridge, old_state);
> -	}
> +	enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
>   
>   	drm_atomic_helper_commit_writebacks(dev, old_state);
>   }
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 9aa0a05aa072..b45a175a9f8a 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -43,6 +43,13 @@
>    */
>   #define DRM_PLANE_NO_SCALING (1<<16)
>   
> +enum bridge_chain_operation_type {
> +	DRM_BRIDGE_PRE_ENABLE,
> +	DRM_BRIDGE_POST_DISABLE,
> +	DRM_ENCODER_BRIDGE_ENABLE,
> +	DRM_ENCODER_BRIDGE_DISABLE,
> +};

Why are the last two "DRM_ENCODER"?

I don't like the enum... Having "enum bridge_chain_operation_type" as a 
parameter to a function looks like one can pass any of the enum's 
values, which is not the case.

How about an enum with just two values:

DRM_BRIDGE_PRE_ENABLE_POST_DISABLE
DRM_BRIDGE_ENABLE_DISABLE

  Tomi

Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10495 bytes --]

On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
> > Move the bridge pre_enable call before crtc enable, and the bridge
> > post_disable call after the crtc disable.
> > 
> > The sequence of enable after this patch will look like:
> > 
> > 	bridge[n]_pre_enable
> > 	...
> > 	bridge[1]_pre_enable
> > 
> > 	crtc_enable
> > 	encoder_enable
> > 
> > 	bridge[1]_enable
> > 	...
> > 	bridge[n]__enable
> > 
> > and vice-versa for the bridge chain disable sequence.
> > 
> > The definition of bridge pre_enable hook says that,
> > "The display pipe (i.e. clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called".
> > 
> > Since CRTC is also a source feeding the bridge, it should not be enabled
> > before the bridges in the pipeline are pre_enabled. Fix that by
> > re-ordering the sequence of bridge pre_enable and bridge post_disable.
> > 
> > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> > ---
> >   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
> >   include/drm/drm_atomic_helper.h     |   7 ++
> >   2 files changed, 114 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index fb97b51b38f1..e8ad08634f58 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -74,6 +74,7 @@
> >    * also shares the &struct drm_plane_helper_funcs function table with the plane
> >    * helpers.
> >    */
> > +
> 
> Extra change.
> 
> >   static void
> >   drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> >   				struct drm_plane_state *old_plane_state,
> > @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
> >   }
> >   static void
> > -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > +disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> > +			    enum bridge_chain_operation_type op_type)
> >   {
> >   	struct drm_connector *connector;
> >   	struct drm_connector_state *old_conn_state, *new_conn_state;
> > -	struct drm_crtc *crtc;
> >   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> >   	int i;
> > @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >   		if (WARN_ON(!encoder))
> >   			continue;
> > -		funcs = encoder->helper_private;
> > -
> > -		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> > -			       encoder->base.id, encoder->name);
> > -
> >   		/*
> >   		 * Each encoder has at most one connector (since we always steal
> >   		 * it away), so we won't call disable hooks twice.
> >   		 */
> >   		bridge = drm_bridge_chain_get_first_bridge(encoder);
> > -		drm_atomic_bridge_chain_disable(bridge, old_state);
> > -		/* Right function depends upon target state. */
> > -		if (funcs) {
> > -			if (funcs->atomic_disable)
> > -				funcs->atomic_disable(encoder, old_state);
> > -			else if (new_conn_state->crtc && funcs->prepare)
> > -				funcs->prepare(encoder);
> > -			else if (funcs->disable)
> > -				funcs->disable(encoder);
> > -			else if (funcs->dpms)
> > -				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > -		}
> > +		switch (op_type) {
> > +		case DRM_ENCODER_BRIDGE_DISABLE:
> > +			funcs = encoder->helper_private;
> > +
> > +			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> > +				       encoder->base.id, encoder->name);
> > +
> > +			drm_atomic_bridge_chain_disable(bridge, old_state);
> > +
> > +			/* Right function depends upon target state. */
> > +			if (funcs) {
> > +				if (funcs->atomic_disable)
> > +					funcs->atomic_disable(encoder, old_state);
> > +				else if (new_conn_state->crtc && funcs->prepare)
> > +					funcs->prepare(encoder);
> > +				else if (funcs->disable)
> > +					funcs->disable(encoder);
> > +				else if (funcs->dpms)
> > +					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > +			}
> > +
> > +			break;
> > +
> > +		case DRM_BRIDGE_POST_DISABLE:
> > +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > +			break;
> > +
> > +		default:
> > +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> > +			break;
> > +		}
> >   	}
> > +}
> > +
> > +static void
> > +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > +	int i;
> > +
> > +	disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
> >   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> >   		const struct drm_crtc_helper_funcs *funcs;
> > @@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >   		if (ret == 0)
> >   			drm_crtc_vblank_put(crtc);
> >   	}
> > +
> > +	disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
> >   }
> >   /**
> > @@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> >   	}
> >   }
> > +static void
> > +enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> > +			   enum bridge_chain_operation_type op_type)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_connector_state *new_conn_state;
> > +	int i;
> > +
> > +	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> > +		const struct drm_encoder_helper_funcs *funcs;
> > +		struct drm_encoder *encoder;
> > +		struct drm_bridge *bridge;
> > +
> > +		if (!new_conn_state->best_encoder)
> > +			continue;
> > +
> > +		if (!new_conn_state->crtc->state->active ||
> > +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> > +			continue;
> > +
> > +		encoder = new_conn_state->best_encoder;
> > +
> > +		/*
> > +		 * Each encoder has at most one connector (since we always steal
> > +		 * it away), so we won't call enable hooks twice.
> > +		 */
> > +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> > +
> > +		switch (op_type) {
> > +		case DRM_BRIDGE_PRE_ENABLE:
> > +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> > +			break;
> > +
> > +		case DRM_ENCODER_BRIDGE_ENABLE:
> > +			funcs = encoder->helper_private;
> > +
> > +			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> > +				       encoder->base.id, encoder->name);
> > +
> > +			if (funcs) {
> > +				if (funcs->atomic_enable)
> > +					funcs->atomic_enable(encoder, old_state);
> > +				else if (funcs->enable)
> > +					funcs->enable(encoder);
> > +				else if (funcs->commit)
> > +					funcs->commit(encoder);
> > +			}
> > +
> > +			drm_atomic_bridge_chain_enable(bridge, old_state);
> > +			break;
> > +
> > +		default:
> > +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> >   /**
> >    * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> >    * @dev: DRM device
> > @@ -1465,10 +1549,10 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >   	struct drm_crtc *crtc;
> >   	struct drm_crtc_state *old_crtc_state;
> >   	struct drm_crtc_state *new_crtc_state;
> > -	struct drm_connector *connector;
> > -	struct drm_connector_state *new_conn_state;
> >   	int i;
> > +	enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
> > +
> >   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> >   		const struct drm_crtc_helper_funcs *funcs;
> > @@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> >   		}
> >   	}
> > -	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> > -		const struct drm_encoder_helper_funcs *funcs;
> > -		struct drm_encoder *encoder;
> > -		struct drm_bridge *bridge;
> > -
> > -		if (!new_conn_state->best_encoder)
> > -			continue;
> > -
> > -		if (!new_conn_state->crtc->state->active ||
> > -		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> > -			continue;
> > -
> > -		encoder = new_conn_state->best_encoder;
> > -		funcs = encoder->helper_private;
> > -
> > -		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> > -			       encoder->base.id, encoder->name);
> > -
> > -		/*
> > -		 * Each encoder has at most one connector (since we always steal
> > -		 * it away), so we won't call enable hooks twice.
> > -		 */
> > -		bridge = drm_bridge_chain_get_first_bridge(encoder);
> > -		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> > -
> > -		if (funcs) {
> > -			if (funcs->atomic_enable)
> > -				funcs->atomic_enable(encoder, old_state);
> > -			else if (funcs->enable)
> > -				funcs->enable(encoder);
> > -			else if (funcs->commit)
> > -				funcs->commit(encoder);
> > -		}
> > -
> > -		drm_atomic_bridge_chain_enable(bridge, old_state);
> > -	}
> > +	enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
> >   	drm_atomic_helper_commit_writebacks(dev, old_state);
> >   }
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 9aa0a05aa072..b45a175a9f8a 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -43,6 +43,13 @@
> >    */
> >   #define DRM_PLANE_NO_SCALING (1<<16)
> > +enum bridge_chain_operation_type {
> > +	DRM_BRIDGE_PRE_ENABLE,
> > +	DRM_BRIDGE_POST_DISABLE,
> > +	DRM_ENCODER_BRIDGE_ENABLE,
> > +	DRM_ENCODER_BRIDGE_DISABLE,
> > +};
> 
> Why are the last two "DRM_ENCODER"?
> 
> I don't like the enum... Having "enum bridge_chain_operation_type" as a
> parameter to a function looks like one can pass any of the enum's values,
> which is not the case.
> 
> How about an enum with just two values:
> 
> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE
> DRM_BRIDGE_ENABLE_DISABLE

I think I'd go a step further and ask why do we need that rework in the
first place. We had a discussion about changing the time the CRTC is
enabled compared to bridges, but it's not clear, nor explained, why we
need to do that rework in the first place.

It should even be two patches imo.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Aradhya Bhatia]

On 26/06/24 18:37, Maxime Ripard wrote:
> On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
>> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>>> Move the bridge pre_enable call before crtc enable, and the bridge
>>> post_disable call after the crtc disable.
>>>
>>> The sequence of enable after this patch will look like:
>>>
>>> 	bridge[n]_pre_enable
>>> 	...
>>> 	bridge[1]_pre_enable
>>>
>>> 	crtc_enable
>>> 	encoder_enable
>>>
>>> 	bridge[1]_enable
>>> 	...
>>> 	bridge[n]__enable
>>>
>>> and vice-versa for the bridge chain disable sequence.
>>>
>>> The definition of bridge pre_enable hook says that,
>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>> will not yet be running when this callback is called".
>>>
>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>> before the bridges in the pipeline are pre_enabled. Fix that by
>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
>>>   include/drm/drm_atomic_helper.h     |   7 ++
>>>   2 files changed, 114 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index fb97b51b38f1..e8ad08634f58 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -74,6 +74,7 @@
>>>    * also shares the &struct drm_plane_helper_funcs function table with the plane
>>>    * helpers.
>>>    */
>>> +
>>
>> Extra change.
>>
>>>   static void
>>>   drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>>>   				struct drm_plane_state *old_plane_state,
>>> @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>>>   }
>>>   static void
>>> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>> +disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
>>> +			    enum bridge_chain_operation_type op_type)
>>>   {
>>>   	struct drm_connector *connector;
>>>   	struct drm_connector_state *old_conn_state, *new_conn_state;
>>> -	struct drm_crtc *crtc;
>>>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>>   	int i;
>>> @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>>   		if (WARN_ON(!encoder))
>>>   			continue;
>>> -		funcs = encoder->helper_private;
>>> -
>>> -		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
>>> -			       encoder->base.id, encoder->name);
>>> -
>>>   		/*
>>>   		 * Each encoder has at most one connector (since we always steal
>>>   		 * it away), so we won't call disable hooks twice.
>>>   		 */
>>>   		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> -		drm_atomic_bridge_chain_disable(bridge, old_state);
>>> -		/* Right function depends upon target state. */
>>> -		if (funcs) {
>>> -			if (funcs->atomic_disable)
>>> -				funcs->atomic_disable(encoder, old_state);
>>> -			else if (new_conn_state->crtc && funcs->prepare)
>>> -				funcs->prepare(encoder);
>>> -			else if (funcs->disable)
>>> -				funcs->disable(encoder);
>>> -			else if (funcs->dpms)
>>> -				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>> -		}
>>> +		switch (op_type) {
>>> +		case DRM_ENCODER_BRIDGE_DISABLE:
>>> +			funcs = encoder->helper_private;
>>> +
>>> +			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
>>> +				       encoder->base.id, encoder->name);
>>> +
>>> +			drm_atomic_bridge_chain_disable(bridge, old_state);
>>> +
>>> +			/* Right function depends upon target state. */
>>> +			if (funcs) {
>>> +				if (funcs->atomic_disable)
>>> +					funcs->atomic_disable(encoder, old_state);
>>> +				else if (new_conn_state->crtc && funcs->prepare)
>>> +					funcs->prepare(encoder);
>>> +				else if (funcs->disable)
>>> +					funcs->disable(encoder);
>>> +				else if (funcs->dpms)
>>> +					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>> +			}
>>> +
>>> +			break;
>>> +
>>> +		case DRM_BRIDGE_POST_DISABLE:
>>> +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
>>> -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
>>> +			break;
>>> +
>>> +		default:
>>> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
>>> +			break;
>>> +		}
>>>   	}
>>> +}
>>> +
>>> +static void
>>> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>> +{
>>> +	struct drm_crtc *crtc;
>>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>> +	int i;
>>> +
>>> +	disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
>>>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>>>   		const struct drm_crtc_helper_funcs *funcs;
>>> @@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>>   		if (ret == 0)
>>>   			drm_crtc_vblank_put(crtc);
>>>   	}
>>> +
>>> +	disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
>>>   }
>>>   /**
>>> @@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>>>   	}
>>>   }
>>> +static void
>>> +enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
>>> +			   enum bridge_chain_operation_type op_type)
>>> +{
>>> +	struct drm_connector *connector;
>>> +	struct drm_connector_state *new_conn_state;
>>> +	int i;
>>> +
>>> +	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
>>> +		const struct drm_encoder_helper_funcs *funcs;
>>> +		struct drm_encoder *encoder;
>>> +		struct drm_bridge *bridge;
>>> +
>>> +		if (!new_conn_state->best_encoder)
>>> +			continue;
>>> +
>>> +		if (!new_conn_state->crtc->state->active ||
>>> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
>>> +			continue;
>>> +
>>> +		encoder = new_conn_state->best_encoder;
>>> +
>>> +		/*
>>> +		 * Each encoder has at most one connector (since we always steal
>>> +		 * it away), so we won't call enable hooks twice.
>>> +		 */
>>> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> +
>>> +		switch (op_type) {
>>> +		case DRM_BRIDGE_PRE_ENABLE:
>>> +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
>>> +			break;
>>> +
>>> +		case DRM_ENCODER_BRIDGE_ENABLE:
>>> +			funcs = encoder->helper_private;
>>> +
>>> +			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
>>> +				       encoder->base.id, encoder->name);
>>> +
>>> +			if (funcs) {
>>> +				if (funcs->atomic_enable)
>>> +					funcs->atomic_enable(encoder, old_state);
>>> +				else if (funcs->enable)
>>> +					funcs->enable(encoder);
>>> +				else if (funcs->commit)
>>> +					funcs->commit(encoder);
>>> +			}
>>> +
>>> +			drm_atomic_bridge_chain_enable(bridge, old_state);
>>> +			break;
>>> +
>>> +		default:
>>> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
>>> +			break;
>>> +		}
>>> +	}
>>> +}
>>> +
>>>   /**
>>>    * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
>>>    * @dev: DRM device
>>> @@ -1465,10 +1549,10 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>>   	struct drm_crtc *crtc;
>>>   	struct drm_crtc_state *old_crtc_state;
>>>   	struct drm_crtc_state *new_crtc_state;
>>> -	struct drm_connector *connector;
>>> -	struct drm_connector_state *new_conn_state;
>>>   	int i;
>>> +	enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
>>> +
>>>   	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>>>   		const struct drm_crtc_helper_funcs *funcs;
>>> @@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>>   		}
>>>   	}
>>> -	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
>>> -		const struct drm_encoder_helper_funcs *funcs;
>>> -		struct drm_encoder *encoder;
>>> -		struct drm_bridge *bridge;
>>> -
>>> -		if (!new_conn_state->best_encoder)
>>> -			continue;
>>> -
>>> -		if (!new_conn_state->crtc->state->active ||
>>> -		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
>>> -			continue;
>>> -
>>> -		encoder = new_conn_state->best_encoder;
>>> -		funcs = encoder->helper_private;
>>> -
>>> -		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
>>> -			       encoder->base.id, encoder->name);
>>> -
>>> -		/*
>>> -		 * Each encoder has at most one connector (since we always steal
>>> -		 * it away), so we won't call enable hooks twice.
>>> -		 */
>>> -		bridge = drm_bridge_chain_get_first_bridge(encoder);
>>> -		drm_atomic_bridge_chain_pre_enable(bridge, old_state);
>>> -
>>> -		if (funcs) {
>>> -			if (funcs->atomic_enable)
>>> -				funcs->atomic_enable(encoder, old_state);
>>> -			else if (funcs->enable)
>>> -				funcs->enable(encoder);
>>> -			else if (funcs->commit)
>>> -				funcs->commit(encoder);
>>> -		}
>>> -
>>> -		drm_atomic_bridge_chain_enable(bridge, old_state);
>>> -	}
>>> +	enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
>>>   	drm_atomic_helper_commit_writebacks(dev, old_state);
>>>   }
>>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>>> index 9aa0a05aa072..b45a175a9f8a 100644
>>> --- a/include/drm/drm_atomic_helper.h
>>> +++ b/include/drm/drm_atomic_helper.h
>>> @@ -43,6 +43,13 @@
>>>    */
>>>   #define DRM_PLANE_NO_SCALING (1<<16)
>>> +enum bridge_chain_operation_type {
>>> +	DRM_BRIDGE_PRE_ENABLE,
>>> +	DRM_BRIDGE_POST_DISABLE,
>>> +	DRM_ENCODER_BRIDGE_ENABLE,
>>> +	DRM_ENCODER_BRIDGE_DISABLE,
>>> +};
>>
>> Why are the last two "DRM_ENCODER"?

I do agree that the naming is odd. It's supposed to mean both, encoder
and bridge.

When we are enabling/disabling bridges, the encoders are also getting
enabled/disabled right before/after.

But in case of pre_enable/post_disable its only the bridges that are
being operated on.

>>
>> I don't like the enum... Having "enum bridge_chain_operation_type" as a
>> parameter to a function looks like one can pass any of the enum's values,
>> which is not the case.
>>
>> How about an enum with just two values:
>>
>> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE
>> DRM_BRIDGE_ENABLE_DISABLE

Yes, I can combine them like this.

> 
> I think I'd go a step further and ask why do we need that rework in the
> first place. We had a discussion about changing the time the CRTC is
> enabled compared to bridges, but it's not clear, nor explained, why we
> need to do that rework in the first place.
> 

We did discuss this, which resulted in a bunch of duplicated code in
v2[0]. The same block of code was required before the CRTC enable, as
well as after. Hence this patch.

Maybe all of this should have been explained better. =)

> It should even be two patches imo.
> 

Yeah, I can split this in 2 patches.


Regards
Aradhya

[0]:
https://lore.kernel.org/all/20240530093621.1925863-9-a-bhatia1@ti.com/

Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Devarsh Thakkar]

Hi Aradhya, Tomi,

Thanks for the patch Aradhya.

On 11/07/24 13:02, Aradhya Bhatia wrote:
> On 26/06/24 18:37, Maxime Ripard wrote:
>> On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
>>> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>>>> Move the bridge pre_enable call before crtc enable, and the bridge
>>>> post_disable call after the crtc disable.
>>>>
>>>> The sequence of enable after this patch will look like:
>>>>
>>>> 	bridge[n]_pre_enable
>>>> 	...
>>>> 	bridge[1]_pre_enable
>>>>
>>>> 	crtc_enable
>>>> 	encoder_enable
>>>>
>>>> 	bridge[1]_enable
>>>> 	...
>>>> 	bridge[n]__enable
>>>>

>>> and vice-versa for the bridge chain disable sequence.

Could you please add the disable sequence too (including the
post_disable hook in the sequence)?

>>>>
>>>> The definition of bridge pre_enable hook says that,
>>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>> will not yet be running when this callback is called".
>>>>
>>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>>> before the bridges in the pipeline are pre_enabled. Fix that by
>>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
>>>>   include/drm/drm_atomic_helper.h     |   7 ++
>>>>   2 files changed, 114 insertions(+), 58 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index fb97b51b38f1..e8ad08634f58 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c

<snip>
>>> I don't like the enum... Having "enum bridge_chain_operation_type" as a
>>> parameter to a function looks like one can pass any of the enum's values,
>>> which is not the case.
>>>
>>> How about an enum with just two values:
>>>
>>> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE

This sounds like it is being used to do both pre-enable and post-disable
at once, but in reality it is either doing pre-enable (when called
during enable sequence) or post-disable (when called during disable
sequence).

Hence, I would suggest to make it DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE

>>> DRM_BRIDGE_ENABLE_DISABLE

Similarly for this, it should be DRM_BRIDGE_ENABLE_OR_DISABLE

Regards
Devarsh

[PATCH v4 11/11] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

[Aradhya Bhatia]

The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).

Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 32 +++----------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index c9697818308e..c352ea7db4ed 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -655,8 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -680,15 +680,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
-						struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	pm_runtime_put(dsi->base.dev);
-}
-
 static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -757,8 +748,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -909,19 +900,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-					      struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,
@@ -952,9 +930,7 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
-- 
2.34.1

Re: [PATCH v4 11/11] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

[Tomi Valkeinen]

On 22/06/2024 14:09, Aradhya Bhatia wrote:
> The cdns-dsi controller requires that it be turned on completely before
> the input DPI's source has begun streaming[0]. Not having that, allows
> for a small window before cdns-dsi enable and after cdns-dsi disable
> where the previous entity (in this case tidss's videoport) to continue
> streaming DPI video signals. This small window where cdns-dsi is
> disabled but is still receiving signals causes the input FIFO of
> cdns-dsi to get corrupted. This causes the colors to shift on the output
> display. The colors can either shift by one color component (R->G, G->B,
> B->R), or by two color components (R->B, G->R, B->G).
> 
> Since tidss's videoport starts streaming via crtc enable hooks, we need
> cdns-dsi to be up and running before that. Now that the bridges are
> pre_enabled before crtc is enabled, and post_disabled after crtc is
> disabled, use the pre_enable and post_disable hooks to get cdns-dsi
> ready and running before the tidss videoport to get pass the color shift
> issues.
> 
> [0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
>       TRM Link: http://www.ti.com/lit/pdf/spruil1
> 

I think it makes sense to explain a bit about this in a comment in the 
driver code. Otherwise doing all of this in pre_enable and post_disable 
looks a bit odd.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 32 +++----------------
>   1 file changed, 4 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index c9697818308e..c352ea7db4ed 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -655,8 +655,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>   	return MODE_OK;
>   }
>   
> -static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
> -					   struct drm_bridge_state *old_bridge_state)
> +static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -680,15 +680,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
>   	pm_runtime_put(dsi->base.dev);
>   }
>   
> -static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> -						struct drm_bridge_state *old_bridge_state)
> -{
> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> -	struct cdns_dsi *dsi = input_to_dsi(input);
> -
> -	pm_runtime_put(dsi->base.dev);
> -}
> -
>   static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
>   {
>   	struct cdns_dsi_output *output = &dsi->output;
> @@ -757,8 +748,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
>   	dsi->link_initialized = true;
>   }
>   
> -static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
> -					  struct drm_bridge_state *old_bridge_state)
> +static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *old_bridge_state)
>   {
>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>   	struct cdns_dsi *dsi = input_to_dsi(input);
> @@ -909,19 +900,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>   	writel(tmp, dsi->regs + MCTL_MAIN_EN);
>   }
>   
> -static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> -					      struct drm_bridge_state *old_bridge_state)
> -{
> -	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> -	struct cdns_dsi *dsi = input_to_dsi(input);
> -
> -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> -		return;
> -
> -	cdns_dsi_init_link(dsi);
> -	cdns_dsi_hs_init(dsi);
> -}
> -
>   static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>   					       struct drm_bridge_state *bridge_state,
>   					       struct drm_crtc_state *crtc_state,
> @@ -952,9 +930,7 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>   static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.attach = cdns_dsi_bridge_attach,
>   	.mode_valid = cdns_dsi_bridge_mode_valid,
> -	.atomic_disable = cdns_dsi_bridge_atomic_disable,
>   	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
> -	.atomic_enable = cdns_dsi_bridge_atomic_enable,
>   	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
>   	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>   	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,