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

[PATCH v7 00/12] 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 Sitara AM62P and AM62L SoCs.

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-v7_1_tests" branch of my github fork[1] for anyone who would like
to test them.

Thanks,
Aradhya


* Important note about the authorship of patches *

All the patches in the previous revisions of this series, as well as a majority
of this revision too have been authored when I owned a "ti.com" based email id,
i.e. <a-bhatia1@ti.com>. This email id is not in use anymore, and all the work
done later has been part of my personal work. Since the original patches were
authored using TI's email id, I have maintained the original authorships as they
are, as well as their sign offs.

I have further added another another sign off that uses my current (and
personal) email id, the one that is being used to send this revision,
i.e. <aradhya.bhatia@linux.dev>.


[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-v7_1_tests


Change Log:
  - Changes in v7:
    - phy_init()/exit() were called from the PM path in v6. Change it back to
      the bridge enable/disable path in patch-3, so that the phy_init() can go
      back to being called after D-Phy reset assert.
    - Reword commit text in patch-5 to explain the need of the fix.
    - Drop the stray code in patch-10.
    - Add R-b tag of Dmitry Baryshkov in patch-6.

  - Changes in v6:
    - Reword patch 3 to better explain the fixes around phy de-init.
    - Fix the Lane ready timeout condition in patch 7.
    - Fix the dsi _bridge_atomic_check() implementation by adding a new
      bridge state structure in patch 10.
    - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
    - Generate the patches of these series using the "patience" algorithm.
      Note: All patches, except v6:11/12, *do not* differ from their default
      (greedy) algorithm variants.
      For patch 11, the patience algorithm significantly improves the readability.
    - Rename and move the Bridge enable/disable enums from public to private
      in patch 11.
    - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov in patch 2.

  - Changes in v5:
    - Fix subject and description in patch 1/13.
    - Add patch to check the return value of
      phy_mipi_dphy_get_default_config() (patch: 6/13).
    - Change the Clk and Data Lane ready timeout from forever to 5s.
    - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
    - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write FIFO".
      There has been some inconsistencies found with this patch upon further
      testing. This patch was being used to enable a DSI panel based on ILITEK
      ILI9881C bridge. This will be debugged separately.
    - Add patch to move the DSI mode check from _atomic_enable() to
      _atomic_check() (patch: 10/13).
    - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
      Patch 11/13 separates out the Encoder-Bridge operations into a helper
      function *without* changing the logic. Patch 12/13 then changes the order
      of the encoder-bridge operations as was intended in the original patch.
    - Add detailed comment for patch 13/13.
    - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.

  - 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/
v4: https://lore.kernel.org/all/20240622110929.3115714-1-a-bhatia1@ti.com/
v5: https://lore.kernel.org/all/20241019195411.266860-1-aradhya.bhatia@linux.dev/
v6: https://lore.kernel.org/all/20250111192738.308889-1-aradhya.bhatia@linux.dev/


Aradhya Bhatia (12):
  drm/bridge: cdns-dsi: Fix connecting to next bridge
  drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()
  drm/bridge: cdns-dsi: Fix phy de-init and flag it so
  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: Check return value when getting default PHY
    config
  drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready
  drm/mipi-dsi: Add helper to find input format
  drm/bridge: cdns-dsi: Support atomic bridge APIs
  drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check()
  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    | 218 ++++++++++---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.h    |   2 -
 drivers/gpu/drm/drm_atomic_helper.c           | 300 +++++++++++-------
 drivers/gpu/drm/drm_mipi_dsi.c                |  37 +++
 include/drm/drm_mipi_dsi.h                    |   1 +
 5 files changed, 386 insertions(+), 172 deletions(-)


base-commit: 37136bf5c3a6f6b686d74f41837a6406bec6b7bc
-- 
2.34.1

[PATCH v7 01/12] drm/bridge: cdns-dsi: Fix connecting to next bridge

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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

The code to find the next panel (and create its panel-bridge) works
fine, but to find the next (non-panel) bridge does not.

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 (which is what the code does currently)
will 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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
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 c7a0247e06ad..2f897ea5e80a 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

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

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../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 2f897ea5e80a..056583e81155 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

[PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

The driver code doesn't have a Phy de-initialization path as yet, and so
it does not clear the phy_initialized flag while suspending. This is a
problem because after resume the driver looks at this flag to determine
if a Phy re-initialization is required or not. It is in fact required
because the hardware is resuming from a suspend, but the driver does not
carry out any re-initialization causing the D-Phy to not work at all.

Call the counterparts of phy_init() and phy_power_on(), that are
phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
flags so that the Phy can be initialized again when required.

Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
 1 file changed, 5 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 056583e81155..039c5eb7fb66 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 	if (dsi->platform_ops && dsi->platform_ops->disable)
 		dsi->platform_ops->disable(dsi);
 
+	dsi->phy_initialized = false;
+	dsi->link_initialized = false;
+	phy_power_off(dsi->dphy);
+	phy_exit(dsi->dphy);
+
 	pm_runtime_put(dsi->base.dev);
 }
 
@@ -1130,7 +1135,6 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
 	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

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

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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")
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 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 039c5eb7fb66..89e8d6f1c4dd 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -780,8 +780,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

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

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

The crtc_* mode parameters do not get generated (duplicated in this
case) from the regular parameters before the mode validation phase
begins.

The rest of the code conditionally uses the crtc_* parameters only
during the bridge enable phase, but sticks to the regular parameters
for mode validation. In this singular instance, however, the driver
tries to use the crtc_clock parameter even during the mode validation,
causing the validation to fail.

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")
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 ++-
 1 file changed, 2 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 89e8d6f1c4dd..ccd964ba8c23 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -568,13 +568,14 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long dsi_hss_hsa_hse_hbp;
 	unsigned int nlanes = output->dev->lanes;
+	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
 	int ret;
 
 	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
 	if (ret)
 		return ret;
 
-	phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+	phy_mipi_dphy_get_default_config(mode_clock * 1000,
 					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					 nlanes, phy_cfg);
 
-- 
2.34.1

[PATCH v7 06/12] drm/bridge: cdns-dsi: Check return value when getting default PHY config

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

Check for the return value of the phy_mipi_dphy_get_default_config()
call, and incase of an error, return back the same.

Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index ccd964ba8c23..b278e424b4b5 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -575,9 +575,11 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	phy_mipi_dphy_get_default_config(mode_clock * 1000,
-					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
-					 nlanes, phy_cfg);
+	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
+					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
+					       nlanes, phy_cfg);
+	if (ret)
+		return ret;
 
 	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
 	if (ret)
-- 
2.34.1

[PATCH v7 07/12] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 15 ++++++++++++++-
 1 file changed, 14 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 b278e424b4b5..713003e6c210 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -769,7 +769,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))
@@ -786,6 +786,19 @@ 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);
+
+	if (readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+			       (tmp == (status & tmp)), 100, 500000))
+		dev_err(dsi->base.dev,
+			"Timed Out: DSI-DPhy Clock and Data Lanes not ready.\n");
+
 	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

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

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 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 5e5c5f84daac..076826f2445a 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>
 
 /**
@@ -870,6 +872,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 94400a78031f..9e2804e3a2b0 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -293,6 +293,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

[PATCH v7 09/12] drm/bridge: cdns-dsi: Support atomic bridge APIs

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../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 713003e6c210..b8db984ea9fa 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -658,7 +658,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);
@@ -683,7 +684,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);
@@ -760,7 +762,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);
@@ -913,7 +916,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);
@@ -925,13 +929,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

[PATCH v7 10/12] drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check()

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

At present, the DSI mode configuration check happens during the
_atomic_enable() phase, which is not really the best place for this.
Moreover, if the mode is not valid, the driver gives a warning and
continues the hardware configuration.

Move the DSI mode configuration check to _atomic_check() instead, which
can properly report back any invalid mode, before the _enable phase even
begins.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b8db984ea9fa..d60254e1270c 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -425,6 +425,17 @@
 #define DSI_NULL_FRAME_OVERHEAD		6
 #define DSI_EOT_PKT_SIZE		4
 
+struct cdns_dsi_bridge_state {
+	struct drm_bridge_state base;
+	struct cdns_dsi_cfg dsi_cfg;
+};
+
+static inline struct cdns_dsi_bridge_state *
+to_cdns_dsi_bridge_state(struct drm_bridge_state *bridge_state)
+{
+	return container_of(bridge_state, struct cdns_dsi_bridge_state, base);
+}
+
 static inline struct cdns_dsi *input_to_dsi(struct cdns_dsi_input *input)
 {
 	return container_of(input, struct cdns_dsi, input);
@@ -768,6 +779,9 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	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;
+	struct drm_atomic_state *state = old_bridge_state->base.state;
+	struct cdns_dsi_bridge_state *dsi_state;
+	struct drm_bridge_state *new_bridge_state;
 	struct drm_display_mode *mode;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
@@ -778,14 +792,19 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
+	new_bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	if (WARN_ON(!new_bridge_state))
+		return;
+
+	dsi_state = to_cdns_dsi_bridge_state(new_bridge_state);
+	dsi_cfg = dsi_state->dsi_cfg;
+
 	if (dsi->platform_ops && dsi->platform_ops->enable)
 		dsi->platform_ops->enable(dsi);
 
 	mode = &bridge->encoder->crtc->state->adjusted_mode;
 	nlanes = output->dev->lanes;
 
-	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
-
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 
@@ -956,6 +975,63 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
+					struct drm_bridge_state *bridge_state,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input_to_dsi(input);
+	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
+	struct drm_display_mode *mode = &crtc_state->mode;
+	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
+
+	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
+}
+
+static struct drm_bridge_state *
+cdns_dsi_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	if (WARN_ON(!bridge->base.state))
+		return NULL;
+
+	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
+	if (!dsi_state)
+		return NULL;
+
+	__drm_atomic_helper_bridge_duplicate_state(bridge, &dsi_state->base);
+
+	return &dsi_state->base;
+}
+
+static void
+cdns_dsi_bridge_atomic_destroy_state(struct drm_bridge *bridge,
+				     struct drm_bridge_state *state)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	dsi_state = to_cdns_dsi_bridge_state(state);
+
+	kfree(dsi_state);
+}
+
+static struct drm_bridge_state *
+cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge)
+{
+	struct cdns_dsi_bridge_state *dsi_state;
+
+	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
+	if (!dsi_state)
+		return NULL;
+
+	memset(dsi_state, 0, sizeof(*dsi_state));
+	dsi_state->base.bridge = bridge;
+
+	return &dsi_state->base;
+}
+
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
@@ -963,9 +1039,10 @@ static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.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_check = cdns_dsi_bridge_atomic_check,
+	.atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state,
+	.atomic_destroy_state = cdns_dsi_bridge_atomic_destroy_state,
+	.atomic_reset = cdns_dsi_bridge_atomic_reset,
 	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
 };
 
-- 
2.34.1

[PATCH v7 11/12] 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, the disable sequence for the display pipeline will look like:

	bridge[n]_disable
	...
	bridge[1]_disable

	encoder_disable
	crtc_disable

	bridge[1]_post_disable
	...
	bridge[n]_post_disable

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>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
 1 file changed, 181 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5186d2114a50..ad6290a4a528 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -74,6 +74,12 @@
  * also shares the &struct drm_plane_helper_funcs function table with the plane
  * helpers.
  */
