[PATCH 0/4] Enable fastset for mbus_join state change

[PATCH 0/4] Enable fastset for mbus_join state change

[Stanislav Lisovskiy]

Currently fastset is not supported, if mbus join state changes,
so whenever we have to switch mbus state, we have to force a full
modeset. This patch series makes fastset possible from MBUS state
point of view.

Stanislav Lisovskiy (4):
  drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly
  drm/i915: Use old mbus_join value when increasing CDCLK
  drm/i915: Loop over all active pipes in intel_mbus_dbox_update
  drm/i915: Implement vblank synchronized MBUS join changes

 drivers/gpu/drm/i915/display/intel_cdclk.c   |  12 +-
 drivers/gpu/drm/i915/display/intel_display.c |   6 +-
 drivers/gpu/drm/i915/display/skl_watermark.c | 166 ++++++++++++++-----
 drivers/gpu/drm/i915/display/skl_watermark.h |   2 +
 4 files changed, 141 insertions(+), 45 deletions(-)

-- 
2.37.3

[PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Stanislav Lisovskiy]

According to BSpec we need to do correspondent MBUS updates before
or after DBUF reallocation, depending on whether we are enabling
or disabling mbus joining(typical scenario is swithing between
multiple and single displays).

Also we need to be able to update dbuf min tracker and mdclk ratio
separately if mbus_join state didn't change, so lets add one
degree of freedom and make it possible.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index bc341abcab2fe..2b947870527fc 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
 			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
 }
 
+static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_dbuf_state *old_dbuf_state =
+		intel_atomic_get_old_dbuf_state(state);
+	const struct intel_dbuf_state *new_dbuf_state =
+		intel_atomic_get_new_dbuf_state(state);
+
+	if (DISPLAY_VER(i915) >= 20 &&
+	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
+		/*
+		 * For Xe2LPD and beyond, when there is a change in the ratio
+		 * between MDCLK and CDCLK, updates to related registers need to
+		 * happen at a specific point in the CDCLK change sequence. In
+		 * that case, we defer to the call to
+		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
+		 */
+		return;
+	}
+
+	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
+					    new_dbuf_state->joined_mbus);
+}
+
 /*
  * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
  * update the request state of all DBUS slices.
  */
-static void update_mbus_pre_enable(struct intel_atomic_state *state)
+static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
 	u32 mbus_ctl;
-	const struct intel_dbuf_state *old_dbuf_state =
-		intel_atomic_get_old_dbuf_state(state);
 	const struct intel_dbuf_state *new_dbuf_state =
 		intel_atomic_get_new_dbuf_state(state);
 
@@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
 	intel_de_rmw(i915, MBUS_CTL,
 		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
 		     MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
-
-	if (DISPLAY_VER(i915) >= 20 &&
-	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
-		/*
-		 * For Xe2LPD and beyond, when there is a change in the ratio
-		 * between MDCLK and CDCLK, updates to related registers need to
-		 * happen at a specific point in the CDCLK change sequence. In
-		 * that case, we defer to the call to
-		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
-		 */
-		return;
-	}
-
-	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
-					    new_dbuf_state->joined_mbus);
 }
 
 void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
@@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
 
 	WARN_ON(!new_dbuf_state->base.changed);
 
-	update_mbus_pre_enable(state);
+	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
+		intel_dbuf_mbus_join_update(state);
+		intel_dbuf_mdclk_min_tracker_update(state);
+	}
+
 	gen9_dbuf_slices_update(i915,
 				old_dbuf_state->enabled_slices |
 				new_dbuf_state->enabled_slices);
@@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
 
 	WARN_ON(!new_dbuf_state->base.changed);
 
+	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
+		intel_dbuf_mbus_join_update(state);
+		intel_dbuf_mdclk_min_tracker_update(state);
+	}
+
 	gen9_dbuf_slices_update(i915,
 				new_dbuf_state->enabled_slices);
 }
-- 
2.37.3

[PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Stanislav Lisovskiy]

In order to make sure we are not breaking the proper sequence
lets to updates step by step and don't change MBUS join value
during MDCLK/CDCLK programming stage.
MBUS join programming would be taken care by pre/post ddb hooks.

v2: - Reworded comment about using old mbus_join value in
      intel_set_cdclk(Ville Syrjälä)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 31aaa9780dfcf..c7813d433c424 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
 
 	if (pipe == INVALID_PIPE ||
 	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
+		struct intel_cdclk_config cdclk_config;
+
 		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
 
-		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
+		/*
+		 * By this hack we want to prevent mbus_join to be changed
+		 * beforehand - we will take care of this later in
+		 * intel_dbuf_mbus_post_ddb_update
+		 */
+		cdclk_config = new_cdclk_state->actual;
+		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
+
+		intel_set_cdclk(i915, &cdclk_config, pipe);
 	}
 }
 
-- 
2.37.3

[PATCH 3/4] drm/i915: Loop over all active pipes in intel_mbus_dbox_update

[Stanislav Lisovskiy]

We need to loop through all active pipes, not just the ones, that
are in current state, because disabling and enabling even a particular
pipe affects credits in another one.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/skl_watermark.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 2b947870527fc..7eb78e0c8c8e3 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -3696,10 +3696,8 @@ void intel_mbus_dbox_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
 	const struct intel_dbuf_state *new_dbuf_state, *old_dbuf_state;
-	const struct intel_crtc_state *new_crtc_state;
 	const struct intel_crtc *crtc;
 	u32 val = 0;
-	int i;
 
 	if (DISPLAY_VER(i915) < 11)
 		return;
@@ -3743,12 +3741,9 @@ void intel_mbus_dbox_update(struct intel_atomic_state *state)
 		val |= MBUS_DBOX_B_CREDIT(8);
 	}
 
-	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+	for_each_intel_crtc_in_pipe_mask(&i915->drm, crtc, new_dbuf_state->active_pipes) {
 		u32 pipe_val = val;
 
-		if (!new_crtc_state->hw.active)
-			continue;
-
 		if (DISPLAY_VER(i915) >= 14) {
 			if (xelpdp_is_only_pipe_per_dbuf_bank(crtc->pipe,
 							      new_dbuf_state->active_pipes))
-- 
2.37.3

[PATCH 4/4] drm/i915: Implement vblank synchronized MBUS join changes

[Stanislav Lisovskiy]

Currently we can't change MBUS join status without doing a modeset,
because we are lacking mechanism to synchronize those with vblank.
However then this means that we can't do a fastset, if there is a need
to change MBUS join state. Fix that by implementing such change.
We already call correspondent check and update at pre_plane dbuf update,
so the only thing left is to have a non-modeset version of that.
If active pipes stay the same then fastset is possible and only MBUS
join state/ddb allocation updates would be committed.

v2: - Removed redundant parentheses(Ville Syrjälä)
    - Constified new_crtc_state in intel_mbus_joined_pipe(Ville Syrjälä)
    - Removed pipe_select variable(Ville Syrjälä)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |   6 +-
 drivers/gpu/drm/i915/display/skl_watermark.c | 119 +++++++++++++++----
 drivers/gpu/drm/i915/display/skl_watermark.h |   2 +
 3 files changed, 101 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b88f214e111ae..d5351f6fa2eb4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6895,6 +6895,8 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 		intel_pre_update_crtc(state, crtc);
 	}
 
+	intel_dbuf_mbus_pre_ddb_update(state);
+
 	while (update_pipes) {
 		for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 						    new_crtc_state, i) {
@@ -6925,6 +6927,8 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 		}
 	}
 
+	intel_dbuf_mbus_post_ddb_update(state);
+
 	update_pipes = modeset_pipes;
 
 	/*
@@ -7169,9 +7173,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	}
 
 	intel_encoders_update_prepare(state);
-
 	intel_dbuf_pre_plane_update(state);
-	intel_mbus_dbox_update(state);
 
 	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
 		if (new_crtc_state->do_async_flip)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 7eb78e0c8c8e3..7f2b4dc2a27d4 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -4,6 +4,7 @@
  */
 
 #include <drm/drm_blend.h>
+#include <drm/drm_print.h>
 
 #include "i915_drv.h"
 #include "i915_fixed.h"
@@ -2636,13 +2637,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
 		if (ret)
 			return ret;
 
-		if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
-			/* TODO: Implement vblank synchronized MBUS joining changes */
-			ret = intel_modeset_all_pipes_late(state, "MBUS joining change");
-			if (ret)
-				return ret;
-		}
-
 		drm_dbg_kms(&i915->drm,
 			    "Enabled dbuf slices 0x%x -> 0x%x (total dbuf slices 0x%x), mbus joined? %s->%s\n",
 			    old_dbuf_state->enabled_slices,
@@ -3594,11 +3588,32 @@ static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state
 					    new_dbuf_state->joined_mbus);
 }
 
