drm/bridge: cdns-dsi: Fix the color-shift issue

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -0

index 056583e81155..039c5eb7fb66 100644

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -1

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

Hi,

Quoted 26 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-

  1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -0

index 056583e81155..039c5eb7fb66 100644

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c static void cdns_dsi_bridge_disable(struct · line 679

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +4 -0

@@ -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.

Quoted 12 lines

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -0

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

Hi Tomi,

Quoted 41 lines · Tomi Valkeinen

Hi,

Quoted 26 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-
   1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -0

index 056583e81155..039c5eb7fb66 100644

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c static void cdns_dsi_bridge_disable(struct · line 679

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +4 -0

@@ -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(),

Quoted 4 lines

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)

Quoted 4 lines

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.

Hi,

Quoted 51 lines · Aradhya Bhatia

Hi Tomi,

Quoted 43 lines · Tomi Valkeinen

Hi,

Quoted 28 lines · Aradhya Bhatia

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 6 +++++-

   1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +0 -0

index 056583e81155..039c5eb7fb66 100644

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c static void cdns_dsi_bridge_disable(struct · line 679

Quoted 8 lines

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +4 -0

@@ -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(),

Quoted 4 lines

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?

Quoted 6 lines

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.

Quoted 3 lines

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 =).

Quoted 3 lines

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.

Quoted 7 lines

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).

Quoted 2 lines

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?

Quoted 10 lines

(one of the potential concerns detailed below)

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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()! =)

Hi,

Quoted 12 lines · Aradhya Bhatia

Quoted 4 lines

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 -

Quoted 6 lines

    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().

Quoted 3 lines

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.

Quoted 2 lines

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.

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

From: Aradhya Bhatia <[email protected]>

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 87 +++++++++++++++++--
  1 file changed, 82 insertions(+), 5 deletions(-)

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +21 -2

index b8db984ea9fa..d60254e1270c 100644
@@ -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;
  }

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +8 -0

+	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)
+{

drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +29 -0

+	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,

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 <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
 drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
 1 file changed, 181 insertions(+), 119 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 5186d2114a50..ad6290a4a528 100644

drivers/gpu/drm/drm_atomic_helper.c +1 -63

 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;

drivers/gpu/drm/drm_atomic_helper.c +174 -55

 		/* 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);
 }

Signature · 1 line

2.34.1

Quoted 29 lines · 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

Quoted 7 lines

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.

Quoted 17 lines

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------
  1 file changed, 181 insertions(+), 119 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +2 -0

index 5186d2114a50..ad6290a4a528 100644
@@ -74,6 +74,12 @@
   * also shares the &struct drm_plane_helper_funcs function table with the plane
   * helpers.
   */
+
+enum bridge_chain_operation_type {

drivers/gpu/drm/drm_atomic_helper.c line 82

Quoted 4 lines

drivers/gpu/drm/drm_atomic_helper.c +4 -0

+	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?

Hi,

Quoted 60 lines · Dmitry Baryshkov

Quoted 29 lines · 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

Quoted 7 lines

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.

Quoted 19 lines

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------

  1 file changed, 181 insertions(+), 119 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 5186d2114a50..ad6290a4a528 100644
@@ -74,6 +74,12 @@
    * also shares the &struct drm_plane_helper_funcs function table

with the plane
   * helpers.
   */

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+
+enum bridge_chain_operation_type {

Quoted 8 lines

drivers/gpu/drm/drm_atomic_helper.c line 82

Quoted 4 lines

+    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
+    DRM_BRIDGE_ENABLE_OR_DISABLE,

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+};
+

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.

Quoted 77 lines · Tomi Valkeinen

Hi,

Quoted 60 lines · Dmitry Baryshkov

Quoted 29 lines · 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

Quoted 7 lines

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.

Quoted 19 lines

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------

  1 file changed, 181 insertions(+), 119 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 5186d2114a50..ad6290a4a528 100644
@@ -74,6 +74,12 @@
    * also shares the &struct drm_plane_helper_funcs function table

with the plane
   * helpers.
   */

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+
+enum bridge_chain_operation_type {

Quoted 8 lines

drivers/gpu/drm/drm_atomic_helper.c line 82

Quoted 4 lines

+    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
+    DRM_BRIDGE_ENABLE_OR_DISABLE,

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+};
+

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.

Quoted 3 lines

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.

Quoted 84 lines · Aradhya Bhatia

Quoted 80 lines · Tomi Valkeinen

Hi,

Quoted 60 lines · Dmitry Baryshkov

Quoted 29 lines · 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

Quoted 7 lines

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.

Quoted 19 lines

Signed-off-by Aradhya Bhatia <[email protected]>

Signed-off-by Aradhya Bhatia <[email protected]>

---
  drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++-----------

  1 file changed, 181 insertions(+), 119 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +0 -0

index 5186d2114a50..ad6290a4a528 100644
@@ -74,6 +74,12 @@
    * also shares the &struct drm_plane_helper_funcs function table

with the plane
   * helpers.
   */

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+
+enum bridge_chain_operation_type {

Quoted 8 lines

drivers/gpu/drm/drm_atomic_helper.c line 82

Quoted 4 lines

+    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
+    DRM_BRIDGE_ENABLE_OR_DISABLE,

drivers/gpu/drm/drm_atomic_helper.c +2 -0

+};
+

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.

Quoted 2 lines

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.

Quoted 33 lines · 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".

Quoted 3 lines

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.

Hi Dmitry,

Quoted 34 lines · Dmitry Baryshkov

Quoted 33 lines · 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".

Quoted 7 lines

Quoted 3 lines

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/[email protected]/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/[email protected]/

Quoted 92 lines · Aradhya Bhatia

Hi Dmitry,

Quoted 34 lines · Dmitry Baryshkov

Quoted 33 lines · 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".

Quoted 7 lines

Quoted 3 lines

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 -

Quoted 7 lines

#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.>

Quoted 28 lines

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/[email protected]/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/[email protected]/

Hi Dmitry,

Quoted 109 lines · Dmitry Baryshkov

Quoted 92 lines · Aradhya Bhatia

Hi Dmitry,

Quoted 34 lines · Dmitry Baryshkov

Quoted 33 lines · 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".

Quoted 7 lines

Quoted 3 lines

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 -

Quoted 7 lines

#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)

Quoted 7 lines

#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)?

Quoted 11 lines

#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?

Quoted 34 lines

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.>

Quoted 28 lines

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/[email protected]/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/[email protected]/

Regards
Aradhya

Quoted 124 lines · Aradhya Bhatia

Hi Dmitry,

Quoted 109 lines · Dmitry Baryshkov

Quoted 92 lines · Aradhya Bhatia

Hi Dmitry,

Quoted 34 lines · Dmitry Baryshkov

Quoted 33 lines · 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".

Quoted 7 lines

Quoted 3 lines

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 -

Quoted 7 lines

#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)

Quoted 7 lines

#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.

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 12 lines

Quoted 11 lines

#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).

Quoted 5 lines

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.

Quoted 37 lines

Quoted 35 lines

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.>

Quoted 28 lines

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/[email protected]/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/[email protected]/

Quoted 3 lines

Regards
Aradhya