+
+enum bridge_chain_operation_type {
+	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
+	DRM_BRIDGE_ENABLE_OR_DISABLE,
+};
+
 static void
 drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 				struct drm_plane_state *old_plane_state,
@@ -1122,74 +1128,12 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+crtc_disable(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
-	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;
 
-	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
-		const struct drm_encoder_helper_funcs *funcs;
-		struct drm_encoder *encoder;
-		struct drm_bridge *bridge;
-
-		/*
-		 * Shut down everything that's in the changeset and currently
-		 * still on. So need to check the old, saved state.
-		 */
-		if (!old_conn_state->crtc)
-			continue;
-
-		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
-
-		if (new_conn_state->crtc)
-			new_crtc_state = drm_atomic_get_new_crtc_state(
-						old_state,
-						new_conn_state->crtc);
-		else
-			new_crtc_state = NULL;
-
-		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
-		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
-			continue;
-
-		encoder = old_conn_state->best_encoder;
-
-		/* We shouldn't get this far if we didn't previously have
-		 * an encoder.. but WARN_ON() rather than explode.
-		 */
-		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);
-		}
-
-		drm_atomic_bridge_chain_post_disable(bridge, old_state);
-	}
-
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 		int ret;
@@ -1206,7 +1150,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		drm_dbg_atomic(dev, "disabling [CRTC:%d:%s]\n",
 			       crtc->base.id, crtc->name);
 
-
 		/* Right function depends upon target state. */
 		if (new_crtc_state->enable && funcs->prepare)
 			funcs->prepare(crtc);
@@ -1236,6 +1179,97 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 	}
 }
 
+static void
+encoder_bridge_chain_disable(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_state *old_crtc_state, *new_crtc_state;
+	int i;
+
+	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
+		const struct drm_encoder_helper_funcs *funcs;
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		/*
+		 * Shut down everything that's in the changeset and currently
+		 * still on. So need to check the old, saved state.
+		 */
+		if (!old_conn_state->crtc)
+			continue;
+
+		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
+
+		if (new_conn_state->crtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(
+						old_state,
+						new_conn_state->crtc);
+		else
+			new_crtc_state = NULL;
+
+		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
+		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
+			continue;
+
+		encoder = old_conn_state->best_encoder;
+
+		/* We shouldn't get this far if we didn't previously have
+		 * an encoder.. but WARN_ON() rather than explode.
+		 */
+		if (WARN_ON(!encoder))
+			continue;
+
+		/*
+		 * 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);
+
+		switch (op_type) {
+		case DRM_BRIDGE_ENABLE_OR_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_PRE_ENABLE_OR_POST_DISABLE:
+			drm_atomic_bridge_chain_post_disable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge operation (%d).\n", op_type);
+		}
+	}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
+
+	crtc_disable(dev, old_state);
+
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
+}
+
 /**
  * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
  * @dev: DRM device
@@ -1445,28 +1479,69 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
-/**
- * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
- * @dev: DRM device
- * @old_state: atomic state object with old state structures
- *
- * This function enables all the outputs with the new configuration which had to
- * be turned off for the update.
- *
- * For compatibility with legacy CRTC helpers this should be called after
- * drm_atomic_helper_commit_planes(), which is what the default commit function
- * does. But drivers with different needs can group the modeset commits together
- * and do the plane commits at the end. This is useful for drivers doing runtime
- * PM since planes updates then only happen when the CRTC is actually enabled.
- */
-void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
-					      struct drm_atomic_state *old_state)
+static void
+encoder_bridge_chain_enable(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_OR_POST_DISABLE:
+			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+			break;
+
+		case DRM_BRIDGE_ENABLE_OR_DISABLE:
+			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;
+		}
+	}
+}
+
+static void
+crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	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;
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1490,43 +1565,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 				funcs->commit(crtc);
 		}
 	}
-
-	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);
-	}
+}
+
+/**
+ * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
+ * @dev: DRM device
+ * @old_state: atomic state object with old state structures
+ *
+ * This function enables all the outputs with the new configuration which had to
+ * be turned off for the update.
+ *
+ * For compatibility with legacy CRTC helpers this should be called after
+ * drm_atomic_helper_commit_planes(), which is what the default commit function
+ * does. But drivers with different needs can group the modeset commits together
+ * and do the plane commits at the end. This is useful for drivers doing runtime
+ * PM since planes updates then only happen when the CRTC is actually enabled.
+ */
+void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
+					      struct drm_atomic_state *old_state)
+{
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
+
+	crtc_enable(dev, old_state);
+
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }
-- 
2.34.1

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

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

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

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 62 ++++++++++---------
 1 file changed, 34 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 d60254e1270c..cb79abd4c7e8 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -669,13 +669,28 @@ 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);
 	u32 val;
 
+	/*
+	 * The cdns-dsi controller needs to be disabled after it's DPI source
+	 * has stopped streaming. If this is not followed, there is a brief
+	 * window before DPI source is disabled and after cdns-dsi controller
+	 * has been disabled where the DPI stream is still on, but the cdns-dsi
+	 * controller is not ready anymore to accept the incoming signals. This
+	 * is one of the reasons why a shift in pixel colors is observed on
+	 * displays that have cdns-dsi as one of the bridges.
+	 *
+	 * To mitigate this, disable this bridge from the bridge post_disable()
+	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
+	 * hook gets called after the CRTC disable, where often many DPI sources
+	 * disable their streams.
+	 */
+
 	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
 	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
 		 DISP_EOT_GEN);
@@ -695,15 +710,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;
@@ -773,8 +779,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);
@@ -789,6 +795,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 
+	/*
+	 * The cdns-dsi controller needs to be enabled before it's DPI source
+	 * has begun streaming. If this is not followed, there is a brief window
+	 * after DPI source enable and before cdns-dsi controller enable where
+	 * the DPI stream is on, but the cdns-dsi controller is not ready to
+	 * accept the incoming signals. This is one of the reasons why a shift
+	 * in pixel colors is observed on displays that have cdns-dsi as one of
+	 * the bridges.
+	 *
+	 * To mitigate this, enable this bridge from the bridge pre_enable()
+	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
+	 * hook gets called before the CRTC enable, where often many DPI sources
+	 * enable their streams.
+	 */
+
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
@@ -935,19 +956,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,
@@ -1035,9 +1043,7 @@ cdns_dsi_bridge_atomic_reset(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_check = cdns_dsi_bridge_atomic_check,
 	.atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state,
-- 
2.34.1

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

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 11:26:19AM +0530, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> The crtc_* mode parameters do not get generated (duplicated in this
> case) from the regular parameters before the mode validation phase
> begins.
> 
> The rest of the code conditionally uses the crtc_* parameters only
> during the bridge enable phase, but sticks to the regular parameters
> for mode validation. In this singular instance, however, the driver
> tries to use the crtc_clock parameter even during the mode validation,
> causing the validation to fail.
> 
> 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")
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
-- 
With best wishes
Dmitry

Re: [PATCH v7 10/12] drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check()

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 11:26:24AM +0530, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> At present, the DSI mode configuration check happens during the
> _atomic_enable() phase, which is not really the best place for this.
> Moreover, if the mode is not valid, the driver gives a warning and
> continues the hardware configuration.
> 
> Move the DSI mode configuration check to _atomic_check() instead, which
> can properly report back any invalid mode, before the _enable phase even
> begins.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>  .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
>  1 file changed, 82 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
> 
> 	bridge[n]_disable
> 	...
> 	bridge[1]_disable
> 
> 	encoder_disable
> 	crtc_disable
> 
> 	bridge[1]_post_disable
> 	...
> 	bridge[n]_post_disable
> 
> 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.

The patch contains both refactoring of the corresponding functions and
changing of the order. Please split it into two separate commits.

> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
>  1 file changed, 181 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5186d2114a50..ad6290a4a528 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -74,6 +74,12 @@
>   * also shares the &struct drm_plane_helper_funcs function table with the plane
>   * helpers.
>   */
> +
> +enum bridge_chain_operation_type {
> +	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
> +	DRM_BRIDGE_ENABLE_OR_DISABLE,
> +};
> +

I have mixed feelings towards this approach. I doubt that it actually
helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
post_disable' arguments?

>  static void
>  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  				struct drm_plane_state *old_plane_state,
> @@ -1122,74 +1128,12 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>  }
>  
>  static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +crtc_disable(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
> -	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;
>  
> -	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
> -		const struct drm_encoder_helper_funcs *funcs;
> -		struct drm_encoder *encoder;
> -		struct drm_bridge *bridge;
> -
> -		/*
> -		 * Shut down everything that's in the changeset and currently
> -		 * still on. So need to check the old, saved state.
> -		 */
> -		if (!old_conn_state->crtc)
> -			continue;
> -
> -		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
> -
> -		if (new_conn_state->crtc)
> -			new_crtc_state = drm_atomic_get_new_crtc_state(
> -						old_state,
> -						new_conn_state->crtc);
> -		else
> -			new_crtc_state = NULL;
> -
> -		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> -		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> -			continue;
> -
> -		encoder = old_conn_state->best_encoder;
> -
> -		/* We shouldn't get this far if we didn't previously have
> -		 * an encoder.. but WARN_ON() rather than explode.
> -		 */
> -		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);
> -		}
> -
> -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
> -	}
> -
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  		int ret;
> @@ -1206,7 +1150,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		drm_dbg_atomic(dev, "disabling [CRTC:%d:%s]\n",
>  			       crtc->base.id, crtc->name);
>  
> -