+static enum pipe intel_mbus_joined_pipe(struct intel_atomic_state *state,
+				        const struct intel_dbuf_state *dbuf_state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	enum pipe sync_pipe = ffs(dbuf_state->active_pipes) - 1;
+	const struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+
+	drm_WARN_ON(&i915->drm, !dbuf_state->joined_mbus);
+	drm_WARN_ON(&i915->drm, !is_power_of_2(dbuf_state->active_pipes));
+
+	crtc = intel_crtc_for_pipe(i915, sync_pipe);
+	new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+
+	if (new_crtc_state && !intel_crtc_needs_modeset(new_crtc_state))
+		return sync_pipe;
+	else
+		return INVALID_PIPE;
+}
+
 /*
  * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
  * update the request state of all DBUS slices.
  */
-static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
+static void intel_dbuf_mbus_ctl_update(struct intel_atomic_state *state,
+				       enum pipe sync_pipe)
 {
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
 	u32 mbus_ctl;
@@ -3612,12 +3627,21 @@ static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
 	 * TODO: Implement vblank synchronized MBUS joining changes.
 	 * Must be properly coordinated with dbuf reprogramming.
 	 */
-	if (new_dbuf_state->joined_mbus)
-		mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
-			MBUS_JOIN_PIPE_SELECT_NONE;
-	else
-		mbus_ctl = MBUS_HASHING_MODE_2x2 |
-			MBUS_JOIN_PIPE_SELECT_NONE;
+	if (new_dbuf_state->joined_mbus) {
+		if (sync_pipe != INVALID_PIPE)
+			mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
+				   MBUS_JOIN_PIPE_SELECT(sync_pipe);
+		else
+			mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
+				   MBUS_JOIN_PIPE_SELECT_NONE;
+	} else {
+		if (sync_pipe != INVALID_PIPE)
+			mbus_ctl = MBUS_HASHING_MODE_2x2 |
+				   MBUS_JOIN_PIPE_SELECT(sync_pipe);
+		else
+			mbus_ctl = MBUS_HASHING_MODE_2x2 |
+				   MBUS_JOIN_PIPE_SELECT_NONE;
+	}
 
 	intel_de_rmw(i915, MBUS_CTL,
 		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
@@ -3632,6 +3656,42 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
 	const struct intel_dbuf_state *old_dbuf_state =
 		intel_atomic_get_old_dbuf_state(state);
 
+	if (!new_dbuf_state ||
+	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
+		return;
+
+	WARN_ON(!new_dbuf_state->base.changed);
+
+	gen9_dbuf_slices_update(i915,
+				old_dbuf_state->enabled_slices |
+				new_dbuf_state->enabled_slices);
+}
+
+void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_dbuf_state *new_dbuf_state =
+		intel_atomic_get_new_dbuf_state(state);
+	const struct intel_dbuf_state *old_dbuf_state =
+		intel_atomic_get_old_dbuf_state(state);
+
+	if (!new_dbuf_state ||
+	    new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices)
+		return;
+
+	WARN_ON(!new_dbuf_state->base.changed);
+
+	gen9_dbuf_slices_update(i915,
+				new_dbuf_state->enabled_slices);
+}
+
+void intel_dbuf_mbus_pre_ddb_update(struct intel_atomic_state *state)
+{
+	const struct intel_dbuf_state *new_dbuf_state =
+		intel_atomic_get_new_dbuf_state(state);
+	const struct intel_dbuf_state *old_dbuf_state =
+		intel_atomic_get_old_dbuf_state(state);
+
 	if (!new_dbuf_state ||
 	    (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
 	     new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
@@ -3640,16 +3700,15 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
 	WARN_ON(!new_dbuf_state->base.changed);
 
 	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
-		intel_dbuf_mbus_join_update(state);
+		enum pipe sync_pipe = intel_mbus_joined_pipe(state, new_dbuf_state);
+
+		intel_dbuf_mbus_ctl_update(state, sync_pipe);
+		intel_mbus_dbox_update(state);
 		intel_dbuf_mdclk_min_tracker_update(state);
 	}
-
-	gen9_dbuf_slices_update(i915,
-				old_dbuf_state->enabled_slices |
-				new_dbuf_state->enabled_slices);
 }
 
-void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
+void intel_dbuf_mbus_post_ddb_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
 	const struct intel_dbuf_state *new_dbuf_state =
@@ -3657,6 +3716,12 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
 	const struct intel_dbuf_state *old_dbuf_state =
 		intel_atomic_get_old_dbuf_state(state);
 
+	if (new_dbuf_state && old_dbuf_state &&
+	    new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus) {
+		intel_dbuf_mdclk_min_tracker_update(state);
+		intel_mbus_dbox_update(state);
+	}
+
 	if (!new_dbuf_state ||
 	    (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
 	     new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
@@ -3665,12 +3730,18 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
 	WARN_ON(!new_dbuf_state->base.changed);
 
 	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
-		intel_dbuf_mbus_join_update(state);
+		enum pipe sync_pipe = intel_mbus_joined_pipe(state, old_dbuf_state);
+
 		intel_dbuf_mdclk_min_tracker_update(state);
-	}
+		intel_mbus_dbox_update(state);
+		intel_dbuf_mbus_ctl_update(state, sync_pipe);
 
-	gen9_dbuf_slices_update(i915,
-				new_dbuf_state->enabled_slices);
+		if (sync_pipe != INVALID_PIPE) {
+			struct intel_crtc *crtc = intel_crtc_for_pipe(i915, sync_pipe);
+
+			intel_crtc_wait_for_next_vblank(crtc);
+		}
+	}
 }
 
 static bool xelpdp_is_only_pipe_per_dbuf_bank(enum pipe pipe, u8 active_pipes)
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
index 3a90741cab06a..f6d38b41e3a6c 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.h
+++ b/drivers/gpu/drm/i915/display/skl_watermark.h
@@ -77,6 +77,8 @@ int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8
 void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
 void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
 void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus);
+void intel_dbuf_mbus_pre_ddb_update(struct intel_atomic_state *state);
+void intel_dbuf_mbus_post_ddb_update(struct intel_atomic_state *state);
 void intel_mbus_dbox_update(struct intel_atomic_state *state);
 
 #endif /* __SKL_WATERMARK_H__ */
-- 
2.37.3

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> In order to make sure we are not breaking the proper sequence
> lets to updates step by step and don't change MBUS join value
> during MDCLK/CDCLK programming stage.
> MBUS join programming would be taken care by pre/post ddb hooks.
> 
> v2: - Reworded comment about using old mbus_join value in
>       intel_set_cdclk(Ville Syrjälä)
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfcf..c7813d433c424 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>  
>  	if (pipe == INVALID_PIPE ||
>  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +		struct intel_cdclk_config cdclk_config;
> +
>  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>  
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		/*
> +		 * By this hack we want to prevent mbus_join to be changed
> +		 * beforehand

That sentence is still confusing.

> - we will take care of this later in
> +		 * intel_dbuf_mbus_post_ddb_update
> +		 */
> +		cdclk_config = new_cdclk_state->actual;
> +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> +
> +		intel_set_cdclk(i915, &cdclk_config, pipe);
>  	}
>  }
>  
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> According to BSpec we need to do correspondent MBUS updates before
> or after DBUF reallocation, depending on whether we are enabling
> or disabling mbus joining(typical scenario is swithing between
> multiple and single displays).
> 
> Also we need to be able to update dbuf min tracker and mdclk ratio
> separately if mbus_join state didn't change, so lets add one
> degree of freedom and make it possible.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index bc341abcab2fe..2b947870527fc 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
>  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
>  }
>  
> +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	const struct intel_dbuf_state *old_dbuf_state =
> +		intel_atomic_get_old_dbuf_state(state);
> +	const struct intel_dbuf_state *new_dbuf_state =
> +		intel_atomic_get_new_dbuf_state(state);
> +
> +	if (DISPLAY_VER(i915) >= 20 &&
> +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> +		/*
> +		 * For Xe2LPD and beyond, when there is a change in the ratio
> +		 * between MDCLK and CDCLK, updates to related registers need to
> +		 * happen at a specific point in the CDCLK change sequence. In
> +		 * that case, we defer to the call to
> +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> +		 */
> +		return;
> +	}

That still needs to be removed or else we'll not update the ratio at
all during the mbus_join changes. I don't think I saw any removal
in subsequent patches.

> +
> +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,

And it just occurred to me that this thing will in fact be wrong
during the pre/post ddb hooks *and* cdclk is getting decreased
from the post plane update hook.

I can't immediately think of a super nice way to handle this.

Perhaps the most stragithforward idea is to just get the mdclk/cdclk
ratio from i915->display.cdclk.hw during the pre/post ddb hooks.
cdclk serialization should guard against parallel updates from
two both places and thus isplay.cdclk.hw should be safe to use.

The other option would be to determine if a cdclk decrease
is going to happen or not, and depending on that use the
old vs. new dbuf_state when updating the ratio in the
pre/post ddb hooks.