Irrelevant, please drop.

>  		/* Right function depends upon target state. */
>  		if (new_crtc_state->enable && funcs->prepare)
>  			funcs->prepare(crtc);
> @@ -1236,6 +1179,97 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  	}
>  }
>  
> +static void
> +encoder_bridge_chain_disable(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_state *old_crtc_state, *new_crtc_state;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
> +		const struct drm_encoder_helper_funcs *funcs;
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		/*
> +		 * Shut down everything that's in the changeset and currently
> +		 * still on. So need to check the old, saved state.
> +		 */
> +		if (!old_conn_state->crtc)
> +			continue;
> +
> +		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
> +
> +		if (new_conn_state->crtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(
> +						old_state,
> +						new_conn_state->crtc);
> +		else
> +			new_crtc_state = NULL;
> +
> +		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> +		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = old_conn_state->best_encoder;
> +
> +		/* We shouldn't get this far if we didn't previously have
> +		 * an encoder.. but WARN_ON() rather than explode.
> +		 */
> +		if (WARN_ON(!encoder))
> +			continue;
> +
> +		/*
> +		 * 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);
> +
> +		switch (op_type) {
> +		case DRM_BRIDGE_ENABLE_OR_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_PRE_ENABLE_OR_POST_DISABLE:
> +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
> +			break;
> +
> +		default:
> +			drm_err(dev, "Unrecognized Encoder/Bridge operation (%d).\n", op_type);
> +		}
> +	}
> +}
> +
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> +	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
> +
> +	crtc_disable(dev, old_state);
> +
> +	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
> +}
> +
>  /**
>   * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
>   * @dev: DRM device
> @@ -1445,28 +1479,69 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>  	}
>  }
>  
> -/**
> - * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> - * @dev: DRM device
> - * @old_state: atomic state object with old state structures
> - *
> - * This function enables all the outputs with the new configuration which had to
> - * be turned off for the update.
> - *
> - * For compatibility with legacy CRTC helpers this should be called after
> - * drm_atomic_helper_commit_planes(), which is what the default commit function
> - * does. But drivers with different needs can group the modeset commits together
> - * and do the plane commits at the end. This is useful for drivers doing runtime
> - * PM since planes updates then only happen when the CRTC is actually enabled.
> - */
> -void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> -					      struct drm_atomic_state *old_state)
> +static void
> +encoder_bridge_chain_enable(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_OR_POST_DISABLE:
> +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> +			break;
> +
> +		case DRM_BRIDGE_ENABLE_OR_DISABLE:
> +			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;
> +		}
> +	}
> +}
> +
> +static void
> +crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
>  	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;
>  
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1490,43 +1565,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  				funcs->commit(crtc);
>  		}
>  	}
> -
> -	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);
> -	}
> +}
> +
> +/**
> + * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> + * @dev: DRM device
> + * @old_state: atomic state object with old state structures
> + *
> + * This function enables all the outputs with the new configuration which had to
> + * be turned off for the update.
> + *
> + * For compatibility with legacy CRTC helpers this should be called after
> + * drm_atomic_helper_commit_planes(), which is what the default commit function
> + * does. But drivers with different needs can group the modeset commits together
> + * and do the plane commits at the end. This is useful for drivers doing runtime
> + * PM since planes updates then only happen when the CRTC is actually enabled.
> + */
> +void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> +					      struct drm_atomic_state *old_state)
> +{
> +	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
> +
> +	crtc_enable(dev, old_state);
> +
> +	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
>  
>  	drm_atomic_helper_commit_writebacks(dev, old_state);
>  }
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Tomi Valkeinen]

Hi,

On 14/01/2025 07:56, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> The driver code doesn't have a Phy de-initialization path as yet, and so
> it does not clear the phy_initialized flag while suspending. This is a
> problem because after resume the driver looks at this flag to determine
> if a Phy re-initialization is required or not. It is in fact required
> because the hardware is resuming from a suspend, but the driver does not
> carry out any re-initialization causing the D-Phy to not work at all.
> 
> Call the counterparts of phy_init() and phy_power_on(), that are
> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
> flags so that the Phy can be initialized again when required.
> 
> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>   1 file changed, 5 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 056583e81155..039c5eb7fb66 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
>   	if (dsi->platform_ops && dsi->platform_ops->disable)
>   		dsi->platform_ops->disable(dsi);
>   
> +	dsi->phy_initialized = false;
> +	dsi->link_initialized = false;
> +	phy_power_off(dsi->dphy);
> +	phy_exit(dsi->dphy);
> +

The phy related lines are counterparts to what's done in 
cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(),

But is the phy_initialized even needed? phy_initialized() is called from 
cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the 
call in cdns_dsi_bridge_enable() be always skipped, as 
cdns_dsi_bridge_pre_enable() already set phy_initialized?

Same question for cdns_dsi_init_link(), although that's also called from 
cdns_dsi_transfer(), so we probably need dsi->link_initialized.

  Tomi

>   	pm_runtime_put(dsi->base.dev);
>   }
>   
> @@ -1130,7 +1135,6 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
>   	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;
>   }
>   

Re: [PATCH v7 10/12] drm/bridge: cdns-dsi: Move DSI mode check to _atomic_check()

[Tomi Valkeinen]

Hi,

On 14/01/2025 07:56, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
> 
> At present, the DSI mode configuration check happens during the
> _atomic_enable() phase, which is not really the best place for this.
> Moreover, if the mode is not valid, the driver gives a warning and
> continues the hardware configuration.
> 
> Move the DSI mode configuration check to _atomic_check() instead, which
> can properly report back any invalid mode, before the _enable phase even
> begins.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
>   1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index b8db984ea9fa..d60254e1270c 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -425,6 +425,17 @@
>   #define DSI_NULL_FRAME_OVERHEAD		6
>   #define DSI_EOT_PKT_SIZE		4
>   
> +struct cdns_dsi_bridge_state {
> +	struct drm_bridge_state base;
> +	struct cdns_dsi_cfg dsi_cfg;
> +};
> +
> +static inline struct cdns_dsi_bridge_state *
> +to_cdns_dsi_bridge_state(struct drm_bridge_state *bridge_state)
> +{
> +	return container_of(bridge_state, struct cdns_dsi_bridge_state, base);
> +}
> +
>   static inline struct cdns_dsi *input_to_dsi(struct cdns_dsi_input *input)
>   {
>   	return container_of(input, struct cdns_dsi, input);
> @@ -768,6 +779,9 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>   	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;
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
> +	struct cdns_dsi_bridge_state *dsi_state;
> +	struct drm_bridge_state *new_bridge_state;
>   	struct drm_display_mode *mode;
>   	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>   	unsigned long tx_byte_period;
> @@ -778,14 +792,19 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
>   	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
>   		return;
>   
> +	new_bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> +	if (WARN_ON(!new_bridge_state))
> +		return;
> +
> +	dsi_state = to_cdns_dsi_bridge_state(new_bridge_state);
> +	dsi_cfg = dsi_state->dsi_cfg;
> +
>   	if (dsi->platform_ops && dsi->platform_ops->enable)
>   		dsi->platform_ops->enable(dsi);
>   
>   	mode = &bridge->encoder->crtc->state->adjusted_mode;
>   	nlanes = output->dev->lanes;
>   
> -	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
> -
>   	cdns_dsi_init_link(dsi);
>   	cdns_dsi_hs_init(dsi);
>   
> @@ -956,6 +975,63 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
>   	return input_fmts;
>   }
>   
> +static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
> +					struct drm_bridge_state *bridge_state,
> +					struct drm_crtc_state *crtc_state,
> +					struct drm_connector_state *conn_state)
> +{
> +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> +	struct cdns_dsi *dsi = input_to_dsi(input);
> +	struct cdns_dsi_bridge_state *dsi_state = to_cdns_dsi_bridge_state(bridge_state);
> +	struct drm_display_mode *mode = &crtc_state->mode;

const

> +	struct cdns_dsi_cfg *dsi_cfg = &dsi_state->dsi_cfg;
> +
> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
> +}
> +
> +static struct drm_bridge_state *
> +cdns_dsi_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	if (WARN_ON(!bridge->base.state))
> +		return NULL;
> +
> +	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
> +	if (!dsi_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_bridge_duplicate_state(bridge, &dsi_state->base);

Hmm, if I'm not mistaken, you should copy the private state here 
(cdns_dsi_cfg).

> +	return &dsi_state->base;
> +}
> +
> +static void
> +cdns_dsi_bridge_atomic_destroy_state(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *state)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	dsi_state = to_cdns_dsi_bridge_state(state);
> +
> +	kfree(dsi_state);
> +}
> +
> +static struct drm_bridge_state *
> +cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge)
> +{
> +	struct cdns_dsi_bridge_state *dsi_state;
> +
> +	dsi_state = kzalloc(sizeof(*dsi_state), GFP_KERNEL);
> +	if (!dsi_state)
> +		return NULL;
> +
> +	memset(dsi_state, 0, sizeof(*dsi_state));
> +	dsi_state->base.bridge = bridge;
> +
> +	return &dsi_state->base;
> +}
> +
>   static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.attach = cdns_dsi_bridge_attach,
>   	.mode_valid = cdns_dsi_bridge_mode_valid,
> @@ -963,9 +1039,10 @@ static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
>   	.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_check = cdns_dsi_bridge_atomic_check,
> +	.atomic_duplicate_state = cdns_dsi_bridge_atomic_duplicate_state,
> +	.atomic_destroy_state = cdns_dsi_bridge_atomic_destroy_state,
> +	.atomic_reset = cdns_dsi_bridge_atomic_reset,
>   	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
>   };
>   

Other than the two small comments:

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

  Tomi

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

[Tomi Valkeinen]

Hi,

On 14/01/2025 13:24, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>
>> 	bridge[n]_disable
>> 	...
>> 	bridge[1]_disable
>>
>> 	encoder_disable
>> 	crtc_disable
>>
>> 	bridge[1]_post_disable
>> 	...
>> 	bridge[n]_post_disable
>>
>> 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.
> 
> The patch contains both refactoring of the corresponding functions and
> changing of the order. Please split it into two separate commits.
> 
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 5186d2114a50..ad6290a4a528 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -74,6 +74,12 @@
>>    * also shares the &struct drm_plane_helper_funcs function table with the plane
>>    * helpers.
>>    */
>> +
>> +enum bridge_chain_operation_type {
>> +	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>> +	DRM_BRIDGE_ENABLE_OR_DISABLE,
>> +};
>> +
> 
> I have mixed feelings towards this approach. I doubt that it actually
> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
> post_disable' arguments?

If my memory serves, I suggested the enum. I don't like it too much 
either. But neither do I like the boolean that much, as this is not a 
yes/no, on/off case... Then again, maybe boolean is fine here, as the 
"off" case is the "normal/default" case so it's still ok-ish.

But this doesn't matter much, I think. It's internal, and can be 
trivially adjusted later.

  Tomi

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Aradhya Bhatia]

Hi Tomi,

On 1/14/25 18:00, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 07:56, Aradhya Bhatia wrote:
>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>
>> The driver code doesn't have a Phy de-initialization path as yet, and so
>> it does not clear the phy_initialized flag while suspending. This is a
>> problem because after resume the driver looks at this flag to determine
>> if a Phy re-initialization is required or not. It is in fact required
>> because the hardware is resuming from a suspend, but the driver does not
>> carry out any re-initialization causing the D-Phy to not work at all.
>>
>> Call the counterparts of phy_init() and phy_power_on(), that are
>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
>> flags so that the Phy can be initialized again when required.
>>
>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>>   1 file changed, 5 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 056583e81155..039c5eb7fb66 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct
>> drm_bridge *bridge)
>>       if (dsi->platform_ops && dsi->platform_ops->disable)
>>           dsi->platform_ops->disable(dsi);
>>   +    dsi->phy_initialized = false;
>> +    dsi->link_initialized = false;
>> +    phy_power_off(dsi->dphy);
>> +    phy_exit(dsi->dphy);
>> +
> 
> The phy related lines are counterparts to what's done in
> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(),
> 
> But is the phy_initialized even needed? phy_initialized() is called from
> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the
> call in cdns_dsi_bridge_enable() be always skipped, as
> cdns_dsi_bridge_pre_enable() already set phy_initialized?

Yes, that is how the behavior has been. The initialization calls inside
the _bridge_enable() end-up getting skipped.

My first thought after reading your comments was to remove the init
calls entirely from the _bridge_pre_enable(), and drop the
phy_initialized flag too, and let _bridge_enable() only handle the init.
The _bridge_enable() will anyway get renamed to _bridge_pre_enable(),
while the existing _bridge_pre_enable() will get dropped, by the last
patch of this series.

But since this patch is intended as a fix, it will get applied to
previous versions while that last patch of the series won't... and then
we may end up having init calls only from _bridge_enable() for the older
versions.
Also, given all the fixes in the series, there is a possibility that an
older-version of the driver might become functional (except for the
color shift issue).

My question then is, would it be a cause for concern if all the init
calls are handled from the _bridge_enable() only?

(one of the potential concerns detailed below)

> 
> Same question for cdns_dsi_init_link(), although that's also called from
> cdns_dsi_transfer(), so we probably need dsi->link_initialized.
> 

Don't you think we'd need the phy to also be initialized for the DCS
command to work?

Usually, since DSI is among the initial bridges to get pre_enabled, the
Link and Phy are both initialized by the time cdns_dsi_transfer() is
called. So, even if cdns_dsi_transfer() doesn't call for
cdns_dsi_hs_init(), it is able to work fine.

If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting
it to _bridge_enable() (like I suggested above) would be problematic
without fixing it here.


Regards
Aradhya

> 
>>       pm_runtime_put(dsi->base.dev);
>>   }
>>   @@ -1130,7 +1135,6 @@ static int __maybe_unused
>> cdns_dsi_suspend(struct device *dev)
>>       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;
>>   }
>>   
> 

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Tomi Valkeinen]

Hi,

On 14/01/2025 16:44, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 1/14/25 18:00, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 14/01/2025 07:56, Aradhya Bhatia wrote:
>>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>>
>>> The driver code doesn't have a Phy de-initialization path as yet, and so
>>> it does not clear the phy_initialized flag while suspending. This is a
>>> problem because after resume the driver looks at this flag to determine
>>> if a Phy re-initialization is required or not. It is in fact required
>>> because the hardware is resuming from a suspend, but the driver does not
>>> carry out any re-initialization causing the D-Phy to not work at all.
>>>
>>> Call the counterparts of phy_init() and phy_power_on(), that are
>>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
>>> flags so that the Phy can be initialized again when required.
>>>
>>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>>> ---
>>>    drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>>>    1 file changed, 5 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 056583e81155..039c5eb7fb66 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct
>>> drm_bridge *bridge)
>>>        if (dsi->platform_ops && dsi->platform_ops->disable)
>>>            dsi->platform_ops->disable(dsi);
>>>    +    dsi->phy_initialized = false;
>>> +    dsi->link_initialized = false;
>>> +    phy_power_off(dsi->dphy);
>>> +    phy_exit(dsi->dphy);
>>> +
>>
>> The phy related lines are counterparts to what's done in
>> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(),
>>
>> But is the phy_initialized even needed? phy_initialized() is called from
>> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the
>> call in cdns_dsi_bridge_enable() be always skipped, as
>> cdns_dsi_bridge_pre_enable() already set phy_initialized?
> 
> Yes, that is how the behavior has been. The initialization calls inside
> the _bridge_enable() end-up getting skipped.
> 
> My first thought after reading your comments was to remove the init
> calls entirely from the _bridge_pre_enable(), and drop the
> phy_initialized flag too, and let _bridge_enable() only handle the init.

Isn't that the wrong way around? If currently bridge_pre_enable enables 
the phy, your suggestion above would change that. I would think keeping 
the init calls in bridge_pre_enable, and drop from bridge_enable.

> The _bridge_enable() will anyway get renamed to _bridge_pre_enable(),
> while the existing _bridge_pre_enable() will get dropped, by the last
> patch of this series.

Ok, but you can't do a fix that'll only be right after some future patch 
does more changes =).

> But since this patch is intended as a fix, it will get applied to
> previous versions while that last patch of the series won't... and then

Speaking of which, I think you should cc: stable for the ones that 
should be applied to earlier kernels. And it would be good to have all 
such patches first in the series, to decrease any dependencies.

> we may end up having init calls only from _bridge_enable() for the older
> versions.
> Also, given all the fixes in the series, there is a possibility that an
> older-version of the driver might become functional (except for the
> color shift issue).
> 
> My question then is, would it be a cause for concern if all the init
> calls are handled from the _bridge_enable() only?

I'm not sure I follow here. Don't we want the init calls to happen in 
the pre_enable phase, both before and after the sequence change (patch 12)?

But generally speaking, yes, it's good to keep fixes simple, and do 
cleanups later on top. Keeping that in mind, maybe this current patch is 
fine as it is. Although... if the init is done in pre_enable, shouldn't 
the deinit be done in post_disable?

> (one of the potential concerns detailed below)
> 
>>
>> Same question for cdns_dsi_init_link(), although that's also called from
>> cdns_dsi_transfer(), so we probably need dsi->link_initialized.
>>
> 
> Don't you think we'd need the phy to also be initialized for the DCS
> command to work?

I'm sure we do. But the driver doesn't do that currently, does it? Which 
I did find a bit odd, but I'm not familiar with the HW.

However, my comment was related to calling cdns_dsi_init_link() in both 
cdns_dsi_bridge_enable and cdns_dsi_bridge_pre_enable functions. In this 
case the call in the cdns_dsi_bridge_enable function is a no-op, similar 
to calling cdns_dsi_hs_init().

But, if changed, that's also a cleanup, so maybe better keep away from 
fix patches.

> Usually, since DSI is among the initial bridges to get pre_enabled, the
> Link and Phy are both initialized by the time cdns_dsi_transfer() is
> called. So, even if cdns_dsi_transfer() doesn't call for
> cdns_dsi_hs_init(), it is able to work fine.
> 
> If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting
> it to _bridge_enable() (like I suggested above) would be problematic
> without fixing it here.

I don't know what how the HW works, but we definitely need PHY to send 
DCS commands. But we don't necessarily need HS mode, LP works fine 
(usually). It's just not clear to me what exactly cdns_dsi_hs_init() and 
cdns_dsi_init_link() do. What is "link"? Looks like cdns_dsi_init_link 
is doing some PHY stuff, which is kind of strange thing to do, as 
phy_init() and phy_power_on() are only done later.