> +					    new_dbuf_state->joined_mbus);
> +}
> +
>  /*
>   * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
>   * update the request state of all DBUS slices.
>   */
> -static void update_mbus_pre_enable(struct intel_atomic_state *state)
> +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *i915 = to_i915(state->base.dev);
>  	u32 mbus_ctl;
> -	const struct intel_dbuf_state *old_dbuf_state =
> -		intel_atomic_get_old_dbuf_state(state);
>  	const struct intel_dbuf_state *new_dbuf_state =
>  		intel_atomic_get_new_dbuf_state(state);
>  
> @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
>  	intel_de_rmw(i915, MBUS_CTL,
>  		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
>  		     MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> -
> -	if (DISPLAY_VER(i915) >= 20 &&
> -	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> -		/*
> -		 * For Xe2LPD and beyond, when there is a change in the ratio
> -		 * between MDCLK and CDCLK, updates to related registers need to
> -		 * happen at a specific point in the CDCLK change sequence. In
> -		 * that case, we defer to the call to
> -		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> -		 */
> -		return;
> -	}
> -
> -	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> -					    new_dbuf_state->joined_mbus);
>  }
>  
>  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
>  
>  	WARN_ON(!new_dbuf_state->base.changed);
>  
> -	update_mbus_pre_enable(state);
> +	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {

I think you squashed that stuff into the wrong patch.
This one should have a pure refactoring patch.

> +		intel_dbuf_mbus_join_update(state);
> +		intel_dbuf_mdclk_min_tracker_update(state);
> +	}
> +
>  	gen9_dbuf_slices_update(i915,
>  				old_dbuf_state->enabled_slices |
>  				new_dbuf_state->enabled_slices);
> @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
>  
>  	WARN_ON(!new_dbuf_state->base.changed);
>  
> +	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> +		intel_dbuf_mbus_join_update(state);
> +		intel_dbuf_mdclk_min_tracker_update(state);
> +	}
> +
>  	gen9_dbuf_slices_update(i915,
>  				new_dbuf_state->enabled_slices);
>  }
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

✗ Fi.CI.CHECKPATCH: warning for Enable fastset for mbus_join state change (rev4)

[Patchwork]

== Series Details ==

Series: Enable fastset for mbus_join state change (rev4)
URL   : https://patchwork.freedesktop.org/series/130480/
State : warning

== Summary ==

Error: dim checkpatch failed
bfc7f71c18ac drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly
-:12: WARNING:TYPO_SPELLING: 'swithing' may be misspelled - perhaps 'switching'?
#12: 
or disabling mbus joining(typical scenario is swithing between
                                              ^^^^^^^^

total: 0 errors, 1 warnings, 0 checks, 85 lines checked
3e60b23144b5 drm/i915: Use old mbus_join value when increasing CDCLK
2d8a015d22f3 drm/i915: Loop over all active pipes in intel_mbus_dbox_update
fc12aac791b6 drm/i915: Implement vblank synchronized MBUS join changes
-:87: ERROR:CODE_INDENT: code indent should use tabs where possible
#87: FILE: drivers/gpu/drm/i915/display/skl_watermark.c:3592:
+^I^I^I^I        const struct intel_dbuf_state *dbuf_state)$

-:87: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#87: FILE: drivers/gpu/drm/i915/display/skl_watermark.c:3592:
+static enum pipe intel_mbus_joined_pipe(struct intel_atomic_state *state,
+				        const struct intel_dbuf_state *dbuf_state)

total: 1 errors, 0 warnings, 1 checks, 210 lines checked

✗ Fi.CI.BAT: failure for Enable fastset for mbus_join state change (rev4)

[Patchwork]

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

== Series Details ==

Series: Enable fastset for mbus_join state change (rev4)
URL   : https://patchwork.freedesktop.org/series/130480/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14478 -> Patchwork_130480v4
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_130480v4 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_130480v4, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/index.html