In any case, yes, the cdns_dsi_transfer() has to make sure we have LP/HS 
working. So indeed it might mean calling both functions. This is, 
however, perhaps a different topic, best left out of this series.

  Tomi

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Aradhya Bhatia]

Hi,

On 1/14/25 20:50, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 16:44, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 1/14/25 18:00, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 14/01/2025 07:56, Aradhya Bhatia wrote:
>>>> From: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>
>>>> The driver code doesn't have a Phy de-initialization path as yet,
>>>> and so
>>>> it does not clear the phy_initialized flag while suspending. This is a
>>>> problem because after resume the driver looks at this flag to determine
>>>> if a Phy re-initialization is required or not. It is in fact required
>>>> because the hardware is resuming from a suspend, but the driver does
>>>> not
>>>> carry out any re-initialization causing the D-Phy to not work at all.
>>>>
>>>> Call the counterparts of phy_init() and phy_power_on(), that are
>>>> phy_exit() and phy_power_off(), from _bridge_disable(), and clear the
>>>> flags so that the Phy can be initialized again when required.
>>>>
>>>> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>>>> ---
>>>>    drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
>>>>    1 file changed, 5 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 056583e81155..039c5eb7fb66 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> @@ -672,6 +672,11 @@ static void cdns_dsi_bridge_disable(struct
>>>> drm_bridge *bridge)
>>>>        if (dsi->platform_ops && dsi->platform_ops->disable)
>>>>            dsi->platform_ops->disable(dsi);
>>>>    +    dsi->phy_initialized = false;
>>>> +    dsi->link_initialized = false;
>>>> +    phy_power_off(dsi->dphy);
>>>> +    phy_exit(dsi->dphy);
>>>> +
>>>
>>> The phy related lines are counterparts to what's done in
>>> cdns_dsi_hs_init(), right? Maybe add cdns_dsi_hs_uninit(),
>>>
>>> But is the phy_initialized even needed? phy_initialized() is called from
>>> cdns_dsi_bridge_enable() and cdns_dsi_bridge_pre_enable(). Won't the
>>> call in cdns_dsi_bridge_enable() be always skipped, as
>>> cdns_dsi_bridge_pre_enable() already set phy_initialized?
>>
>> Yes, that is how the behavior has been. The initialization calls inside
>> the _bridge_enable() end-up getting skipped.
>>
>> My first thought after reading your comments was to remove the init
>> calls entirely from the _bridge_pre_enable(), and drop the
>> phy_initialized flag too, and let _bridge_enable() only handle the init.
> 
> Isn't that the wrong way around? If currently bridge_pre_enable enables
> the phy, your suggestion above would change that. I would think keeping
> the init calls in bridge_pre_enable, and drop from bridge_enable.
> 
>> The _bridge_enable() will anyway get renamed to _bridge_pre_enable(),
>> while the existing _bridge_pre_enable() will get dropped, by the last
>> patch of this series.
> 
> Ok, but you can't do a fix that'll only be right after some future patch
> does more changes =).

Yeah, that would be wrong. =)

> 
>> But since this patch is intended as a fix, it will get applied to
>> previous versions while that last patch of the series won't... and then
> 
> Speaking of which, I think you should cc: stable for the ones that
> should be applied to earlier kernels. And it would be good to have all
> such patches first in the series, to decrease any dependencies.

Will do!

> 
>> we may end up having init calls only from _bridge_enable() for the older
>> versions.
>> Also, given all the fixes in the series, there is a possibility that an
>> older-version of the driver might become functional (except for the
>> color shift issue).
>>
>> My question then is, would it be a cause for concern if all the init
>> calls are handled from the _bridge_enable() only?
> 
> I'm not sure I follow here. Don't we want the init calls to happen in
> the pre_enable phase, both before and after the sequence change (patch 12)?
> 

It is, now. For that brief period, I was considering to keep them only
in _bridge_enable().

> But generally speaking, yes, it's good to keep fixes simple, and do
> cleanups later on top. Keeping that in mind, maybe this current patch is
> fine as it is. Although... if the init is done in pre_enable, shouldn't
> the deinit be done in post_disable?

Yes, I will move the deinit to _bridge_post_disable().


So, if we keep the fix limited to deinit in _bridge_post_disable(), then
the cleanup would involve dropping the init calls from _bridge_enable().
And then the patch-12 would do 3 things -

	1. Drop older _bridge_pre_enable()
	2. Rename old _bridge_enable() to _bridge_pre_enable()
	3. Since the _old_ _bridge_enable() has the calls dropped in the
	   cleanup patch, add those calls again in the _new_
	   _bridge_pre_enable() (which are really the same function
	   bodies).

Do you think we can instead skip the cleanup patch, as well as #3 from
patch-12?

Fun fact: We already have patch-4 which fixes the order of init calls in
_bridge_enable()! =)

> 
>> (one of the potential concerns detailed below)
>>
>>>
>>> Same question for cdns_dsi_init_link(), although that's also called from
>>> cdns_dsi_transfer(), so we probably need dsi->link_initialized.
>>>
>>
>> Don't you think we'd need the phy to also be initialized for the DCS
>> command to work?
> 
> I'm sure we do. But the driver doesn't do that currently, does it? Which
> I did find a bit odd, but I'm not familiar with the HW.
> 
> However, my comment was related to calling cdns_dsi_init_link() in both
> cdns_dsi_bridge_enable and cdns_dsi_bridge_pre_enable functions. In this
> case the call in the cdns_dsi_bridge_enable function is a no-op, similar
> to calling cdns_dsi_hs_init().
> 
> But, if changed, that's also a cleanup, so maybe better keep away from
> fix patches.
> 
>> Usually, since DSI is among the initial bridges to get pre_enabled, the
>> Link and Phy are both initialized by the time cdns_dsi_transfer() is
>> called. So, even if cdns_dsi_transfer() doesn't call for
>> cdns_dsi_hs_init(), it is able to work fine.
>>
>> If DCS commands do indeed require the cdns_dsi_hs_init(), then shifting
>> it to _bridge_enable() (like I suggested above) would be problematic
>> without fixing it here.
> 
> I don't know what how the HW works, but we definitely need PHY to send
> DCS commands. But we don't necessarily need HS mode, LP works fine
> (usually). It's just not clear to me what exactly cdns_dsi_hs_init() and
> cdns_dsi_init_link() do. What is "link"? Looks like cdns_dsi_init_link
> is doing some PHY stuff, which is kind of strange thing to do, as
> phy_init() and phy_power_on() are only done later.

That is where my confusion is too. A quick look into the TRM didn't
help me with distinctions either.

> 
> In any case, yes, the cdns_dsi_transfer() has to make sure we have LP/HS
> working. So indeed it might mean calling both functions. This is,
> however, perhaps a different topic, best left out of this series.
> 

Alright. Since it is decided to keep the init calls in
_bridge_pre_enable(), cdns_dsi_transfer() is not going to be affected
any more than it already is, and we won't be breaking anything new.

I guess there can be some trial and error done to find whether
cdns_dsi_transfer() is really dependent on cdns_dsi_hs_init() -
but yes, let's keep that out of this series' scope.


Regards
Aradhya

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

[Aradhya Bhatia]

On 1/14/25 18:34, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 13:24, Dmitry Baryshkov wrote:
>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>>
>>>     bridge[n]_disable
>>>     ...
>>>     bridge[1]_disable
>>>
>>>     encoder_disable
>>>     crtc_disable
>>>
>>>     bridge[1]_post_disable
>>>     ...
>>>     bridge[n]_post_disable
>>>
>>> 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.
>>
>> The patch contains both refactoring of the corresponding functions and
>> changing of the order. Please split it into two separate commits.
>>
>>>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
>>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 5186d2114a50..ad6290a4a528 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -74,6 +74,12 @@
>>>    * also shares the &struct drm_plane_helper_funcs function table
>>> with the plane
>>>    * helpers.
>>>    */
>>> +
>>> +enum bridge_chain_operation_type {
>>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
>>> +};
>>> +
>>
>> I have mixed feelings towards this approach. I doubt that it actually
>> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
>> post_disable' arguments?
> 
> If my memory serves, I suggested the enum. I don't like it too much
> either. But neither do I like the boolean that much, as this is not a
> yes/no, on/off case... Then again, maybe boolean is fine here, as the
> "off" case is the "normal/default" case so it's still ok-ish.
> 
> But this doesn't matter much, I think. It's internal, and can be
> trivially adjusted later.
> 

Alright! I will drop the enum reference entirely, and just use the
booleans.

Regards
Aradhya

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

[Maxime Ripard]

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

On Tue, Jan 14, 2025 at 10:05:56PM +0530, Aradhya Bhatia wrote:
> 
> 
> On 1/14/25 18:34, Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 14/01/2025 13:24, Dmitry Baryshkov wrote:
> >> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
> >>>
> >>>     bridge[n]_disable
> >>>     ...
> >>>     bridge[1]_disable
> >>>
> >>>     encoder_disable
> >>>     crtc_disable
> >>>
> >>>     bridge[1]_post_disable
> >>>     ...
> >>>     bridge[n]_post_disable
> >>>
> >>> 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.
> >>
> >> The patch contains both refactoring of the corresponding functions and
> >> changing of the order. Please split it into two separate commits.
> >>
> >>>
> >>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> >>> ---
> >>>   drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
> >>>   1 file changed, 181 insertions(+), 119 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >>> b/drivers/gpu/drm/drm_atomic_helper.c
> >>> index 5186d2114a50..ad6290a4a528 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>> @@ -74,6 +74,12 @@
> >>>    * also shares the &struct drm_plane_helper_funcs function table
> >>> with the plane
> >>>    * helpers.
> >>>    */
> >>> +
> >>> +enum bridge_chain_operation_type {
> >>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
> >>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
> >>> +};
> >>> +
> >>
> >> I have mixed feelings towards this approach. I doubt that it actually
> >> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
> >> post_disable' arguments?
> > 
> > If my memory serves, I suggested the enum. I don't like it too much
> > either. But neither do I like the boolean that much, as this is not a
> > yes/no, on/off case... Then again, maybe boolean is fine here, as the
> > "off" case is the "normal/default" case so it's still ok-ish.
> > 
> > But this doesn't matter much, I think. It's internal, and can be
> > trivially adjusted later.
> > 
> 
> Alright! I will drop the enum reference entirely, and just use the
> booleans.

Whatever you do, I think that we're at a point where the bridge chain
traversal is complicated enough that we'll want to have tests for all
cases.

Maxime

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

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

[Aradhya Bhatia]

On 1/14/25 22:13, Maxime Ripard wrote:
> On Tue, Jan 14, 2025 at 10:05:56PM +0530, Aradhya Bhatia wrote:
>>
>>
>> On 1/14/25 18:34, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 14/01/2025 13:24, Dmitry Baryshkov wrote:
>>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>>>>
>>>>>     bridge[n]_disable
>>>>>     ...
>>>>>     bridge[1]_disable
>>>>>
>>>>>     encoder_disable
>>>>>     crtc_disable
>>>>>
>>>>>     bridge[1]_post_disable
>>>>>     ...
>>>>>     bridge[n]_post_disable
>>>>>
>>>>> 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.
>>>>
>>>> The patch contains both refactoring of the corresponding functions and
>>>> changing of the order. Please split it into two separate commits.
>>>>
>>>>>
>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>>>>> ---
>>>>>   drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
>>>>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index 5186d2114a50..ad6290a4a528 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -74,6 +74,12 @@
>>>>>    * also shares the &struct drm_plane_helper_funcs function table
>>>>> with the plane
>>>>>    * helpers.
>>>>>    */
>>>>> +
>>>>> +enum bridge_chain_operation_type {
>>>>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>>>>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
>>>>> +};
>>>>> +
>>>>
>>>> I have mixed feelings towards this approach. I doubt that it actually
>>>> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
>>>> post_disable' arguments?
>>>
>>> If my memory serves, I suggested the enum. I don't like it too much
>>> either. But neither do I like the boolean that much, as this is not a
>>> yes/no, on/off case... Then again, maybe boolean is fine here, as the
>>> "off" case is the "normal/default" case so it's still ok-ish.
>>>
>>> But this doesn't matter much, I think. It's internal, and can be
>>> trivially adjusted later.
>>>
>>
>> Alright! I will drop the enum reference entirely, and just use the
>> booleans.
> 
> Whatever you do, I think that we're at a point where the bridge chain
> traversal is complicated enough that we'll want to have tests for all
> cases.
>

I don't think I follow. Which all cases are you referring to?


Regards
Aradhya

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Tomi Valkeinen]

Hi,

On 14/01/2025 18:32, Aradhya Bhatia wrote:

>> But generally speaking, yes, it's good to keep fixes simple, and do
>> cleanups later on top. Keeping that in mind, maybe this current patch is
>> fine as it is. Although... if the init is done in pre_enable, shouldn't
>> the deinit be done in post_disable?
> 
> Yes, I will move the deinit to _bridge_post_disable().
> 
> 
> So, if we keep the fix limited to deinit in _bridge_post_disable(), then
> the cleanup would involve dropping the init calls from _bridge_enable().
> And then the patch-12 would do 3 things -
> 
> 	1. Drop older _bridge_pre_enable()
> 	2. Rename old _bridge_enable() to _bridge_pre_enable()
> 	3. Since the _old_ _bridge_enable() has the calls dropped in the
> 	   cleanup patch, add those calls again in the _new_
> 	   _bridge_pre_enable() (which are really the same function
> 	   bodies).

I would think patch-12 differently: it doesn't do what you list above, 
but rather combines the current pre_enable() and enable() into a new 
pre_enable().

> Do you think we can instead skip the cleanup patch, as well as #3 from
> patch-12?

Yes, I think the cleanup patch can just be dropped. It's not really 
relevant.

> Fun fact: We already have patch-4 which fixes the order of init calls in
> _bridge_enable()! =)

Right. And I guess that fix doesn't fix anything in practice, as those 
init calls are no-ops in the bridge_enable()...

It's a bit difficult to make meaningful fixes when things are so badly 
messed up =).

So, maybe try to arrange the series so that the obvious "makes-sense" 
fixes for stable are in the beginning. So... patches 1, 3, 5? And then 
work towards the patch 12.

And I'll try to not nit-pick too much, so that we can actually get this 
series merged, and then later do the cleanups on top =).

  Tomi

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

[Aradhya Bhatia]

Hi Dmitry,

On 14/01/25 16:54, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>
>> 	bridge[n]_disable
>> 	...
>> 	bridge[1]_disable
>>
>> 	encoder_disable
>> 	crtc_disable
>>
>> 	bridge[1]_post_disable
>> 	...
>> 	bridge[n]_post_disable
>>
>> 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.
> 
> The patch contains both refactoring of the corresponding functions and
> changing of the order. Please split it into two separate commits.
> 

I assume that you already understand this, so this is just for the
record -

There is no trivial way to do this. Initially, this is what I had in my
mind - about what the split patches would look like.

#1
 * refactoring of entire loops into separate functions.
 * Separate out the pre_enable and enable operations inside the
   encoder_bridge_enable().
 * call them in their (seemingly) original sequences
	- crtc_enable
	- encoder_bridge_enable(pre_enable)
	- encoder_bridge_enable(!pre_enable)

#2
 * rearrange the calls to re-order the operation
	- encoder_bridge_enable(pre_enable)
	- crtc_enable
	- encoder_bridge_enable(!pre_enable)

This would have made the patch#2 seem quite trivial, as patch#1 would
take the brunt of changes, while keeping the functionality intact.


What I have now realized is that, the above isn't possible,
unfortunately. The moment we separate out pre_enable and enable into 2
different calls, the overall sequence gets even changed when there are
multiple pipelines in the system.

So to implement the split properly, the first patch would look like this

#1
 * refactoring of entire loops into separate functions.
 * The calling sequences will be as follows -
 	- crtc_enable()
	- encoder_bridge_enable()
		- this will do both pre_enable and enable
		  unconditionally.

The pre_enable and enable operations wouldn't be separated in patch 1,
just that the crtc enable and encoder bridge pre_enable/enable loops
would be put into individual functions.

The next patch will then introduce booleans, and separate out pre_enable
and enable, and implement the new order -

#2
 * pre_enable and enable operations will be conditionally segregated
   inside encoder_bridge_enable(), based on the pre_enable boolean.
 * The calling sequences will then be updated to -
	- encoder_bridge_enable(pre_enable)
	- crtc_enable()
	- encoder_bridge_enable(!pre_enable)

This wouldn't be all that much trivial as I had imagined it to be earlier.

It would also *kind of* like these patches in a previous revision,
v5:11/13 [0], and v5:12/13 [1]. The only differences being,

1) these older patches separated out only the bridge/encoder operations
into a function, and not the crtc operations, and

2) An enum is being used instead of the booleans.


I believe this is what you are looking for? If I have misunderstood
something, do let me know.


Regards
Aradhya


[0]: v5:11/13
drm/atomic-helper: Separate out Encoder-Bridge enable and disable
https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/

Re: [PATCH v7 03/12] drm/bridge: cdns-dsi: Fix phy de-init and flag it so

[Aradhya Bhatia]

Hi Tomi,

On 15/01/25 13:47, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 18:32, Aradhya Bhatia wrote:
> 
>>> But generally speaking, yes, it's good to keep fixes simple, and do
>>> cleanups later on top. Keeping that in mind, maybe this current patch is
>>> fine as it is. Although... if the init is done in pre_enable, shouldn't
>>> the deinit be done in post_disable?
>>
>> Yes, I will move the deinit to _bridge_post_disable().
>>
>>
>> So, if we keep the fix limited to deinit in _bridge_post_disable(), then
>> the cleanup would involve dropping the init calls from _bridge_enable().
>> And then the patch-12 would do 3 things -
>>
>>     1. Drop older _bridge_pre_enable()
>>     2. Rename old _bridge_enable() to _bridge_pre_enable()
>>     3. Since the _old_ _bridge_enable() has the calls dropped in the
>>        cleanup patch, add those calls again in the _new_
>>        _bridge_pre_enable() (which are really the same function
>>        bodies).
> 
> I would think patch-12 differently: it doesn't do what you list above,
> but rather combines the current pre_enable() and enable() into a new
> pre_enable().

Right, yes!

> 
>> Do you think we can instead skip the cleanup patch, as well as #3 from
>> patch-12?
> 
> Yes, I think the cleanup patch can just be dropped. It's not really
> relevant.
> 
>> Fun fact: We already have patch-4 which fixes the order of init calls in
>> _bridge_enable()! =)
> 
> Right. And I guess that fix doesn't fix anything in practice, as those
> init calls are no-ops in the bridge_enable()...

Yeah, it doesn't do anything... until patch-12 comes back in picture.
So, I shall drop patch-4 too as there's no point in getting that patch
backported. And I will let patch-12 take care of the correct ordering.

> 
> It's a bit difficult to make meaningful fixes when things are so badly
> messed up =).
> 
> So, maybe try to arrange the series so that the obvious "makes-sense"
> fixes for stable are in the beginning. So... patches 1, 3, 5? And then
> work towards the patch 12.
> 

Yes, this sounds good.


Regards
Aradhya

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

[Dmitry Baryshkov]