Participating hosts (36 -> 38)
------------------------------

  Additional (4): bat-arls-4 bat-atsm-1 bat-rpls-3 bat-arls-3 
  Missing    (2): bat-mtlp-8 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_130480v4:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - bat-rpls-3:         NOTRUN -> [SKIP][1] +9 other tests skip
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  
Known issues
------------

  Here are the changes found in Patchwork_130480v4 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - bat-dg2-11:         [PASS][2] -> [FAIL][3] ([i915#10491])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14478/bat-dg2-11/boot.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-11/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-arls-3:         NOTRUN -> [SKIP][4] ([i915#9318])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@debugfs_test@basic-hwmon.html
    - bat-rpls-3:         NOTRUN -> [SKIP][5] ([i915#9318])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@info:
    - bat-rpls-3:         NOTRUN -> [SKIP][6] ([i915#1849] / [i915#2582])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@fbdev@info.html

  * igt@fbdev@write:
    - bat-rpls-3:         NOTRUN -> [SKIP][7] ([i915#2582]) +3 other tests skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@fbdev@write.html

  * igt@gem_huc_copy@huc-copy:
    - bat-atsm-1:         NOTRUN -> [FAIL][8] ([i915#10563])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-arls-3:         NOTRUN -> [SKIP][9] ([i915#10213]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@random-engines:
    - bat-rpls-3:         NOTRUN -> [SKIP][10] ([i915#4613]) +3 other tests skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_mmap@basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][11] ([i915#4083])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@gem_mmap@basic.html
    - bat-arls-3:         NOTRUN -> [SKIP][12] ([i915#4083])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][13] ([i915#10197] / [i915#10211] / [i915#4079])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_blits@basic:
    - bat-arls-3:         NOTRUN -> [SKIP][14] ([i915#10196] / [i915#4077]) +2 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@gem_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][15] ([i915#4079]) +1 other test skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@gem_tiled_pread_basic.html
    - bat-arls-3:         NOTRUN -> [SKIP][16] ([i915#10206] / [i915#4079])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@gem_tiled_pread_basic.html
    - bat-rpls-3:         NOTRUN -> [SKIP][17] ([i915#3282])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-atsm-1:         NOTRUN -> [SKIP][18] ([i915#6621])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@i915_pm_rps@basic-api.html
    - bat-dg2-9:          NOTRUN -> [SKIP][19] ([i915#6621])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@i915_pm_rps@basic-api.html
    - bat-arls-3:         NOTRUN -> [SKIP][20] ([i915#10209])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gem:
    - bat-atsm-1:         NOTRUN -> [ABORT][21] ([i915#10564])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@i915_selftest@live@gem.html

  * igt@i915_selftest@live@hangcheck:
    - bat-rpls-3:         NOTRUN -> [DMESG-WARN][22] ([i915#5591])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@i915_selftest@live@hangcheck.html

  * igt@kms_addfb_basic@addfb25-x-tiled-legacy:
    - bat-arls-3:         NOTRUN -> [SKIP][23] ([i915#10200]) +9 other tests skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_addfb_basic@addfb25-x-tiled-legacy.html

  * igt@kms_addfb_basic@size-max:
    - bat-atsm-1:         NOTRUN -> [SKIP][24] ([i915#6077]) +37 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_addfb_basic@size-max.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - bat-arls-3:         NOTRUN -> [SKIP][25] ([i915#10202]) +1 other test skip
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - bat-atsm-1:         NOTRUN -> [SKIP][26] ([i915#6078]) +22 other tests skip
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - bat-arls-3:         NOTRUN -> [SKIP][27] ([i915#9886])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_dsc@dsc-basic.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - bat-rpls-3:         NOTRUN -> [SKIP][28] ([i915#3637]) +3 other tests skip
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_flip@basic-flip-vs-dpms.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-arls-3:         NOTRUN -> [SKIP][29] ([i915#10207])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_force_connector_basic@force-load-detect.html
    - bat-dg2-9:          NOTRUN -> [SKIP][30]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html
    - bat-rpls-3:         NOTRUN -> [SKIP][31]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-atsm-1:         NOTRUN -> [SKIP][32] ([i915#6093]) +4 other tests skip
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_force_connector_basic@prune-stale-modes.html
    - bat-dg2-9:          NOTRUN -> [SKIP][33] ([i915#5274])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-rpls-3:         NOTRUN -> [SKIP][34] ([i915#1849] / [i915#5354])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24:
    - bat-atsm-1:         NOTRUN -> [SKIP][35] ([i915#1836]) +6 other tests skip
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24.html

  * igt@kms_pipe_crc_basic@hang-read-crc:
    - bat-dg2-9:          NOTRUN -> [SKIP][36] ([i915#9197]) +6 other tests skip
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_pipe_crc_basic@hang-read-crc.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-rpls-3:         NOTRUN -> [SKIP][37] ([i915#9920]) +6 other tests skip
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_pipe_crc_basic@read-crc.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-arls-3:         NOTRUN -> [SKIP][38] ([i915#9812])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_pm_backlight@basic-brightness.html
    - bat-rpls-3:         NOTRUN -> [SKIP][39] ([i915#5354])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_pm_backlight@basic-brightness.html
    - bat-dg2-9:          NOTRUN -> [SKIP][40] ([i915#5354]) +1 other test skip
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_prop_blob@basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][41] ([i915#7357])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_prop_blob@basic.html

  * igt@kms_psr@psr-cursor-plane-move:
    - bat-rpls-3:         NOTRUN -> [SKIP][42] ([i915#1072] / [i915#9673] / [i915#9732]) +3 other tests skip
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_psr@psr-cursor-plane-move.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-arls-3:         NOTRUN -> [SKIP][43] ([i915#9732]) +3 other tests skip
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - bat-dg2-9:          NOTRUN -> [SKIP][44] ([i915#1072] / [i915#9673] / [i915#9732]) +3 other tests skip
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-atsm-1:         NOTRUN -> [SKIP][45] ([i915#6094])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-dg2-9:          NOTRUN -> [SKIP][46] ([i915#3555])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-arls-3:         NOTRUN -> [SKIP][47] ([i915#10208] / [i915#8809])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-rpls-3:         NOTRUN -> [SKIP][48] ([i915#3555])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-9:          NOTRUN -> [SKIP][49] ([i915#3708] / [i915#9197])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html
    - bat-rpls-3:         NOTRUN -> [SKIP][50] ([i915#3708]) +3 other tests skip
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-rpls-3/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-atsm-1:         NOTRUN -> [SKIP][51] ([i915#4077]) +4 other tests skip
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@prime_vgem@basic-fence-mmap.html
    - bat-arls-3:         NOTRUN -> [SKIP][52] ([i915#10196] / [i915#3708] / [i915#4077]) +1 other test skip
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@prime_vgem@basic-fence-mmap.html
    - bat-dg2-9:          NOTRUN -> [SKIP][53] ([i915#3708] / [i915#4077]) +1 other test skip
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-arls-3:         NOTRUN -> [SKIP][54] ([i915#10212] / [i915#3708])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-read:
    - bat-arls-3:         NOTRUN -> [SKIP][55] ([i915#10214] / [i915#3708])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-atsm-1:         NOTRUN -> [SKIP][56] +2 other tests skip
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-atsm-1/igt@prime_vgem@basic-write.html
    - bat-dg2-9:          NOTRUN -> [SKIP][57] ([i915#3291] / [i915#3708]) +2 other tests skip
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-dg2-9/igt@prime_vgem@basic-write.html
    - bat-arls-3:         NOTRUN -> [SKIP][58] ([i915#10216] / [i915#3708])
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/bat-arls-3/igt@prime_vgem@basic-write.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
  [i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
  [i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
  [i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
  [i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
  [i915#10436]: https://gitlab.freedesktop.org/drm/intel/issues/10436
  [i915#10491]: https://gitlab.freedesktop.org/drm/intel/issues/10491
  [i915#10563]: https://gitlab.freedesktop.org/drm/intel/issues/10563
  [i915#10564]: https://gitlab.freedesktop.org/drm/intel/issues/10564
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
  [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
  [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
  [i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
  [i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
  [i915#8809]: https://gitlab.freedesktop.org/drm/intel/issues/8809
  [i915#9197]: https://gitlab.freedesktop.org/drm/intel/issues/9197
  [i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
  [i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673
  [i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
  [i915#9812]: https://gitlab.freedesktop.org/drm/intel/issues/9812
  [i915#9886]: https://gitlab.freedesktop.org/drm/intel/issues/9886
  [i915#9920]: https://gitlab.freedesktop.org/drm/intel/issues/9920


Build changes
-------------

  * Linux: CI_DRM_14478 -> Patchwork_130480v4

  CI-20190529: 20190529
  CI_DRM_14478: fc736bbab7abcae683f52604591fe16cf2a85b3e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7780: 93a5298ac980333738cf75da6b66fa6f3769205f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_130480v4: fc736bbab7abcae683f52604591fe16cf2a85b3e @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

9513122d69d8 drm/i915: Implement vblank synchronized MBUS join changes
fe974b8f9b3c drm/i915: Loop over all active pipes in intel_mbus_dbox_update
a884c131a14e drm/i915: Use old mbus_join value when increasing CDCLK
05dd78413fc2 drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_130480v4/index.html

[-- Attachment #2: Type: text/html, Size: 21346 bytes --]

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 04:30:14PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> > In order to make sure we are not breaking the proper sequence
> > lets to updates step by step and don't change MBUS join value
> > during MDCLK/CDCLK programming stage.
> > MBUS join programming would be taken care by pre/post ddb hooks.
> > 
> > v2: - Reworded comment about using old mbus_join value in
> >       intel_set_cdclk(Ville Syrjälä)
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 31aaa9780dfcf..c7813d433c424 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >  
> >  	if (pipe == INVALID_PIPE ||
> >  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > +		struct intel_cdclk_config cdclk_config;
> > +
> >  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >  
> > -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > +		/*
> > +		 * By this hack we want to prevent mbus_join to be changed
> > +		 * beforehand
> 
> That sentence is still confusing.

Write it yourself then. I'm not going to rephrase it anymore.

> 
> > - we will take care of this later in
> > +		 * intel_dbuf_mbus_post_ddb_update
> > +		 */
> > +		cdclk_config = new_cdclk_state->actual;
> > +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> > +
> > +		intel_set_cdclk(i915, &cdclk_config, pipe);
> >  	}
> >  }
> >  
> > -- 
> > 2.37.3
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > According to BSpec we need to do correspondent MBUS updates before
> > or after DBUF reallocation, depending on whether we are enabling
> > or disabling mbus joining(typical scenario is swithing between
> > multiple and single displays).
> > 
> > Also we need to be able to update dbuf min tracker and mdclk ratio
> > separately if mbus_join state didn't change, so lets add one
> > degree of freedom and make it possible.
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> >  1 file changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index bc341abcab2fe..2b947870527fc 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> >  }
> >  
> > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > +	const struct intel_dbuf_state *old_dbuf_state =
> > +		intel_atomic_get_old_dbuf_state(state);
> > +	const struct intel_dbuf_state *new_dbuf_state =
> > +		intel_atomic_get_new_dbuf_state(state);
> > +
> > +	if (DISPLAY_VER(i915) >= 20 &&
> > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > +		/*
> > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > +		 * between MDCLK and CDCLK, updates to related registers need to
> > +		 * happen at a specific point in the CDCLK change sequence. In
> > +		 * that case, we defer to the call to
> > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > +		 */
> > +		return;
> > +	}
> 
> That still needs to be removed or else we'll not update the ratio at
> all during the mbus_join changes. I don't think I saw any removal
> in subsequent patches.
> 
> > +
> > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,

I don't get what is happening here.

"That whole condition I think needs to go. We want to update the ratio
also when changing mbus joining. But that behavioural change doesn't
really belong in this patch, so this is

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"

Now it again needs to be changed or changed in other patch(in this series or which one), 
I don't follow.
Should it be the patch changing MBUS join value?

Stan

> 
> And it just occurred to me that this thing will in fact be wrong
> during the pre/post ddb hooks *and* cdclk is getting decreased
> from the post plane update hook.
> 
> I can't immediately think of a super nice way to handle this.
> 
> Perhaps the most stragithforward idea is to just get the mdclk/cdclk
> ratio from i915->display.cdclk.hw during the pre/post ddb hooks.
> cdclk serialization should guard against parallel updates from
> two both places and thus isplay.cdclk.hw should be safe to use.
> 
> The other option would be to determine if a cdclk decrease
> is going to happen or not, and depending on that use the
> old vs. new dbuf_state when updating the ratio in the
> pre/post ddb hooks.
> 
> > +					    new_dbuf_state->joined_mbus);
> > +}
> > +
> >  /*
> >   * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
> >   * update the request state of all DBUS slices.
> >   */
> > -static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> >  	u32 mbus_ctl;
> > -	const struct intel_dbuf_state *old_dbuf_state =
> > -		intel_atomic_get_old_dbuf_state(state);
> >  	const struct intel_dbuf_state *new_dbuf_state =
> >  		intel_atomic_get_new_dbuf_state(state);
> >  
> > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
> >  	intel_de_rmw(i915, MBUS_CTL,
> >  		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> >  		     MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> > -
> > -	if (DISPLAY_VER(i915) >= 20 &&
> > -	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > -		/*
> > -		 * For Xe2LPD and beyond, when there is a change in the ratio
> > -		 * between MDCLK and CDCLK, updates to related registers need to
> > -		 * happen at a specific point in the CDCLK change sequence. In
> > -		 * that case, we defer to the call to
> > -		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > -		 */
> > -		return;
> > -	}
> > -
> > -	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > -					    new_dbuf_state->joined_mbus);
> >  }
> >  
> >  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> >  
> >  	WARN_ON(!new_dbuf_state->base.changed);
> >  
> > -	update_mbus_pre_enable(state);
> > +	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> 
> I think you squashed that stuff into the wrong patch.
> This one should have a pure refactoring patch.
> 
> > +		intel_dbuf_mbus_join_update(state);
> > +		intel_dbuf_mdclk_min_tracker_update(state);
> > +	}
> > +
> >  	gen9_dbuf_slices_update(i915,
> >  				old_dbuf_state->enabled_slices |
> >  				new_dbuf_state->enabled_slices);
> > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> >  
> >  	WARN_ON(!new_dbuf_state->base.changed);
> >  
> > +	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> > +		intel_dbuf_mbus_join_update(state);
> > +		intel_dbuf_mdclk_min_tracker_update(state);
> > +	}
> > +
> >  	gen9_dbuf_slices_update(i915,
> >  				new_dbuf_state->enabled_slices);
> >  }
> > -- 
> > 2.37.3
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 06:55:21PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 04:30:14PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> > > In order to make sure we are not breaking the proper sequence
> > > lets to updates step by step and don't change MBUS join value
> > > during MDCLK/CDCLK programming stage.
> > > MBUS join programming would be taken care by pre/post ddb hooks.
> > > 
> > > v2: - Reworded comment about using old mbus_join value in
> > >       intel_set_cdclk(Ville Syrjälä)
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index 31aaa9780dfcf..c7813d433c424 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > >  
> > >  	if (pipe == INVALID_PIPE ||
> > >  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > > +		struct intel_cdclk_config cdclk_config;
> > > +
> > >  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > >  
> > > -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > > +		/*
> > > +		 * By this hack we want to prevent mbus_join to be changed
> > > +		 * beforehand
> > 
> > That sentence is still confusing.
> 
> Write it yourself then. I'm not going to rephrase it anymore.

You didn't rephrase it at all AFAIK.

Something like
"MBUS joining will be changed later by
 intel_dbuf_mbus_{pre,post}_ddb_update(), thus
 keep using the old joined_mbus state during cdclk
 programming to match the actual hardware state."

> 
> > 
> > > - we will take care of this later in
> > > +		 * intel_dbuf_mbus_post_ddb_update
> > > +		 */
> > > +		cdclk_config = new_cdclk_state->actual;
> > > +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> > > +
> > > +		intel_set_cdclk(i915, &cdclk_config, pipe);
> > >  	}
> > >  }
> > >  
> > > -- 
> > > 2.37.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > According to BSpec we need to do correspondent MBUS updates before
> > > or after DBUF reallocation, depending on whether we are enabling
> > > or disabling mbus joining(typical scenario is swithing between
> > > multiple and single displays).
> > > 
> > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > separately if mbus_join state didn't change, so lets add one
> > > degree of freedom and make it possible.
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index bc341abcab2fe..2b947870527fc 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > >  }
> > >  
> > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > +		intel_atomic_get_old_dbuf_state(state);
> > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > +		intel_atomic_get_new_dbuf_state(state);
> > > +
> > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > +		/*
> > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > +		 * that case, we defer to the call to
> > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > +		 */
> > > +		return;
> > > +	}
> > 
> > That still needs to be removed or else we'll not update the ratio at
> > all during the mbus_join changes. I don't think I saw any removal
> > in subsequent patches.
> > 
> > > +
> > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> 
> I don't get what is happening here.
> 
> "That whole condition I think needs to go. We want to update the ratio
> also when changing mbus joining. But that behavioural change doesn't
> really belong in this patch, so this is
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> 
> Now it again needs to be changed or changed in other patch(in this series or which one), 
> I don't follow.
> Should it be the patch changing MBUS join value?

Yeah, probably should be in the last patch. Perhaps we
could change it before that, but that would need some
extra brain power to make sure it doesn't temporarily
break something. So probably not worth the hassle
to do as a separate patch.

> 
> Stan
> 
> > 
> > And it just occurred to me that this thing will in fact be wrong
> > during the pre/post ddb hooks *and* cdclk is getting decreased
> > from the post plane update hook.
> > 
> > I can't immediately think of a super nice way to handle this.
> > 
> > Perhaps the most stragithforward idea is to just get the mdclk/cdclk
> > ratio from i915->display.cdclk.hw during the pre/post ddb hooks.
> > cdclk serialization should guard against parallel updates from
> > two both places and thus isplay.cdclk.hw should be safe to use.
> > 
> > The other option would be to determine if a cdclk decrease
> > is going to happen or not, and depending on that use the
> > old vs. new dbuf_state when updating the ratio in the
> > pre/post ddb hooks.
> > 
> > > +					    new_dbuf_state->joined_mbus);
> > > +}
> > > +
> > >  /*
> > >   * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
> > >   * update the request state of all DBUS slices.
> > >   */
> > > -static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
> > >  {
> > >  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > >  	u32 mbus_ctl;
> > > -	const struct intel_dbuf_state *old_dbuf_state =
> > > -		intel_atomic_get_old_dbuf_state(state);
> > >  	const struct intel_dbuf_state *new_dbuf_state =
> > >  		intel_atomic_get_new_dbuf_state(state);
> > >  
> > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > >  	intel_de_rmw(i915, MBUS_CTL,
> > >  		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> > >  		     MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> > > -
> > > -	if (DISPLAY_VER(i915) >= 20 &&
> > > -	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > -		/*
> > > -		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > -		 * between MDCLK and CDCLK, updates to related registers need to
> > > -		 * happen at a specific point in the CDCLK change sequence. In
> > > -		 * that case, we defer to the call to
> > > -		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > -		 */
> > > -		return;
> > > -	}
> > > -
> > > -	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > -					    new_dbuf_state->joined_mbus);
> > >  }
> > >  
> > >  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > >  
> > >  	WARN_ON(!new_dbuf_state->base.changed);
> > >  
> > > -	update_mbus_pre_enable(state);
> > > +	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> > 
> > I think you squashed that stuff into the wrong patch.
> > This one should have a pure refactoring patch.
> > 
> > > +		intel_dbuf_mbus_join_update(state);
> > > +		intel_dbuf_mdclk_min_tracker_update(state);
> > > +	}
> > > +
> > >  	gen9_dbuf_slices_update(i915,
> > >  				old_dbuf_state->enabled_slices |
> > >  				new_dbuf_state->enabled_slices);
> > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> > >  
> > >  	WARN_ON(!new_dbuf_state->base.changed);
> > >  
> > > +	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> > > +		intel_dbuf_mbus_join_update(state);
> > > +		intel_dbuf_mdclk_min_tracker_update(state);
> > > +	}
> > > +
> > >  	gen9_dbuf_slices_update(i915,
> > >  				new_dbuf_state->enabled_slices);
> > >  }
> > > -- 
> > > 2.37.3
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 07:01:48PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 06:55:21PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 04:30:14PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> > > > In order to make sure we are not breaking the proper sequence
> > > > lets to updates step by step and don't change MBUS join value
> > > > during MDCLK/CDCLK programming stage.
> > > > MBUS join programming would be taken care by pre/post ddb hooks.
> > > > 
> > > > v2: - Reworded comment about using old mbus_join value in
> > > >       intel_set_cdclk(Ville Syrjälä)
> > > > 
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 31aaa9780dfcf..c7813d433c424 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > > >  
> > > >  	if (pipe == INVALID_PIPE ||
> > > >  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > > > +		struct intel_cdclk_config cdclk_config;
> > > > +
> > > >  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > > >  
> > > > -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > > > +		/*
> > > > +		 * By this hack we want to prevent mbus_join to be changed
> > > > +		 * beforehand
> > > 
> > > That sentence is still confusing.
> > 
> > Write it yourself then. I'm not going to rephrase it anymore.
> 
> You didn't rephrase it at all AFAIK.
> 
> Something like
> "MBUS joining will be changed later by
>  intel_dbuf_mbus_{pre,post}_ddb_update(), thus
>  keep using the old joined_mbus state during cdclk
>  programming to match the actual hardware state."

Basically my comment says exactly same stuff but with other
words, i.e preventing changing mbus join value beforehand,
until intel_dbuf_mbus_post_ddb_update takes care of that.

> 
> > 
> > > 
> > > > - we will take care of this later in
> > > > +		 * intel_dbuf_mbus_post_ddb_update
> > > > +		 */
> > > > +		cdclk_config = new_cdclk_state->actual;
> > > > +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> > > > +
> > > > +		intel_set_cdclk(i915, &cdclk_config, pipe);
> > > >  	}
> > > >  }
> > > >  
> > > > -- 
> > > > 2.37.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 08:16:55PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 07:01:48PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 06:55:21PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Mar 25, 2024 at 04:30:14PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> > > > > In order to make sure we are not breaking the proper sequence
> > > > > lets to updates step by step and don't change MBUS join value
> > > > > during MDCLK/CDCLK programming stage.
> > > > > MBUS join programming would be taken care by pre/post ddb hooks.
> > > > > 
> > > > > v2: - Reworded comment about using old mbus_join value in
> > > > >       intel_set_cdclk(Ville Syrjälä)
> > > > > 
> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > index 31aaa9780dfcf..c7813d433c424 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > > > >  
> > > > >  	if (pipe == INVALID_PIPE ||
> > > > >  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > > > > +		struct intel_cdclk_config cdclk_config;
> > > > > +
> > > > >  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > > > >  
> > > > > -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > > > > +		/*
> > > > > +		 * By this hack we want to prevent mbus_join to be changed
> > > > > +		 * beforehand
> > > > 
> > > > That sentence is still confusing.
> > > 
> > > Write it yourself then. I'm not going to rephrase it anymore.
> > 
> > You didn't rephrase it at all AFAIK.
> > 
> > Something like
> > "MBUS joining will be changed later by
> >  intel_dbuf_mbus_{pre,post}_ddb_update(), thus
> >  keep using the old joined_mbus state during cdclk
> >  programming to match the actual hardware state."
> 
> Basically my comment says exactly same stuff but with other
> words, i.e preventing changing mbus join value beforehand,
> until intel_dbuf_mbus_post_ddb_update takes care of that.

To me your comment literally says
"we massage cdclk_config in order to prevent mbus
 joining changes from happening here"
Which is a lie; mbus joining will not change here
no matter what we do to the cdclk_config.

What we do is make sure the cdclk_config actually
matches the real hardware state of mbus joining.

> 
> > 
> > > 
> > > > 
> > > > > - we will take care of this later in
> > > > > +		 * intel_dbuf_mbus_post_ddb_update
> > > > > +		 */
> > > > > +		cdclk_config = new_cdclk_state->actual;
> > > > > +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> > > > > +
> > > > > +		intel_set_cdclk(i915, &cdclk_config, pipe);
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.37.3
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > > According to BSpec we need to do correspondent MBUS updates before
> > > > or after DBUF reallocation, depending on whether we are enabling
> > > > or disabling mbus joining(typical scenario is swithing between
> > > > multiple and single displays).
> > > > 
> > > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > > separately if mbus_join state didn't change, so lets add one
> > > > degree of freedom and make it possible.
> > > > 
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > index bc341abcab2fe..2b947870527fc 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > >  }
> > > >  
> > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > > +		intel_atomic_get_old_dbuf_state(state);
> > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > > +		intel_atomic_get_new_dbuf_state(state);
> > > > +
> > > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > +		/*
> > > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > > +		 * that case, we defer to the call to
> > > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > +		 */
> > > > +		return;
> > > > +	}
> > > 
> > > That still needs to be removed or else we'll not update the ratio at
> > > all during the mbus_join changes. I don't think I saw any removal
> > > in subsequent patches.
> > > 
> > > > +
> > > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > 
> > I don't get what is happening here.
> > 
> > "That whole condition I think needs to go. We want to update the ratio
> > also when changing mbus joining. But that behavioural change doesn't
> > really belong in this patch, so this is
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> > 
> > Now it again needs to be changed or changed in other patch(in this series or which one), 
> > I don't follow.
> > Should it be the patch changing MBUS join value?
> 
> Yeah, probably should be in the last patch. Perhaps we
> could change it before that, but that would need some
> extra brain power to make sure it doesn't temporarily
> break something. So probably not worth the hassle
> to do as a separate patch.
> 
> > 
> > Stan
> > 
> > > 
> > > And it just occurred to me that this thing will in fact be wrong
> > > during the pre/post ddb hooks *and* cdclk is getting decreased
> > > from the post plane update hook.
> > > 
> > > I can't immediately think of a super nice way to handle this.

First of all why that
condition above prevents update when mbus join changes?
It exits when mdclk_cdclk ratio is changed not mbus_join?

That review process to me seems rather chaotic.
Constantly something new pops up, moreover we did previously agree
about that code.

> > > 
> > > Perhaps the most stragithforward idea is to just get the mdclk/cdclk
> > > ratio from i915->display.cdclk.hw during the pre/post ddb hooks.
> > > cdclk serialization should guard against parallel updates from
> > > two both places and thus isplay.cdclk.hw should be safe to use.
> > > 
> > > The other option would be to determine if a cdclk decrease
> > > is going to happen or not, and depending on that use the
> > > old vs. new dbuf_state when updating the ratio in the
> > > pre/post ddb hooks.


> > > 
> > > > +					    new_dbuf_state->joined_mbus);
> > > > +}
> > > > +
> > > >  /*
> > > >   * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
> > > >   * update the request state of all DBUS slices.
> > > >   */
> > > > -static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
> > > >  {
> > > >  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > >  	u32 mbus_ctl;
> > > > -	const struct intel_dbuf_state *old_dbuf_state =
> > > > -		intel_atomic_get_old_dbuf_state(state);
> > > >  	const struct intel_dbuf_state *new_dbuf_state =
> > > >  		intel_atomic_get_new_dbuf_state(state);
> > > >  
> > > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > > >  	intel_de_rmw(i915, MBUS_CTL,
> > > >  		     MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> > > >  		     MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> > > > -
> > > > -	if (DISPLAY_VER(i915) >= 20 &&
> > > > -	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > -		/*
> > > > -		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > -		 * between MDCLK and CDCLK, updates to related registers need to
> > > > -		 * happen at a specific point in the CDCLK change sequence. In
> > > > -		 * that case, we defer to the call to
> > > > -		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > -		 */
> > > > -		return;
> > > > -	}
> > > > -
> > > > -	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > > -					    new_dbuf_state->joined_mbus);
> > > >  }
> > > >  
> > > >  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > > >  
> > > >  	WARN_ON(!new_dbuf_state->base.changed);
> > > >  
> > > > -	update_mbus_pre_enable(state);
> > > > +	if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> > > 
> > > I think you squashed that stuff into the wrong patch.
> > > This one should have a pure refactoring patch.
> > > 
> > > > +		intel_dbuf_mbus_join_update(state);
> > > > +		intel_dbuf_mdclk_min_tracker_update(state);
> > > > +	}
> > > > +
> > > >  	gen9_dbuf_slices_update(i915,
> > > >  				old_dbuf_state->enabled_slices |
> > > >  				new_dbuf_state->enabled_slices);
> > > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> > > >  
> > > >  	WARN_ON(!new_dbuf_state->base.changed);
> > > >  
> > > > +	if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> > > > +		intel_dbuf_mbus_join_update(state);
> > > > +		intel_dbuf_mdclk_min_tracker_update(state);
> > > > +	}
> > > > +
> > > >  	gen9_dbuf_slices_update(i915,
> > > >  				new_dbuf_state->enabled_slices);
> > > >  }
> > > > -- 
> > > > 2.37.3
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > > > According to BSpec we need to do correspondent MBUS updates before
> > > > > or after DBUF reallocation, depending on whether we are enabling
> > > > > or disabling mbus joining(typical scenario is swithing between
> > > > > multiple and single displays).
> > > > > 
> > > > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > > > separately if mbus_join state didn't change, so lets add one
> > > > > degree of freedom and make it possible.
> > > > > 
> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > index bc341abcab2fe..2b947870527fc 100644
> > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > > > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > > >  }
> > > > >  
> > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > > > +{
> > > > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > > > +		intel_atomic_get_old_dbuf_state(state);
> > > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > > > +		intel_atomic_get_new_dbuf_state(state);
> > > > > +
> > > > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > > +		/*
> > > > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > > > +		 * that case, we defer to the call to
> > > > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > > +		 */
> > > > > +		return;
> > > > > +	}
> > > > 
> > > > That still needs to be removed or else we'll not update the ratio at
> > > > all during the mbus_join changes. I don't think I saw any removal
> > > > in subsequent patches.
> > > > 
> > > > > +
> > > > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > 
> > > I don't get what is happening here.
> > > 
> > > "That whole condition I think needs to go. We want to update the ratio
> > > also when changing mbus joining. But that behavioural change doesn't
> > > really belong in this patch, so this is
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> > > 
> > > Now it again needs to be changed or changed in other patch(in this series or which one), 
> > > I don't follow.
> > > Should it be the patch changing MBUS join value?
> > 
> > Yeah, probably should be in the last patch. Perhaps we
> > could change it before that, but that would need some
> > extra brain power to make sure it doesn't temporarily
> > break something. So probably not worth the hassle
> > to do as a separate patch.
> > 
> > > 
> > > Stan
> > > 
> > > > 
> > > > And it just occurred to me that this thing will in fact be wrong
> > > > during the pre/post ddb hooks *and* cdclk is getting decreased
> > > > from the post plane update hook.
> > > > 
> > > > I can't immediately think of a super nice way to handle this.
> 
> First of all why that
> condition above prevents update when mbus join changes?
> It exits when mdclk_cdclk ratio is changed not mbus_join?

And what happens when mbus_join needs to be changed
but mdclk_cdclk_ratio remains unchanged?

> 
> That review process to me seems rather chaotic.
> Constantly something new pops up, moreover we did previously agree
> about that code.

The review process exists to make sure the code actually
works correctly. New things come up because of how human
brains work, not all things are immediately apparent to
everyone. If that were the case then you should have
been able to make the code 100% correct from the start,
and I wouldn't be able to come up with new ways in
which it can fail. So I guess you're the pot and
I'm the kettle?

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] drm/i915: Use old mbus_join value when increasing CDCLK

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 08:22:41PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 08:16:55PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 07:01:48PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 06:55:21PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Mar 25, 2024 at 04:30:14PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Mar 25, 2024 at 01:23:27PM +0200, Stanislav Lisovskiy wrote:
> > > > > > In order to make sure we are not breaking the proper sequence
> > > > > > lets to updates step by step and don't change MBUS join value
> > > > > > during MDCLK/CDCLK programming stage.
> > > > > > MBUS join programming would be taken care by pre/post ddb hooks.
> > > > > > 
> > > > > > v2: - Reworded comment about using old mbus_join value in
> > > > > >       intel_set_cdclk(Ville Syrjälä)
> > > > > > 
> > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> > > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > index 31aaa9780dfcf..c7813d433c424 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > > > > >  
> > > > > >  	if (pipe == INVALID_PIPE ||
> > > > > >  	    old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > > > > > +		struct intel_cdclk_config cdclk_config;
> > > > > > +
> > > > > >  		drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> > > > > >  
> > > > > > -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > > > > > +		/*
> > > > > > +		 * By this hack we want to prevent mbus_join to be changed
> > > > > > +		 * beforehand
> > > > > 
> > > > > That sentence is still confusing.
> > > > 
> > > > Write it yourself then. I'm not going to rephrase it anymore.
> > > 
> > > You didn't rephrase it at all AFAIK.
> > > 
> > > Something like
> > > "MBUS joining will be changed later by
> > >  intel_dbuf_mbus_{pre,post}_ddb_update(), thus
> > >  keep using the old joined_mbus state during cdclk
> > >  programming to match the actual hardware state."
> > 
> > Basically my comment says exactly same stuff but with other
> > words, i.e preventing changing mbus join value beforehand,
> > until intel_dbuf_mbus_post_ddb_update takes care of that.
> 
> To me your comment literally says
> "we massage cdclk_config in order to prevent mbus
>  joining changes from happening here"
> Which is a lie; mbus joining will not change here
> no matter what we do to the cdclk_config.
> 
> What we do is make sure the cdclk_config actually
> matches the real hardware state of mbus joining.

Couple of revisions ago we discussed that here we use
old_cdclk state in order to prevent mbus join state to be
changed here so that we kind of do it in steps.
That is what I exactly meant.
To be honest I had some kind of impression from your words
that eventually it might get changed here as well, otherwise
it seems to be quite too much hassle for just keeping
hw/sw in sync.
Arguing about comments is of course really required here.

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > - we will take care of this later in
> > > > > > +		 * intel_dbuf_mbus_post_ddb_update
> > > > > > +		 */
> > > > > > +		cdclk_config = new_cdclk_state->actual;
> > > > > > +		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
> > > > > > +
> > > > > > +		intel_set_cdclk(i915, &cdclk_config, pipe);
> > > > > >  	}
> > > > > >  }
> > > > > >  
> > > > > > -- 
> > > > > > 2.37.3
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Lisovskiy, Stanislav]

On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > > > > According to BSpec we need to do correspondent MBUS updates before
> > > > > > or after DBUF reallocation, depending on whether we are enabling
> > > > > > or disabling mbus joining(typical scenario is swithing between
> > > > > > multiple and single displays).
> > > > > > 
> > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > > > > separately if mbus_join state didn't change, so lets add one
> > > > > > degree of freedom and make it possible.
> > > > > > 
> > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > > > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > index bc341abcab2fe..2b947870527fc 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > > > > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > > > >  }
> > > > > >  
> > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > > > > +{
> > > > > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > > > > +		intel_atomic_get_old_dbuf_state(state);
> > > > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > > > > +		intel_atomic_get_new_dbuf_state(state);
> > > > > > +
> > > > > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > > > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > > > +		/*
> > > > > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > > > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > > > > +		 * that case, we defer to the call to
> > > > > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > > > +		 */
> > > > > > +		return;
> > > > > > +	}
> > > > > 
> > > > > That still needs to be removed or else we'll not update the ratio at
> > > > > all during the mbus_join changes. I don't think I saw any removal
> > > > > in subsequent patches.
> > > > > 
> > > > > > +
> > > > > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > > 
> > > > I don't get what is happening here.
> > > > 
> > > > "That whole condition I think needs to go. We want to update the ratio
> > > > also when changing mbus joining. But that behavioural change doesn't
> > > > really belong in this patch, so this is
> > > > 
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> > > > 
> > > > Now it again needs to be changed or changed in other patch(in this series or which one), 
> > > > I don't follow.
> > > > Should it be the patch changing MBUS join value?
> > > 
> > > Yeah, probably should be in the last patch. Perhaps we
> > > could change it before that, but that would need some
> > > extra brain power to make sure it doesn't temporarily
> > > break something. So probably not worth the hassle
> > > to do as a separate patch.
> > > 
> > > > 
> > > > Stan
> > > > 
> > > > > 
> > > > > And it just occurred to me that this thing will in fact be wrong
> > > > > during the pre/post ddb hooks *and* cdclk is getting decreased
> > > > > from the post plane update hook.
> > > > > 
> > > > > I can't immediately think of a super nice way to handle this.
> > 
> > First of all why that
> > condition above prevents update when mbus join changes?
> > It exits when mdclk_cdclk ratio is changed not mbus_join?
> 
> And what happens when mbus_join needs to be changed
> but mdclk_cdclk_ratio remains unchanged?

If it is not changed, that condition won't exit, 
intel_dbuf_mdclk_cdclk_ratio_update will get called.

> 
> > 
> > That review process to me seems rather chaotic.
> > Constantly something new pops up, moreover we did previously agree
> > about that code.
> 
> The review process exists to make sure the code actually
> works correctly. New things come up because of how human
> brains work, not all things are immediately apparent to
> everyone. If that were the case then you should have
> been able to make the code 100% correct from the start,
> and I wouldn't be able to come up with new ways in
> which it can fail. So I guess you're the pot and
> I'm the kettle?

So do you mean that all code that you commit or give r-b
doesn't have issue and/or will never be required to improve?

There has to be some constructive planning or discussion of what
we aim to do at that stage and what is an acceptance criteria.
Even google/chrome guys tested initially those patches and were fine
with changes.
However what I see here is that you are constantly coming up with something
new.
And both you and me know that current code is far from perfect
as well currently, there are still exist unsolved problems.
So what? We are anyway constantly improving, but not trying
to achieve everything in a single "perfect" patch series.
I don't get why this "perfection" is so particularly required from me here.

Morever many things have to be done in a way exactly how
you say, with no freedom or space for another opinion, while
things like whether to use or not additional variable in the code,
quite often can be done in multiple ways and it is often quite
arguable to say the least, what is the best way to do that.

> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Ville Syrjälä]

On Mon, Mar 25, 2024 at 09:03:32PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > > > > > According to BSpec we need to do correspondent MBUS updates before
> > > > > > > or after DBUF reallocation, depending on whether we are enabling
> > > > > > > or disabling mbus joining(typical scenario is swithing between
> > > > > > > multiple and single displays).
> > > > > > > 
> > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > > > > > separately if mbus_join state didn't change, so lets add one
> > > > > > > degree of freedom and make it possible.
> > > > > > > 
> > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > > > > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > index bc341abcab2fe..2b947870527fc 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > > > > > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > > > > >  }
> > > > > > >  
> > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > > > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > > > > > +		intel_atomic_get_old_dbuf_state(state);
> > > > > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > > > > > +		intel_atomic_get_new_dbuf_state(state);
> > > > > > > +
> > > > > > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > > > > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > > > > +		/*
> > > > > > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > > > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > > > > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > > > > > +		 * that case, we defer to the call to
> > > > > > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > > > > +		 */
> > > > > > > +		return;
> > > > > > > +	}
> > > > > > 
> > > > > > That still needs to be removed or else we'll not update the ratio at
> > > > > > all during the mbus_join changes. I don't think I saw any removal
> > > > > > in subsequent patches.
> > > > > > 
> > > > > > > +
> > > > > > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > > > 
> > > > > I don't get what is happening here.
> > > > > 
> > > > > "That whole condition I think needs to go. We want to update the ratio
> > > > > also when changing mbus joining. But that behavioural change doesn't
> > > > > really belong in this patch, so this is
> > > > > 
> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> > > > > 
> > > > > Now it again needs to be changed or changed in other patch(in this series or which one), 
> > > > > I don't follow.
> > > > > Should it be the patch changing MBUS join value?
> > > > 
> > > > Yeah, probably should be in the last patch. Perhaps we
> > > > could change it before that, but that would need some
> > > > extra brain power to make sure it doesn't temporarily
> > > > break something. So probably not worth the hassle
> > > > to do as a separate patch.
> > > > 
> > > > > 
> > > > > Stan
> > > > > 
> > > > > > 
> > > > > > And it just occurred to me that this thing will in fact be wrong
> > > > > > during the pre/post ddb hooks *and* cdclk is getting decreased
> > > > > > from the post plane update hook.
> > > > > > 
> > > > > > I can't immediately think of a super nice way to handle this.
> > > 
> > > First of all why that
> > > condition above prevents update when mbus join changes?
> > > It exits when mdclk_cdclk ratio is changed not mbus_join?
> > 
> > And what happens when mbus_join needs to be changed
> > but mdclk_cdclk_ratio remains unchanged?
> 
> If it is not changed, that condition won't exit, 
> intel_dbuf_mdclk_cdclk_ratio_update will get called.

Hmm, right. I read it the wrong way around I guess. But it's
still wrong. It will be called only when we change cdclk
(and perhaps not always even in that case) not when we 
change mbus_join. We need to call it in both cases because
they happen at different times in the sequence and we
want to keep this stuff in sync with the actual hardware
state (so same deal as in the the pre plane cdclk hook).

> > 
> > > 
> > > That review process to me seems rather chaotic.
> > > Constantly something new pops up, moreover we did previously agree
> > > about that code.
> > 
> > The review process exists to make sure the code actually
> > works correctly. New things come up because of how human
> > brains work, not all things are immediately apparent to
> > everyone. If that were the case then you should have
> > been able to make the code 100% correct from the start,
> > and I wouldn't be able to come up with new ways in
> > which it can fail. So I guess you're the pot and
> > I'm the kettle?
> 
> So do you mean that all code that you commit or give r-b
> doesn't have issue and/or will never be required to improve?

Rb == says what it does on the tin and has no known
problems that can cause real problems. So far this
does not meet that criteria.

It's fine to have eg. known gaps in functionality
if we plan on dealing with those later. This is,
for example, what we did for the mbus joining
originally. But every patch must work correctly
regardless of those gaps. Of course we sometimes fail
at that and bugs do slip in, but introducing issues
on purpose is not acceptable.

So we need to make sure the ratio gets correctly programmed
in all the steps of the sequence I outlined before. Let me
list it here again:
1. disable pipes
2. increase cdclk
 2.1 reprogram cdclk
 2.2 update dbuf tracker value
3. enable mbus joining if necessary
 3.1 update mbus_ctl
 3.2 update dbuf tracker value
4. reallocate dbuf for planes on active pipes
5. disable mbus joining if necessary
 5.1 update dbuf tracker value
 5.2 update mbus_ctl
6. enable pipes
7. decrease cdclk, mbus joining is unchanged
  7.1 update dbuf tracker value
  7.2 reprogram cdclk

And in order to keep things in sync we need:
Step 2:
- mbus_join == old
- mdclk/cdclk ratio == new
Step 3:
- mbus_join == new
- mdclk/cdclk ratio == old when cdclk is changing in step 7
- mdclk/cdclk ratio == new otherwise
Step 5:
- mbus_join == new
- mdclk/cdclk ratio == old when cdclk is changing in step 7
- mdclk/cdclk ratio == new otherwise
Step 7:
- mbus_join == new
- mdclk/cdclk ratio == new

> Morever many things have to be done in a way exactly how
> you say, with no freedom or space for another opinion, while
> things like whether to use or not additional variable in the code,

Such things are about making the code:
- succinct
- easy to read and understand
- easy to maintain

Unnecessary fluff hinders all that. So just common
practice when writing software that needs to stay
readable and maintainable.

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly

[Ville Syrjälä]

On Tue, Mar 26, 2024 at 02:12:47PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 09:03:32PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > > > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > > > > > > According to BSpec we need to do correspondent MBUS updates before
> > > > > > > > or after DBUF reallocation, depending on whether we are enabling
> > > > > > > > or disabling mbus joining(typical scenario is swithing between
> > > > > > > > multiple and single displays).
> > > > > > > > 
> > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > > > > > > separately if mbus_join state didn't change, so lets add one
> > > > > > > > degree of freedom and make it possible.
> > > > > > > > 
> > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > > > > > >  1 file changed, 35 insertions(+), 19 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > > index bc341abcab2fe..2b947870527fc 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio
> > > > > > > >  			     DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > > > > > > +	const struct intel_dbuf_state *old_dbuf_state =
> > > > > > > > +		intel_atomic_get_old_dbuf_state(state);
> > > > > > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > > > > > > +		intel_atomic_get_new_dbuf_state(state);
> > > > > > > > +
> > > > > > > > +	if (DISPLAY_VER(i915) >= 20 &&
> > > > > > > > +	    old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
> > > > > > > > +		/*
> > > > > > > > +		 * For Xe2LPD and beyond, when there is a change in the ratio
> > > > > > > > +		 * between MDCLK and CDCLK, updates to related registers need to
> > > > > > > > +		 * happen at a specific point in the CDCLK change sequence. In
> > > > > > > > +		 * that case, we defer to the call to
> > > > > > > > +		 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > > > > > > +		 */
> > > > > > > > +		return;
> > > > > > > > +	}
> > > > > > > 
> > > > > > > That still needs to be removed or else we'll not update the ratio at
> > > > > > > all during the mbus_join changes. I don't think I saw any removal
> > > > > > > in subsequent patches.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > > > > 
> > > > > > I don't get what is happening here.
> > > > > > 
> > > > > > "That whole condition I think needs to go. We want to update the ratio
> > > > > > also when changing mbus joining. But that behavioural change doesn't
> > > > > > really belong in this patch, so this is
> > > > > > 
> > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>"
> > > > > > 
> > > > > > Now it again needs to be changed or changed in other patch(in this series or which one), 
> > > > > > I don't follow.
> > > > > > Should it be the patch changing MBUS join value?
> > > > > 
> > > > > Yeah, probably should be in the last patch. Perhaps we
> > > > > could change it before that, but that would need some
> > > > > extra brain power to make sure it doesn't temporarily
> > > > > break something. So probably not worth the hassle
> > > > > to do as a separate patch.
> > > > > 
> > > > > > 
> > > > > > Stan
> > > > > > 
> > > > > > > 
> > > > > > > And it just occurred to me that this thing will in fact be wrong
> > > > > > > during the pre/post ddb hooks *and* cdclk is getting decreased
> > > > > > > from the post plane update hook.
> > > > > > > 
> > > > > > > I can't immediately think of a super nice way to handle this.
> > > > 
> > > > First of all why that
> > > > condition above prevents update when mbus join changes?
> > > > It exits when mdclk_cdclk ratio is changed not mbus_join?
> > > 
> > > And what happens when mbus_join needs to be changed
> > > but mdclk_cdclk_ratio remains unchanged?
> > 
> > If it is not changed, that condition won't exit, 
> > intel_dbuf_mdclk_cdclk_ratio_update will get called.
> 
> Hmm, right. I read it the wrong way around I guess. But it's
> still wrong. It will be called only when we change cdclk
> (and perhaps not always even in that case) not when we 
> change mbus_join. We need to call it in both cases because
> they happen at different times in the sequence and we
> want to keep this stuff in sync with the actual hardware
> state (so same deal as in the the pre plane cdclk hook).
> 
> > > 
> > > > 
> > > > That review process to me seems rather chaotic.
> > > > Constantly something new pops up, moreover we did previously agree
> > > > about that code.
> > > 
> > > The review process exists to make sure the code actually
> > > works correctly. New things come up because of how human
> > > brains work, not all things are immediately apparent to
> > > everyone. If that were the case then you should have
> > > been able to make the code 100% correct from the start,
> > > and I wouldn't be able to come up with new ways in
> > > which it can fail. So I guess you're the pot and
> > > I'm the kettle?
> > 
> > So do you mean that all code that you commit or give r-b
> > doesn't have issue and/or will never be required to improve?
> 
> Rb == says what it does on the tin and has no known
> problems that can cause real problems. So far this
> does not meet that criteria.
> 
> It's fine to have eg. known gaps in functionality
> if we plan on dealing with those later. This is,
> for example, what we did for the mbus joining
> originally. But every patch must work correctly
> regardless of those gaps. Of course we sometimes fail
> at that and bugs do slip in, but introducing issues
> on purpose is not acceptable.
> 
> So we need to make sure the ratio gets correctly programmed
> in all the steps of the sequence I outlined before. Let me
> list it here again:
> 1. disable pipes
> 2. increase cdclk
>  2.1 reprogram cdclk
>  2.2 update dbuf tracker value
> 3. enable mbus joining if necessary
>  3.1 update mbus_ctl
>  3.2 update dbuf tracker value
> 4. reallocate dbuf for planes on active pipes
> 5. disable mbus joining if necessary
>  5.1 update dbuf tracker value
>  5.2 update mbus_ctl
> 6. enable pipes
> 7. decrease cdclk, mbus joining is unchanged
>   7.1 update dbuf tracker value
>   7.2 reprogram cdclk

Ugh. I had a look at our cdclk pre/post hooks and those actually
look very scary. It seems we never updated the logic there to
correctly handle crawl/squash. So it will now happily do the
cdclk update always from intel_set_cdclk_pre_plane_update()
even when decreasing the cdclk frequency. I'll go cook up
a patch...

-- 
Ville Syrjälä
Intel