On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
> 
> On 14/01/25 16:54, Dmitry Baryshkov wrote:
> > On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
> >>
> >> 	bridge[n]_disable
> >> 	...
> >> 	bridge[1]_disable
> >>
> >> 	encoder_disable
> >> 	crtc_disable
> >>
> >> 	bridge[1]_post_disable
> >> 	...
> >> 	bridge[n]_post_disable
> >>
> >> 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.
> > 
> > The patch contains both refactoring of the corresponding functions and
> > changing of the order. Please split it into two separate commits.
> > 
> 
> I assume that you already understand this, so this is just for the
> record -
> 
> There is no trivial way to do this. Initially, this is what I had in my
> mind - about what the split patches would look like.
> 
> #1
>  * refactoring of entire loops into separate functions.
>  * Separate out the pre_enable and enable operations inside the
>    encoder_bridge_enable().
>  * call them in their (seemingly) original sequences
> 	- crtc_enable
> 	- encoder_bridge_enable(pre_enable)
> 	- encoder_bridge_enable(!pre_enable)
> 
> #2
>  * rearrange the calls to re-order the operation
> 	- encoder_bridge_enable(pre_enable)
> 	- crtc_enable
> 	- encoder_bridge_enable(!pre_enable)
> 
> This would have made the patch#2 seem quite trivial, as patch#1 would
> take the brunt of changes, while keeping the functionality intact.
> 
> 
> What I have now realized is that, the above isn't possible,
> unfortunately. The moment we separate out pre_enable and enable into 2
> different calls, the overall sequence gets even changed when there are
> multiple pipelines in the system.
> 
> So to implement the split properly, the first patch would look like this
> 
> #1
>  * refactoring of entire loops into separate functions.
>  * The calling sequences will be as follows -
>  	- crtc_enable()
> 	- encoder_bridge_enable()
> 		- this will do both pre_enable and enable
> 		  unconditionally.
> 
> The pre_enable and enable operations wouldn't be separated in patch 1,
> just that the crtc enable and encoder bridge pre_enable/enable loops
> would be put into individual functions.
> 
> The next patch will then introduce booleans, and separate out pre_enable
> and enable, and implement the new order -
> 
> #2
>  * pre_enable and enable operations will be conditionally segregated
>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>  * The calling sequences will then be updated to -
> 	- encoder_bridge_enable(pre_enable)
> 	- crtc_enable()
> 	- encoder_bridge_enable(!pre_enable)


I'd say slightly differently:

#1 Refactor loops into separate functions:
  - crtc_enable()
  - encoder_bridge_enable(), loops over encoders and calls
    pre_enable(bridges), enable(encoder), enable(bridges)

#2 Split loops into separate functions:
  - crtc_enable()
  - encoder_bridge_pre_enable(), loops over encoders and calls
    pre_enable(bridges),
  - encoder_bridge_enable(), loops over encoders and calls
    enable(encoder), enable(bridges)

#3 Move crtc_enable() calls:
  - encoder_bridge_pre_enable(), loops over encoders and calls
    pre_enable(bridges),
  - crtc_enable()
  - encoder_bridge_enable(), loops over encoders and calls
    enable(encoder), enable(bridges)

You might use enum or booleans to implement encoder_bridge_pre_enable(),
or just keep it as a completely separate function (might be a better
option).

The reason why I'm suggesting it is pretty easy: your patch performs two
refactorings at the same time. If one of the drivers breaks, we have no
way to identify, which of the refactorings caused the break.

> 
> This wouldn't be all that much trivial as I had imagined it to be earlier.
> 
> It would also *kind of* like these patches in a previous revision,
> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
> 
> 1) these older patches separated out only the bridge/encoder operations
> into a function, and not the crtc operations, and
> 
> 2) An enum is being used instead of the booleans.
> 
> 
> I believe this is what you are looking for? If I have misunderstood
> something, do let me know.
> 
> 
> Regards
> Aradhya
> 
> 
> [0]: v5:11/13
> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
> 
> [1]: v5:12/13
> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
> 

-- 
With best wishes
Dmitry

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

[Aradhya Bhatia]

Hi Dmitry,

On 20/01/25 14:08, Dmitry Baryshkov wrote:
> On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
>> Hi Dmitry,
>>
>> On 14/01/25 16:54, Dmitry Baryshkov wrote:
>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>>>
>>>> 	bridge[n]_disable
>>>> 	...
>>>> 	bridge[1]_disable
>>>>
>>>> 	encoder_disable
>>>> 	crtc_disable
>>>>
>>>> 	bridge[1]_post_disable
>>>> 	...
>>>> 	bridge[n]_post_disable
>>>>
>>>> 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.
>>>
>>> The patch contains both refactoring of the corresponding functions and
>>> changing of the order. Please split it into two separate commits.
>>>
>>
>> I assume that you already understand this, so this is just for the
>> record -
>>
>> There is no trivial way to do this. Initially, this is what I had in my
>> mind - about what the split patches would look like.
>>
>> #1
>>  * refactoring of entire loops into separate functions.
>>  * Separate out the pre_enable and enable operations inside the
>>    encoder_bridge_enable().
>>  * call them in their (seemingly) original sequences
>> 	- crtc_enable
>> 	- encoder_bridge_enable(pre_enable)
>> 	- encoder_bridge_enable(!pre_enable)
>>
>> #2
>>  * rearrange the calls to re-order the operation
>> 	- encoder_bridge_enable(pre_enable)
>> 	- crtc_enable
>> 	- encoder_bridge_enable(!pre_enable)
>>
>> This would have made the patch#2 seem quite trivial, as patch#1 would
>> take the brunt of changes, while keeping the functionality intact.
>>
>>
>> What I have now realized is that, the above isn't possible,
>> unfortunately. The moment we separate out pre_enable and enable into 2
>> different calls, the overall sequence gets even changed when there are
>> multiple pipelines in the system.
>>
>> So to implement the split properly, the first patch would look like this
>>
>> #1
>>  * refactoring of entire loops into separate functions.
>>  * The calling sequences will be as follows -
>>  	- crtc_enable()
>> 	- encoder_bridge_enable()
>> 		- this will do both pre_enable and enable
>> 		  unconditionally.
>>
>> The pre_enable and enable operations wouldn't be separated in patch 1,
>> just that the crtc enable and encoder bridge pre_enable/enable loops
>> would be put into individual functions.
>>
>> The next patch will then introduce booleans, and separate out pre_enable
>> and enable, and implement the new order -
>>
>> #2
>>  * pre_enable and enable operations will be conditionally segregated
>>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>>  * The calling sequences will then be updated to -
>> 	- encoder_bridge_enable(pre_enable)
>> 	- crtc_enable()
>> 	- encoder_bridge_enable(!pre_enable)
> 
> 
> I'd say slightly differently:
> 
> #1 Refactor loops into separate functions:
>   - crtc_enable()
>   - encoder_bridge_enable(), loops over encoders and calls
>     pre_enable(bridges), enable(encoder), enable(bridges)
> 
> #2 Split loops into separate functions:
>   - crtc_enable()
>   - encoder_bridge_pre_enable(), loops over encoders and calls
>     pre_enable(bridges),
>   - encoder_bridge_enable(), loops over encoders and calls
>     enable(encoder), enable(bridges)
> 

When we consider setups with multiple independent displays, there are
often multiple crtcs in the system, each with their own encoder-bridge
chain.

In such a scenario, the sequence of crtc/encoder/bridge enable calls
after the #2 that you suggested, will not the remain same as that after
#1 (which is the _original_ sequence of calls).

Do we still require #2 as an intermediate step between the original
sequence, and the intended new sequence? Wouldn't having the sequence
change in 2 half-steps add to the confusion (should some driver break
due to either of the refactorings)?

> #3 Move crtc_enable() calls:
>   - encoder_bridge_pre_enable(), loops over encoders and calls
>     pre_enable(bridges),
>   - crtc_enable()
>   - encoder_bridge_enable(), loops over encoders and calls
>     enable(encoder), enable(bridges)
> 
> You might use enum or booleans to implement encoder_bridge_pre_enable(),
> or just keep it as a completely separate function (might be a better
> option).

Yeah, I suppose a separate function may be better. There will, however,
be some code duplication when we loop over the encoder twice, once for
pre_enable(bridge) and the other for enable(encoder) and enable(bridge).

I hope that will be acceptable?

> 
> The reason why I'm suggesting it is pretty easy: your patch performs two
> refactorings at the same time. If one of the drivers breaks, we have no
> way to identify, which of the refactorings caused the break.>
>>
>> This wouldn't be all that much trivial as I had imagined it to be earlier.
>>
>> It would also *kind of* like these patches in a previous revision,
>> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
>>
>> 1) these older patches separated out only the bridge/encoder operations
>> into a function, and not the crtc operations, and
>>
>> 2) An enum is being used instead of the booleans.
>>
>>
>> I believe this is what you are looking for? If I have misunderstood
>> something, do let me know.
>>
>>
>> Regards
>> Aradhya
>>
>>
>> [0]: v5:11/13
>> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
>> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
>>
>> [1]: v5:12/13
>> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
>>
> 


Regards
Aradhya

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

[Dmitry Baryshkov]

On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
> 
> On 20/01/25 14:08, Dmitry Baryshkov wrote:
> > On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
> >> Hi Dmitry,
> >>
> >> On 14/01/25 16:54, Dmitry Baryshkov wrote:
> >>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
> >>>>
> >>>> 	bridge[n]_disable
> >>>> 	...
> >>>> 	bridge[1]_disable
> >>>>
> >>>> 	encoder_disable
> >>>> 	crtc_disable
> >>>>
> >>>> 	bridge[1]_post_disable
> >>>> 	...
> >>>> 	bridge[n]_post_disable
> >>>>
> >>>> 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.
> >>>
> >>> The patch contains both refactoring of the corresponding functions and
> >>> changing of the order. Please split it into two separate commits.
> >>>
> >>
> >> I assume that you already understand this, so this is just for the
> >> record -
> >>
> >> There is no trivial way to do this. Initially, this is what I had in my
> >> mind - about what the split patches would look like.
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * Separate out the pre_enable and enable operations inside the
> >>    encoder_bridge_enable().
> >>  * call them in their (seemingly) original sequences
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> #2
> >>  * rearrange the calls to re-order the operation
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> This would have made the patch#2 seem quite trivial, as patch#1 would
> >> take the brunt of changes, while keeping the functionality intact.
> >>
> >>
> >> What I have now realized is that, the above isn't possible,
> >> unfortunately. The moment we separate out pre_enable and enable into 2
> >> different calls, the overall sequence gets even changed when there are
> >> multiple pipelines in the system.
> >>
> >> So to implement the split properly, the first patch would look like this
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * The calling sequences will be as follows -
> >>  	- crtc_enable()
> >> 	- encoder_bridge_enable()
> >> 		- this will do both pre_enable and enable
> >> 		  unconditionally.
> >>
> >> The pre_enable and enable operations wouldn't be separated in patch 1,
> >> just that the crtc enable and encoder bridge pre_enable/enable loops
> >> would be put into individual functions.
> >>
> >> The next patch will then introduce booleans, and separate out pre_enable
> >> and enable, and implement the new order -
> >>
> >> #2
> >>  * pre_enable and enable operations will be conditionally segregated
> >>    inside encoder_bridge_enable(), based on the pre_enable boolean.
> >>  * The calling sequences will then be updated to -
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable()
> >> 	- encoder_bridge_enable(!pre_enable)
> > 
> > 
> > I'd say slightly differently:
> > 
> > #1 Refactor loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     pre_enable(bridges), enable(encoder), enable(bridges)
> > 
> > #2 Split loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> 
> When we consider setups with multiple independent displays, there are
> often multiple crtcs in the system, each with their own encoder-bridge
> chain.
> 
> In such a scenario, the sequence of crtc/encoder/bridge enable calls
> after the #2 that you suggested, will not the remain same as that after
> #1 (which is the _original_ sequence of calls).

Yes. The order of ops between display has changed, but each display is
still programmed in exactly the same way as before.

> 
> Do we still require #2 as an intermediate step between the original
> sequence, and the intended new sequence? Wouldn't having the sequence
> change in 2 half-steps add to the confusion (should some driver break
> due to either of the refactorings)?

That's the point. Having two refactorings in one commit makes it harder
to understand, which one broke the driver. Having two separate commits
allows users to revert the latter commit and check what caused the
issue.

> 
> > #3 Move crtc_enable() calls:
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> > You might use enum or booleans to implement encoder_bridge_pre_enable(),
> > or just keep it as a completely separate function (might be a better
> > option).
> 
> Yeah, I suppose a separate function may be better. There will, however,
> be some code duplication when we loop over the encoder twice, once for
> pre_enable(bridge) and the other for enable(encoder) and enable(bridge).
> 
> I hope that will be acceptable?

I'd prefer two separate functions, but I won't insist on that.

> 
> > 
> > The reason why I'm suggesting it is pretty easy: your patch performs two
> > refactorings at the same time. If one of the drivers breaks, we have no
> > way to identify, which of the refactorings caused the break.>
> >>
> >> This wouldn't be all that much trivial as I had imagined it to be earlier.
> >>
> >> It would also *kind of* like these patches in a previous revision,
> >> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
> >>
> >> 1) these older patches separated out only the bridge/encoder operations
> >> into a function, and not the crtc operations, and
> >>
> >> 2) An enum is being used instead of the booleans.
> >>
> >>
> >> I believe this is what you are looking for? If I have misunderstood
> >> something, do let me know.
> >>
> >>
> >> Regards
> >> Aradhya
> >>
> >>
> >> [0]: v5:11/13
> >> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
> >> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
> >>
> >> [1]: v5:12/13
> >> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
> >> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
> >>
> > 
> 
> 
> Regards
> Aradhya
> 

-- 
With best wishes
Dmitry

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

[Aradhya Bhatia]

Hi Dmitry,

On 21/01/25 16:20, Dmitry Baryshkov wrote:
> On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote:
>> Hi Dmitry,
>>
>> On 20/01/25 14:08, Dmitry Baryshkov wrote:
>>> On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 14/01/25 16:54, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, 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, the disable sequence for the display pipeline will look like:
>>>>>>
>>>>>> 	bridge[n]_disable
>>>>>> 	...
>>>>>> 	bridge[1]_disable
>>>>>>
>>>>>> 	encoder_disable
>>>>>> 	crtc_disable
>>>>>>
>>>>>> 	bridge[1]_post_disable
>>>>>> 	...
>>>>>> 	bridge[n]_post_disable
>>>>>>
>>>>>> 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.
>>>>>
>>>>> The patch contains both refactoring of the corresponding functions and
>>>>> changing of the order. Please split it into two separate commits.
>>>>>
>>>>
>>>> I assume that you already understand this, so this is just for the
>>>> record -
>>>>
>>>> There is no trivial way to do this. Initially, this is what I had in my
>>>> mind - about what the split patches would look like.
>>>>
>>>> #1
>>>>  * refactoring of entire loops into separate functions.
>>>>  * Separate out the pre_enable and enable operations inside the
>>>>    encoder_bridge_enable().
>>>>  * call them in their (seemingly) original sequences
>>>> 	- crtc_enable
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>>
>>>> #2
>>>>  * rearrange the calls to re-order the operation
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- crtc_enable
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>>
>>>> This would have made the patch#2 seem quite trivial, as patch#1 would
>>>> take the brunt of changes, while keeping the functionality intact.
>>>>
>>>>
>>>> What I have now realized is that, the above isn't possible,
>>>> unfortunately. The moment we separate out pre_enable and enable into 2
>>>> different calls, the overall sequence gets even changed when there are
>>>> multiple pipelines in the system.
>>>>
>>>> So to implement the split properly, the first patch would look like this
>>>>
>>>> #1
>>>>  * refactoring of entire loops into separate functions.
>>>>  * The calling sequences will be as follows -
>>>>  	- crtc_enable()
>>>> 	- encoder_bridge_enable()
>>>> 		- this will do both pre_enable and enable
>>>> 		  unconditionally.
>>>>
>>>> The pre_enable and enable operations wouldn't be separated in patch 1,
>>>> just that the crtc enable and encoder bridge pre_enable/enable loops
>>>> would be put into individual functions.
>>>>
>>>> The next patch will then introduce booleans, and separate out pre_enable
>>>> and enable, and implement the new order -
>>>>
>>>> #2
>>>>  * pre_enable and enable operations will be conditionally segregated
>>>>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>>>>  * The calling sequences will then be updated to -
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- crtc_enable()
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>
>>>
>>> I'd say slightly differently:
>>>
>>> #1 Refactor loops into separate functions:
>>>   - crtc_enable()
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     pre_enable(bridges), enable(encoder), enable(bridges)
>>>
>>> #2 Split loops into separate functions:
>>>   - crtc_enable()
>>>   - encoder_bridge_pre_enable(), loops over encoders and calls
>>>     pre_enable(bridges),
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     enable(encoder), enable(bridges)
>>>
>>
>> When we consider setups with multiple independent displays, there are
>> often multiple crtcs in the system, each with their own encoder-bridge
>> chain.
>>
>> In such a scenario, the sequence of crtc/encoder/bridge enable calls
>> after the #2 that you suggested, will not the remain same as that after
>> #1 (which is the _original_ sequence of calls).
> 
> Yes. The order of ops between display has changed, but each display is
> still programmed in exactly the same way as before.

Yes, that's right. Sequence for each display will remain the same as
before.

> 
>>
>> Do we still require #2 as an intermediate step between the original
>> sequence, and the intended new sequence? Wouldn't having the sequence
>> change in 2 half-steps add to the confusion (should some driver break
>> due to either of the refactorings)?
> 
> That's the point. Having two refactorings in one commit makes it harder
> to understand, which one broke the driver. Having two separate commits
> allows users to revert the latter commit and check what caused the
> issue.

Right. It's easier to debug each display independently in multi-display
setups, and I can now understand how #2 will be able to help.

This explanation helped a lot. Thank you!

>>
>>> #3 Move crtc_enable() calls:
>>>   - encoder_bridge_pre_enable(), loops over encoders and calls
>>>     pre_enable(bridges),
>>>   - crtc_enable()
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     enable(encoder), enable(bridges)
>>>
>>> You might use enum or booleans to implement encoder_bridge_pre_enable(),
>>> or just keep it as a completely separate function (might be a better
>>> option).
>>
>> Yeah, I suppose a separate function may be better. There will, however,
>> be some code duplication when we loop over the encoder twice, once for
>> pre_enable(bridge) and the other for enable(encoder) and enable(bridge).
>>
>> I hope that will be acceptable?
> 
> I'd prefer two separate functions, but I won't insist on that.

Alright!

I have my work cut out for me for the next revision.

> 
>>
>>>
>>> The reason why I'm suggesting it is pretty easy: your patch performs two
>>> refactorings at the same time. If one of the drivers breaks, we have no
>>> way to identify, which of the refactorings caused the break.>
>>>>
>>>> This wouldn't be all that much trivial as I had imagined it to be earlier.
>>>>
>>>> It would also *kind of* like these patches in a previous revision,
>>>> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
>>>>
>>>> 1) these older patches separated out only the bridge/encoder operations
>>>> into a function, and not the crtc operations, and
>>>>
>>>> 2) An enum is being used instead of the booleans.
>>>>
>>>>
>>>> I believe this is what you are looking for? If I have misunderstood
>>>> something, do let me know.
>>>>
>>>>
>>>> Regards
>>>> Aradhya
>>>>
>>>>
>>>> [0]: v5:11/13
>>>> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
>>>> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
>>>>
>>>> [1]: v5:12/13
>>>> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>>>> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
>>>>
>>>
>>
>>
>> Regards
>> Aradhya
>>
> 

--
Regards
Aradhya