[Intel-gfx] [PATCH 0/6] drm/i915: SAGV fixes

[Intel-gfx] [PATCH 0/6] drm/i915: SAGV fixes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

While pokingaround the watermarks/etc. I noticed our SAGV code
has a bunch of bugs. Let's try to fix it.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Ville Syrjälä (6):
  drm/i915: Correctly populate use_sagv_wm for all pipes
  drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV
  drm/i915: Widen the QGV point mask
  drm/i915: Unconfuse pre-icl vs. icl+
    intel_sagv_{pre,post}_plane_update()
  drm/i915: Split pre-icl vs. icl+ SAGV hooks apart
  drm/i915: Pimp icl+ sagv pre/post update

 drivers/gpu/drm/i915/display/intel_bw.c |   7 +
 drivers/gpu/drm/i915/display/intel_bw.h |   8 +-
 drivers/gpu/drm/i915/intel_pm.c         | 175 ++++++++++++++----------
 3 files changed, 116 insertions(+), 74 deletions(-)

-- 
2.34.1

[PATCH 1/6] drm/i915: Correctly populate use_sagv_wm for all pipes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When changing between SAGV vs. no SAGV on tgl+ we have to
update the use_sagv_wm flag for all the crtcs or else
an active pipe not already in the state will end up using
the wrong watermarks. That is especially bad when we end up
with the tighter non-SAGV watermarks with SAGV enabled.
Usually ends up in underruns.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 7241c57d3140 ("drm/i915: Add TGL+ SAGV support")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1179bf31f743..d8eb553ffad3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4009,6 +4009,17 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
 			return ret;
 	}
 
+	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
+	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
+		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
+		ret = intel_atomic_lock_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	}
+
 	for_each_new_intel_crtc_in_state(state, crtc,
 					 new_crtc_state, i) {
 		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
@@ -4024,17 +4035,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
 			intel_can_enable_sagv(dev_priv, new_bw_state);
 	}
 
-	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
-	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
-		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
-		if (ret)
-			return ret;
-	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
-		ret = intel_atomic_lock_global_state(&new_bw_state->base);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
-- 
2.34.1

[Intel-gfx] [PATCH 1/6] drm/i915: Correctly populate use_sagv_wm for all pipes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When changing between SAGV vs. no SAGV on tgl+ we have to
update the use_sagv_wm flag for all the crtcs or else
an active pipe not already in the state will end up using
the wrong watermarks. That is especially bad when we end up
with the tighter non-SAGV watermarks with SAGV enabled.
Usually ends up in underruns.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 7241c57d3140 ("drm/i915: Add TGL+ SAGV support")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1179bf31f743..d8eb553ffad3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4009,6 +4009,17 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
 			return ret;
 	}
 
+	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
+	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
+		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
+		ret = intel_atomic_lock_global_state(&new_bw_state->base);
+		if (ret)
+			return ret;
+	}
+
 	for_each_new_intel_crtc_in_state(state, crtc,
 					 new_crtc_state, i) {
 		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
@@ -4024,17 +4035,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
 			intel_can_enable_sagv(dev_priv, new_bw_state);
 	}
 
-	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
-	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
-		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
-		if (ret)
-			return ret;
-	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
-		ret = intel_atomic_lock_global_state(&new_bw_state->base);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the only thing that is changing is SAGV vs. no SAGV but
the number of active planes and the total data rates end up
unchanged we currently bail out of intel_bw_atomic_check()
early and forget to actually compute the new WGV point
mask and thus won't actually enable/disable SAGV as requested.
This ends up poorly if we end up running with SAGV enabled
when we shouldn't. Usually ends up in underruns.
To fix this let's go through the QGV point mask computation
if anyone else already added the bw state for us.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 23aa8e06de18..d72ccee7d53b 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
 	if (num_psf_gv_points > 0)
 		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
 
+	/*
+	 * If we already have the bw state then recompute everything
+	 * even if pipe data_rate / active_planes didn't change.
+	 * Other things (such as SAGV) may have changed.
+	 */
+	new_bw_state = intel_atomic_get_new_bw_state(state);
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		unsigned int old_data_rate =
-- 
2.34.1

[Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the only thing that is changing is SAGV vs. no SAGV but
the number of active planes and the total data rates end up
unchanged we currently bail out of intel_bw_atomic_check()
early and forget to actually compute the new WGV point
mask and thus won't actually enable/disable SAGV as requested.
This ends up poorly if we end up running with SAGV enabled
when we shouldn't. Usually ends up in underruns.
To fix this let's go through the QGV point mask computation
if anyone else already added the bw state for us.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 23aa8e06de18..d72ccee7d53b 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
 	if (num_psf_gv_points > 0)
 		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
 
+	/*
+	 * If we already have the bw state then recompute everything
+	 * even if pipe data_rate / active_planes didn't change.
+	 * Other things (such as SAGV) may have changed.
+	 */
+	new_bw_state = intel_atomic_get_new_bw_state(state);
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		unsigned int old_data_rate =
-- 
2.34.1

[PATCH 3/6] drm/i915: Widen the QGV point mask

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

adlp+ adds some extra bits to the QGV point mask. The code attempts
to handle that but forgot to actually make sure we can store those
bits in the bw state. Fix it.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 192fbfb76744 ("drm/i915: Implement PSF GV point support")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index 46c6eecbd917..0ceaed1c9656 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -30,19 +30,19 @@ struct intel_bw_state {
 	 */
 	u8 pipe_sagv_reject;
 
+	/* bitmask of active pipes */
+	u8 active_pipes;
+
 	/*
 	 * Current QGV points mask, which restricts
 	 * some particular SAGV states, not to confuse
 	 * with pipe_sagv_mask.
 	 */
-	u8 qgv_points_mask;
+	u16 qgv_points_mask;
 
 	unsigned int data_rate[I915_MAX_PIPES];
 	u8 num_active_planes[I915_MAX_PIPES];
 
-	/* bitmask of active pipes */
-	u8 active_pipes;
-
 	int min_cdclk;
 };
 
-- 
2.34.1

[Intel-gfx] [PATCH 3/6] drm/i915: Widen the QGV point mask

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

adlp+ adds some extra bits to the QGV point mask. The code attempts
to handle that but forgot to actually make sure we can store those
bits in the bw state. Fix it.

Cc: stable@vger.kernel.org
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Fixes: 192fbfb76744 ("drm/i915: Implement PSF GV point support")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
index 46c6eecbd917..0ceaed1c9656 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.h
+++ b/drivers/gpu/drm/i915/display/intel_bw.h
@@ -30,19 +30,19 @@ struct intel_bw_state {
 	 */
 	u8 pipe_sagv_reject;
 
+	/* bitmask of active pipes */
+	u8 active_pipes;
+
 	/*
 	 * Current QGV points mask, which restricts
 	 * some particular SAGV states, not to confuse
 	 * with pipe_sagv_mask.
 	 */
-	u8 qgv_points_mask;
+	u16 qgv_points_mask;
 
 	unsigned int data_rate[I915_MAX_PIPES];
 	u8 num_active_planes[I915_MAX_PIPES];
 
-	/* bitmask of active pipes */
-	u8 active_pipes;
-
 	int min_cdclk;
 };
 
-- 
2.34.1

[Intel-gfx] [PATCH 4/6] drm/i915: Unconfuse pre-icl vs. icl+ intel_sagv_{pre, post}_plane_update()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_sagv_{pre,post}_plane_update() can accidentally forget
to bail out early on pre-icl and proceed down the icl+ codepath
at the end of the function. Fortunately it'll bail out before
it gets too far due to old_qgv_mask==new_qgv_mask==0 so no real
bug here. But lets make the code less confusing anyway.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d8eb553ffad3..068870b17c43 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3806,8 +3806,9 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
 	if (!new_bw_state)
 		return;
 
-	if (DISPLAY_VER(dev_priv) < 11 && !intel_can_enable_sagv(dev_priv, new_bw_state)) {
-		intel_disable_sagv(dev_priv);
+	if (DISPLAY_VER(dev_priv) < 11) {
+		if (!intel_can_enable_sagv(dev_priv, new_bw_state))
+			intel_disable_sagv(dev_priv);
 		return;
 	}
 
@@ -3857,8 +3858,9 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
 	if (!new_bw_state)
 		return;
 
-	if (DISPLAY_VER(dev_priv) < 11 && intel_can_enable_sagv(dev_priv, new_bw_state)) {
-		intel_enable_sagv(dev_priv);
+	if (DISPLAY_VER(dev_priv) < 11) {
+		if (intel_can_enable_sagv(dev_priv, new_bw_state))
+			intel_enable_sagv(dev_priv);
 		return;
 	}
 
-- 
2.34.1

[Intel-gfx] [PATCH 5/6] drm/i915: Split pre-icl vs. icl+ SAGV hooks apart

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To further reduce the confusion between the pre-icl vs. icl+
SAGV codepaths let's do a full split.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 120 ++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 068870b17c43..8b70cdc3b58b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3785,34 +3785,44 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
+static void skl_sagv_pre_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_bw_state *new_bw_state =
+		intel_atomic_get_new_bw_state(state);
+
+	if (!new_bw_state)
+		return;
+
+	if (!intel_can_enable_sagv(i915, new_bw_state))
+		intel_disable_sagv(i915);
+}
+
+static void skl_sagv_post_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_bw_state *new_bw_state =
+		intel_atomic_get_new_bw_state(state);
+
+	if (!new_bw_state)
+		return;
+
+	if (intel_can_enable_sagv(i915, new_bw_state))
+		intel_enable_sagv(i915);
+}
+
+static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_bw_state *new_bw_state;
-	const struct intel_bw_state *old_bw_state;
-	u32 new_mask = 0;
+	const struct intel_bw_state *old_bw_state =
+		intel_atomic_get_old_bw_state(state);
+	const struct intel_bw_state *new_bw_state =
+		intel_atomic_get_new_bw_state(state);
+	u32 new_mask;
 
-	/*
-	 * Just return if we can't control SAGV or don't have it.
-	 * This is different from situation when we have SAGV but just can't
-	 * afford it due to DBuf limitation - in case if SAGV is completely
-	 * disabled in a BIOS, we are not even allowed to send a PCode request,
-	 * as it will throw an error. So have to check it here.
-	 */
-	if (!intel_has_sagv(dev_priv))
-		return;
-
-	new_bw_state = intel_atomic_get_new_bw_state(state);
 	if (!new_bw_state)
 		return;
 
-	if (DISPLAY_VER(dev_priv) < 11) {
-		if (!intel_can_enable_sagv(dev_priv, new_bw_state))
-			intel_disable_sagv(dev_priv);
-		return;
-	}
-
-	old_bw_state = intel_atomic_get_old_bw_state(state);
 	/*
 	 * Nothing to mask
 	 */
@@ -3837,34 +3847,18 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
 	icl_pcode_restrict_qgv_points(dev_priv, new_mask);
 }
 
-void intel_sagv_post_plane_update(struct intel_atomic_state *state)
+static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	const struct intel_bw_state *new_bw_state;
-	const struct intel_bw_state *old_bw_state;
+	const struct intel_bw_state *old_bw_state =
+		intel_atomic_get_old_bw_state(state);
+	const struct intel_bw_state *new_bw_state =
+		intel_atomic_get_new_bw_state(state);
 	u32 new_mask = 0;
 
-	/*
-	 * Just return if we can't control SAGV or don't have it.
-	 * This is different from situation when we have SAGV but just can't
-	 * afford it due to DBuf limitation - in case if SAGV is completely
-	 * disabled in a BIOS, we are not even allowed to send a PCode request,
-	 * as it will throw an error. So have to check it here.
-	 */
-	if (!intel_has_sagv(dev_priv))
-		return;
-
-	new_bw_state = intel_atomic_get_new_bw_state(state);
 	if (!new_bw_state)
 		return;
 
-	if (DISPLAY_VER(dev_priv) < 11) {
-		if (intel_can_enable_sagv(dev_priv, new_bw_state))
-			intel_enable_sagv(dev_priv);
-		return;
-	}
-
-	old_bw_state = intel_atomic_get_old_bw_state(state);
 	/*
 	 * Nothing to unmask
 	 */
@@ -3882,6 +3876,46 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
 	icl_pcode_restrict_qgv_points(dev_priv, new_mask);
 }
 
+void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+
+	/*
+	 * Just return if we can't control SAGV or don't have it.
+	 * This is different from situation when we have SAGV but just can't
+	 * afford it due to DBuf limitation - in case if SAGV is completely
+	 * disabled in a BIOS, we are not even allowed to send a PCode request,
+	 * as it will throw an error. So have to check it here.
+	 */
+	if (!intel_has_sagv(i915))
+		return;
+
+	if (DISPLAY_VER(i915) >= 11)
+		icl_sagv_pre_plane_update(state);
+	else
+		skl_sagv_pre_plane_update(state);
+}
+
+void intel_sagv_post_plane_update(struct intel_atomic_state *state)
+{
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+
+	/*
+	 * Just return if we can't control SAGV or don't have it.
+	 * This is different from situation when we have SAGV but just can't
+	 * afford it due to DBuf limitation - in case if SAGV is completely
+	 * disabled in a BIOS, we are not even allowed to send a PCode request,
+	 * as it will throw an error. So have to check it here.
+	 */
+	if (!intel_has_sagv(i915))
+		return;
+
+	if (DISPLAY_VER(i915) >= 11)
+		icl_sagv_post_plane_update(state);
+	else
+		skl_sagv_post_plane_update(state);
+}
+
 static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-- 
2.34.1

[Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add some debugs on what exactly we're doing to the QGV point mask
in the icl+ sagv pre/post plane update hooks. Currently we're just
guessing.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8b70cdc3b58b..5d1f1a9988bb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
 		intel_atomic_get_old_bw_state(state);
 	const struct intel_bw_state *new_bw_state =
 		intel_atomic_get_new_bw_state(state);
-	u32 new_mask;
+	u32 old_mask, new_mask;
 
 	if (!new_bw_state)
 		return;
 
-	/*
-	 * Nothing to mask
-	 */
-	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
-		return;
-
+	old_mask = old_bw_state->qgv_points_mask;
 	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
 
-	/*
-	 * If new mask is zero - means there is nothing to mask,
-	 * we can only unmask, which should be done in unmask.
-	 */
-	if (!new_mask)
+	if (old_mask == new_mask)
 		return;
 
+	WARN_ON(!new_bw_state->base.changed);
+
+	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
+		    old_mask, new_mask);
+
 	/*
 	 * Restrict required qgv points before updating the configuration.
 	 * According to BSpec we can't mask and unmask qgv points at the same
@@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
 		intel_atomic_get_old_bw_state(state);
 	const struct intel_bw_state *new_bw_state =
 		intel_atomic_get_new_bw_state(state);
-	u32 new_mask = 0;
+	u32 old_mask, new_mask;
 
 	if (!new_bw_state)
 		return;
 
-	/*
-	 * Nothing to unmask
-	 */
-	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
-		return;
-
+	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
 	new_mask = new_bw_state->qgv_points_mask;
 
+	if (old_mask == new_mask)
+		return;
+
+	WARN_ON(!new_bw_state->base.changed);
+
+	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
+		    old_mask, new_mask);
+
 	/*
 	 * Allow required qgv points after updating the configuration.
 	 * According to BSpec we can't mask and unmask qgv points at the same
-- 
2.34.1

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:11AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add some debugs on what exactly we're doing to the QGV point mask
> in the icl+ sagv pre/post plane update hooks. Currently we're just
> guessing.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 19 deletions(-)

Weird I think, I had those debugs initially. Definitely remember
there was something similar. Was it kinda removed later?

Stan

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
 
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8b70cdc3b58b..5d1f1a9988bb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
>  		intel_atomic_get_old_bw_state(state);
>  	const struct intel_bw_state *new_bw_state =
>  		intel_atomic_get_new_bw_state(state);
> -	u32 new_mask;
> +	u32 old_mask, new_mask;
>  
>  	if (!new_bw_state)
>  		return;
>  
> -	/*
> -	 * Nothing to mask
> -	 */
> -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> -		return;
> -
> +	old_mask = old_bw_state->qgv_points_mask;
>  	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
>  
> -	/*
> -	 * If new mask is zero - means there is nothing to mask,
> -	 * we can only unmask, which should be done in unmask.
> -	 */
> -	if (!new_mask)
> +	if (old_mask == new_mask)
>  		return;
>  
> +	WARN_ON(!new_bw_state->base.changed);
> +
> +	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
> +		    old_mask, new_mask);
> +
>  	/*
>  	 * Restrict required qgv points before updating the configuration.
>  	 * According to BSpec we can't mask and unmask qgv points at the same
> @@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
>  		intel_atomic_get_old_bw_state(state);
>  	const struct intel_bw_state *new_bw_state =
>  		intel_atomic_get_new_bw_state(state);
> -	u32 new_mask = 0;
> +	u32 old_mask, new_mask;
>  
>  	if (!new_bw_state)
>  		return;
>  
> -	/*
> -	 * Nothing to unmask
> -	 */
> -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> -		return;
> -
> +	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
>  	new_mask = new_bw_state->qgv_points_mask;
>  
> +	if (old_mask == new_mask)
> +		return;
> +
> +	WARN_ON(!new_bw_state->base.changed);
> +
> +	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
> +		    old_mask, new_mask);
> +
>  	/*
>  	 * Allow required qgv points after updating the configuration.
>  	 * According to BSpec we can't mask and unmask qgv points at the same
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the only thing that is changing is SAGV vs. no SAGV but
> the number of active planes and the total data rates end up
> unchanged we currently bail out of intel_bw_atomic_check()
> early and forget to actually compute the new WGV point
> mask and thus won't actually enable/disable SAGV as requested.
> This ends up poorly if we end up running with SAGV enabled
> when we shouldn't. Usually ends up in underruns.
> To fix this let's go through the QGV point mask computation
> if anyone else already added the bw state for us.

Haven't been looking this in a while. Despite we have been
looking like few revisions together still some bugs :(

I thought SAGV vs No SAGV can't change if active planes 
or data rate didn't change? Because it means we probably
still have same ddb allocations, which means SAGV state
will just stay the same.

Stan

> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 23aa8e06de18..d72ccee7d53b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	if (num_psf_gv_points > 0)
>  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
>  
> +	/*
> +	 * If we already have the bw state then recompute everything
> +	 * even if pipe data_rate / active_planes didn't change.
> +	 * Other things (such as SAGV) may have changed.
> +	 */
> +	new_bw_state = intel_atomic_get_new_bw_state(state);
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		unsigned int old_data_rate =
> -- 
> 2.34.1
> 

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the only thing that is changing is SAGV vs. no SAGV but
> the number of active planes and the total data rates end up
> unchanged we currently bail out of intel_bw_atomic_check()
> early and forget to actually compute the new WGV point
> mask and thus won't actually enable/disable SAGV as requested.
> This ends up poorly if we end up running with SAGV enabled
> when we shouldn't. Usually ends up in underruns.
> To fix this let's go through the QGV point mask computation
> if anyone else already added the bw state for us.

Haven't been looking this in a while. Despite we have been
looking like few revisions together still some bugs :(

I thought SAGV vs No SAGV can't change if active planes 
or data rate didn't change? Because it means we probably
still have same ddb allocations, which means SAGV state
will just stay the same.

Stan

> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 23aa8e06de18..d72ccee7d53b 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	if (num_psf_gv_points > 0)
>  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
>  
> +	/*
> +	 * If we already have the bw state then recompute everything
> +	 * even if pipe data_rate / active_planes didn't change.
> +	 * Other things (such as SAGV) may have changed.
> +	 */
> +	new_bw_state = intel_atomic_get_new_bw_state(state);
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		unsigned int old_data_rate =
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 1/6] drm/i915: Correctly populate use_sagv_wm for all pipes

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:06AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When changing between SAGV vs. no SAGV on tgl+ we have to
> update the use_sagv_wm flag for all the crtcs or else
> an active pipe not already in the state will end up using
> the wrong watermarks. That is especially bad when we end up
> with the tighter non-SAGV watermarks with SAGV enabled.
> Usually ends up in underruns.

Probably valid point. Just noticed that we have this constant 
confusion, between cases when we have to update only crtc
which are added to the state(i.e only those which changed)
versus cases when everything has to be updated, regardless 
if its in the state or not.

I think it didn't ever caused underruns however, which is
strange - currently we mostly hit underruns once due to 
some PSR magic. Might be we just are lucky enough to get
all crtcs added to the state for some other reasons.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Stan

> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 7241c57d3140 ("drm/i915: Add TGL+ SAGV support")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1179bf31f743..d8eb553ffad3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4009,6 +4009,17 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> +	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> +	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> +		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for_each_new_intel_crtc_in_state(state, crtc,
>  					 new_crtc_state, i) {
>  		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> @@ -4024,17 +4035,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			intel_can_enable_sagv(dev_priv, new_bw_state);
>  	}
>  
> -	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> -	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> -		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> -		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.34.1
> 

Re: [PATCH 1/6] drm/i915: Correctly populate use_sagv_wm for all pipes

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:06AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When changing between SAGV vs. no SAGV on tgl+ we have to
> update the use_sagv_wm flag for all the crtcs or else
> an active pipe not already in the state will end up using
> the wrong watermarks. That is especially bad when we end up
> with the tighter non-SAGV watermarks with SAGV enabled.
> Usually ends up in underruns.

Probably valid point. Just noticed that we have this constant 
confusion, between cases when we have to update only crtc
which are added to the state(i.e only those which changed)
versus cases when everything has to be updated, regardless 
if its in the state or not.

I think it didn't ever caused underruns however, which is
strange - currently we mostly hit underruns once due to 
some PSR magic. Might be we just are lucky enough to get
all crtcs added to the state for some other reasons.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Stan

> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 7241c57d3140 ("drm/i915: Add TGL+ SAGV support")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1179bf31f743..d8eb553ffad3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4009,6 +4009,17 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> +	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> +	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> +		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for_each_new_intel_crtc_in_state(state, crtc,
>  					 new_crtc_state, i) {
>  		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> @@ -4024,17 +4035,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			intel_can_enable_sagv(dev_priv, new_bw_state);
>  	}
>  
> -	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> -	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> -		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> -		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If the only thing that is changing is SAGV vs. no SAGV but
> > the number of active planes and the total data rates end up
> > unchanged we currently bail out of intel_bw_atomic_check()
> > early and forget to actually compute the new WGV point
> > mask and thus won't actually enable/disable SAGV as requested.
> > This ends up poorly if we end up running with SAGV enabled
> > when we shouldn't. Usually ends up in underruns.
> > To fix this let's go through the QGV point mask computation
> > if anyone else already added the bw state for us.
> 
> Haven't been looking this in a while. Despite we have been
> looking like few revisions together still some bugs :(
> 
> I thought SAGV vs No SAGV can't change if active planes 
> or data rate didn't change? Because it means we probably
> still have same ddb allocations, which means SAGV state
> will just stay the same.

SAGV can change due to watermarks/ddb allocations. The easiest
way to trip this up is to try to use the async flip wm0/ddb 
optimization. That immediately forgets to turn off SAGV and
we get underruns, whcih is how I noticed this. And I don't
immediately see any easy proof that this couldn't also happen
due to some other plane changes.

> 
> Stan
> 
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 23aa8e06de18..d72ccee7d53b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> >  	if (num_psf_gv_points > 0)
> >  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
> >  
> > +	/*
> > +	 * If we already have the bw state then recompute everything
> > +	 * even if pipe data_rate / active_planes didn't change.
> > +	 * Other things (such as SAGV) may have changed.
> > +	 */
> > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		unsigned int old_data_rate =
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If the only thing that is changing is SAGV vs. no SAGV but
> > the number of active planes and the total data rates end up
> > unchanged we currently bail out of intel_bw_atomic_check()
> > early and forget to actually compute the new WGV point
> > mask and thus won't actually enable/disable SAGV as requested.
> > This ends up poorly if we end up running with SAGV enabled
> > when we shouldn't. Usually ends up in underruns.
> > To fix this let's go through the QGV point mask computation
> > if anyone else already added the bw state for us.
> 
> Haven't been looking this in a while. Despite we have been
> looking like few revisions together still some bugs :(
> 
> I thought SAGV vs No SAGV can't change if active planes 
> or data rate didn't change? Because it means we probably
> still have same ddb allocations, which means SAGV state
> will just stay the same.

SAGV can change due to watermarks/ddb allocations. The easiest
way to trip this up is to try to use the async flip wm0/ddb 
optimization. That immediately forgets to turn off SAGV and
we get underruns, whcih is how I noticed this. And I don't
immediately see any easy proof that this couldn't also happen
due to some other plane changes.

> 
> Stan
> 
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 23aa8e06de18..d72ccee7d53b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> >  	if (num_psf_gv_points > 0)
> >  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
> >  
> > +	/*
> > +	 * If we already have the bw state then recompute everything
> > +	 * even if pipe data_rate / active_planes didn't change.
> > +	 * Other things (such as SAGV) may have changed.
> > +	 */
> > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		unsigned int old_data_rate =
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

[Ville Syrjälä]

On Mon, Feb 14, 2022 at 12:00:11PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 11:18:11AM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add some debugs on what exactly we're doing to the QGV point mask
> > in the icl+ sagv pre/post plane update hooks. Currently we're just
> > guessing.
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++-----------------
> >  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> Weird I think, I had those debugs initially. Definitely remember
> there was something similar. Was it kinda removed later?

Can't immediately see any such debugs being added or removed
by any commit.

> 
> Stan
> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>  
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8b70cdc3b58b..5d1f1a9988bb 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
> >  		intel_atomic_get_old_bw_state(state);
> >  	const struct intel_bw_state *new_bw_state =
> >  		intel_atomic_get_new_bw_state(state);
> > -	u32 new_mask;
> > +	u32 old_mask, new_mask;
> >  
> >  	if (!new_bw_state)
> >  		return;
> >  
> > -	/*
> > -	 * Nothing to mask
> > -	 */
> > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > -		return;
> > -
> > +	old_mask = old_bw_state->qgv_points_mask;
> >  	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> >  
> > -	/*
> > -	 * If new mask is zero - means there is nothing to mask,
> > -	 * we can only unmask, which should be done in unmask.
> > -	 */
> > -	if (!new_mask)
> > +	if (old_mask == new_mask)
> >  		return;
> >  
> > +	WARN_ON(!new_bw_state->base.changed);
> > +
> > +	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
> > +		    old_mask, new_mask);
> > +
> >  	/*
> >  	 * Restrict required qgv points before updating the configuration.
> >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > @@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
> >  		intel_atomic_get_old_bw_state(state);
> >  	const struct intel_bw_state *new_bw_state =
> >  		intel_atomic_get_new_bw_state(state);
> > -	u32 new_mask = 0;
> > +	u32 old_mask, new_mask;
> >  
> >  	if (!new_bw_state)
> >  		return;
> >  
> > -	/*
> > -	 * Nothing to unmask
> > -	 */
> > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > -		return;
> > -
> > +	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> >  	new_mask = new_bw_state->qgv_points_mask;
> >  
> > +	if (old_mask == new_mask)
> > +		return;
> > +
> > +	WARN_ON(!new_bw_state->base.changed);
> > +
> > +	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
> > +		    old_mask, new_mask);
> > +
> >  	/*
> >  	 * Allow required qgv points after updating the configuration.
> >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > -- 
> > 2.34.1
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If the only thing that is changing is SAGV vs. no SAGV but
> > > the number of active planes and the total data rates end up
> > > unchanged we currently bail out of intel_bw_atomic_check()
> > > early and forget to actually compute the new WGV point
> > > mask and thus won't actually enable/disable SAGV as requested.
> > > This ends up poorly if we end up running with SAGV enabled
> > > when we shouldn't. Usually ends up in underruns.
> > > To fix this let's go through the QGV point mask computation
> > > if anyone else already added the bw state for us.
> > 
> > Haven't been looking this in a while. Despite we have been
> > looking like few revisions together still some bugs :(
> > 
> > I thought SAGV vs No SAGV can't change if active planes 
> > or data rate didn't change? Because it means we probably
> > still have same ddb allocations, which means SAGV state
> > will just stay the same.
> 
> SAGV can change due to watermarks/ddb allocations. The easiest
> way to trip this up is to try to use the async flip wm0/ddb 
> optimization. That immediately forgets to turn off SAGV and
> we get underruns, whcih is how I noticed this. And I don't
> immediately see any easy proof that this couldn't also happen
> due to some other plane changes.

Thats the way it was initially implemented even before SAGV was added.
I think it can be dated back to the very first bw check was implemented.

commit c457d9cf256e942138a54a2e80349ee7fe20c391
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Fri May 24 18:36:14 2019 +0300

    drm/i915: Make sure we have enough memory bandwidth on ICL

+int intel_bw_atomic_check(struct intel_atomic_state *state)
+{
+       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
+       struct intel_bw_state *bw_state = NULL;
+       unsigned int data_rate, max_data_rate;
+       unsigned int num_active_planes;
+       struct intel_crtc *crtc;
+       int i;
+
+       /* FIXME earlier gens need some checks too */
+       if (INTEL_GEN(dev_priv) < 11)
+               return 0;
+
+       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+                                           new_crtc_state, i) {
+               unsigned int old_data_rate =
+                       intel_bw_crtc_data_rate(old_crtc_state);
+               unsigned int new_data_rate =
+                       intel_bw_crtc_data_rate(new_crtc_state);
+               unsigned int old_active_planes =
+                       intel_bw_crtc_num_active_planes(old_crtc_state);
+               unsigned int new_active_planes =
+                       intel_bw_crtc_num_active_planes(new_crtc_state);
+
+               /*
+                * Avoid locking the bw state when
+                * nothing significant has changed.
+                */
+               if (old_data_rate == new_data_rate &&
+                   old_active_planes == new_active_planes)
+                       continue;
+
+               bw_state  = intel_atomic_get_bw_state(state);
+               if (IS_ERR(bw_state))
+                       return PTR_ERR(bw_state);

However, what can cause watermarks/ddb to change, besides plane state change
and/or active planes change? We change watermarks, when we change ddb allocations
and we change ddb allocations when active planes had changed and/or data rate
had changed.

Stan

> 
> > 
> > Stan
> > 
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index 23aa8e06de18..d72ccee7d53b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> > >  	if (num_psf_gv_points > 0)
> > >  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
> > >  
> > > +	/*
> > > +	 * If we already have the bw state then recompute everything
> > > +	 * even if pipe data_rate / active_planes didn't change.
> > > +	 * Other things (such as SAGV) may have changed.
> > > +	 */
> > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		unsigned int old_data_rate =
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If the only thing that is changing is SAGV vs. no SAGV but
> > > the number of active planes and the total data rates end up
> > > unchanged we currently bail out of intel_bw_atomic_check()
> > > early and forget to actually compute the new WGV point
> > > mask and thus won't actually enable/disable SAGV as requested.
> > > This ends up poorly if we end up running with SAGV enabled
> > > when we shouldn't. Usually ends up in underruns.
> > > To fix this let's go through the QGV point mask computation
> > > if anyone else already added the bw state for us.
> > 
> > Haven't been looking this in a while. Despite we have been
> > looking like few revisions together still some bugs :(
> > 
> > I thought SAGV vs No SAGV can't change if active planes 
> > or data rate didn't change? Because it means we probably
> > still have same ddb allocations, which means SAGV state
> > will just stay the same.
> 
> SAGV can change due to watermarks/ddb allocations. The easiest
> way to trip this up is to try to use the async flip wm0/ddb 
> optimization. That immediately forgets to turn off SAGV and
> we get underruns, whcih is how I noticed this. And I don't
> immediately see any easy proof that this couldn't also happen
> due to some other plane changes.

Thats the way it was initially implemented even before SAGV was added.
I think it can be dated back to the very first bw check was implemented.

commit c457d9cf256e942138a54a2e80349ee7fe20c391
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Fri May 24 18:36:14 2019 +0300

    drm/i915: Make sure we have enough memory bandwidth on ICL

+int intel_bw_atomic_check(struct intel_atomic_state *state)
+{
+       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
+       struct intel_bw_state *bw_state = NULL;
+       unsigned int data_rate, max_data_rate;
+       unsigned int num_active_planes;
+       struct intel_crtc *crtc;
+       int i;
+
+       /* FIXME earlier gens need some checks too */
+       if (INTEL_GEN(dev_priv) < 11)
+               return 0;
+
+       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+                                           new_crtc_state, i) {
+               unsigned int old_data_rate =
+                       intel_bw_crtc_data_rate(old_crtc_state);
+               unsigned int new_data_rate =
+                       intel_bw_crtc_data_rate(new_crtc_state);
+               unsigned int old_active_planes =
+                       intel_bw_crtc_num_active_planes(old_crtc_state);
+               unsigned int new_active_planes =
+                       intel_bw_crtc_num_active_planes(new_crtc_state);
+
+               /*
+                * Avoid locking the bw state when
+                * nothing significant has changed.
+                */
+               if (old_data_rate == new_data_rate &&
+                   old_active_planes == new_active_planes)
+                       continue;
+
+               bw_state  = intel_atomic_get_bw_state(state);
+               if (IS_ERR(bw_state))
+                       return PTR_ERR(bw_state);

However, what can cause watermarks/ddb to change, besides plane state change
and/or active planes change? We change watermarks, when we change ddb allocations
and we change ddb allocations when active planes had changed and/or data rate
had changed.

Stan

> 
> > 
> > Stan
> > 
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Fixes: 20f505f22531 ("drm/i915: Restrict qgv points which don't have enough bandwidth.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index 23aa8e06de18..d72ccee7d53b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > @@ -846,6 +846,13 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> > >  	if (num_psf_gv_points > 0)
> > >  		mask |= REG_GENMASK(num_psf_gv_points - 1, 0) << ADLS_PSF_PT_SHIFT;
> > >  
> > > +	/*
> > > +	 * If we already have the bw state then recompute everything
> > > +	 * even if pipe data_rate / active_planes didn't change.
> > > +	 * Other things (such as SAGV) may have changed.
> > > +	 */
> > > +	new_bw_state = intel_atomic_get_new_bw_state(state);
> > > +
> > >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >  					    new_crtc_state, i) {
> > >  		unsigned int old_data_rate =
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 3/6] drm/i915: Widen the QGV point mask

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:08AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> adlp+ adds some extra bits to the QGV point mask. The code attempts
> to handle that but forgot to actually make sure we can store those
> bits in the bw state. Fix it.
> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 192fbfb76744 ("drm/i915: Implement PSF GV point support")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 46c6eecbd917..0ceaed1c9656 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -30,19 +30,19 @@ struct intel_bw_state {
>  	 */
>  	u8 pipe_sagv_reject;
>  
> +	/* bitmask of active pipes */
> +	u8 active_pipes;
> +
>  	/*
>  	 * Current QGV points mask, which restricts
>  	 * some particular SAGV states, not to confuse
>  	 * with pipe_sagv_mask.
>  	 */
> -	u8 qgv_points_mask;
> +	u16 qgv_points_mask;

Weird, that this went unnoticed. Don't we have static analyzer for such
purposes? Wonder if it should catch and warn about this, because in
intel_bw_atomic_check we have u32 bitmask, which is then getting packed
in 8 bit field.
Probably bitmask type width used in intel_bw_atomic_check should match
that one, so that there would be less room for confusion.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  
>  	unsigned int data_rate[I915_MAX_PIPES];
>  	u8 num_active_planes[I915_MAX_PIPES];
>  
> -	/* bitmask of active pipes */
> -	u8 active_pipes;
> -
>  	int min_cdclk;
>  };
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Widen the QGV point mask

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:08AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> adlp+ adds some extra bits to the QGV point mask. The code attempts
> to handle that but forgot to actually make sure we can store those
> bits in the bw state. Fix it.
> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 192fbfb76744 ("drm/i915: Implement PSF GV point support")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 46c6eecbd917..0ceaed1c9656 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -30,19 +30,19 @@ struct intel_bw_state {
>  	 */
>  	u8 pipe_sagv_reject;
>  
> +	/* bitmask of active pipes */
> +	u8 active_pipes;
> +
>  	/*
>  	 * Current QGV points mask, which restricts
>  	 * some particular SAGV states, not to confuse
>  	 * with pipe_sagv_mask.
>  	 */
> -	u8 qgv_points_mask;
> +	u16 qgv_points_mask;

Weird, that this went unnoticed. Don't we have static analyzer for such
purposes? Wonder if it should catch and warn about this, because in
intel_bw_atomic_check we have u32 bitmask, which is then getting packed
in 8 bit field.
Probably bitmask type width used in intel_bw_atomic_check should match
that one, so that there would be less room for confusion.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

>  
>  	unsigned int data_rate[I915_MAX_PIPES];
>  	u8 num_active_planes[I915_MAX_PIPES];
>  
> -	/* bitmask of active pipes */
> -	u8 active_pipes;
> -
>  	int min_cdclk;
>  };
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 4/6] drm/i915: Unconfuse pre-icl vs. icl+ intel_sagv_{pre, post}_plane_update()

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:09AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_sagv_{pre,post}_plane_update() can accidentally forget
> to bail out early on pre-icl and proceed down the icl+ codepath
> at the end of the function. Fortunately it'll bail out before
> it gets too far due to old_qgv_mask==new_qgv_mask==0 so no real
> bug here. But lets make the code less confusing anyway.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d8eb553ffad3..068870b17c43 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3806,8 +3806,9 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11 && !intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		intel_disable_sagv(dev_priv);
> +	if (DISPLAY_VER(dev_priv) < 11) {
> +		if (!intel_can_enable_sagv(dev_priv, new_bw_state))
> +			intel_disable_sagv(dev_priv);
>  		return;
>  	}

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Agree, I think that was my original code as well. However to be honest, don't like
the whole idea of splitting the code flow and bailing our prematurely for gen < 11 here.
Would be nice to have some unified approach, so that we have common main logic for all platforms,
like 
if (intel_bw_state_equals(new, old))
	return

intel_bw_state_apply_restrictions(...)  -> here we would add intel_enable/disable_sagv for gen <11 and
                                           qgv point restrictions for gen >= 11

Stan

>  
> @@ -3857,8 +3858,9 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11 && intel_can_enable_sagv(dev_priv, new_bw_state)) {
> -		intel_enable_sagv(dev_priv);
> +	if (DISPLAY_VER(dev_priv) < 11) {
> +		if (intel_can_enable_sagv(dev_priv, new_bw_state))
> +			intel_enable_sagv(dev_priv);
>  		return;
>  	}
>  
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 12:27:30PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 12:00:11PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 11:18:11AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add some debugs on what exactly we're doing to the QGV point mask
> > > in the icl+ sagv pre/post plane update hooks. Currently we're just
> > > guessing.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++-----------------
> > >  1 file changed, 18 insertions(+), 19 deletions(-)
> > 
> > Weird I think, I had those debugs initially. Definitely remember
> > there was something similar. Was it kinda removed later?
> 
> Can't immediately see any such debugs being added or removed
> by any commit.

Definitely was there, either during restriction itself or in
intel_bw_atomic_check. Don't remember, if it was excessive debugs
someone was complaining or someother reason, why it was removed.

Stan

> 
> > 
> > Stan
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >  
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 8b70cdc3b58b..5d1f1a9988bb 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to mask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask;
> > >  	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  
> > > -	/*
> > > -	 * If new mask is zero - means there is nothing to mask,
> > > -	 * we can only unmask, which should be done in unmask.
> > > -	 */
> > > -	if (!new_mask)
> > > +	if (old_mask == new_mask)
> > >  		return;
> > >  
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Restrict required qgv points before updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > @@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask = 0;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to unmask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  	new_mask = new_bw_state->qgv_points_mask;
> > >  
> > > +	if (old_mask == new_mask)
> > > +		return;
> > > +
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Allow required qgv points after updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 6/6] drm/i915: Pimp icl+ sagv pre/post update

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 12:27:30PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 12:00:11PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 11:18:11AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add some debugs on what exactly we're doing to the QGV point mask
> > > in the icl+ sagv pre/post plane update hooks. Currently we're just
> > > guessing.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++-----------------
> > >  1 file changed, 18 insertions(+), 19 deletions(-)
> > 
> > Weird I think, I had those debugs initially. Definitely remember
> > there was something similar. Was it kinda removed later?
> 
> Can't immediately see any such debugs being added or removed
> by any commit.

Ah, now actually I remember. It was initially added to intel_bw_atomic_check
to show the new qgv mask, however in one of the reviews was asked to remove
that debug as redundant.

Stan

> 
> > 
> > Stan
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >  
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 8b70cdc3b58b..5d1f1a9988bb 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3818,26 +3818,22 @@ static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to mask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask;
> > >  	new_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  
> > > -	/*
> > > -	 * If new mask is zero - means there is nothing to mask,
> > > -	 * we can only unmask, which should be done in unmask.
> > > -	 */
> > > -	if (!new_mask)
> > > +	if (old_mask == new_mask)
> > >  		return;
> > >  
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Restricting QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Restrict required qgv points before updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > @@ -3854,19 +3850,22 @@ static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
> > >  		intel_atomic_get_old_bw_state(state);
> > >  	const struct intel_bw_state *new_bw_state =
> > >  		intel_atomic_get_new_bw_state(state);
> > > -	u32 new_mask = 0;
> > > +	u32 old_mask, new_mask;
> > >  
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	/*
> > > -	 * Nothing to unmask
> > > -	 */
> > > -	if (new_bw_state->qgv_points_mask == old_bw_state->qgv_points_mask)
> > > -		return;
> > > -
> > > +	old_mask = old_bw_state->qgv_points_mask | new_bw_state->qgv_points_mask;
> > >  	new_mask = new_bw_state->qgv_points_mask;
> > >  
> > > +	if (old_mask == new_mask)
> > > +		return;
> > > +
> > > +	WARN_ON(!new_bw_state->base.changed);
> > > +
> > > +	drm_dbg_kms(&dev_priv->drm, "Relaxing QGV points: 0x%x -> 0x%x\n",
> > > +		    old_mask, new_mask);
> > > +
> > >  	/*
> > >  	 * Allow required qgv points after updating the configuration.
> > >  	 * According to BSpec we can't mask and unmask qgv points at the same
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > the number of active planes and the total data rates end up
> > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > early and forget to actually compute the new WGV point
> > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > This ends up poorly if we end up running with SAGV enabled
> > > > when we shouldn't. Usually ends up in underruns.
> > > > To fix this let's go through the QGV point mask computation
> > > > if anyone else already added the bw state for us.
> > > 
> > > Haven't been looking this in a while. Despite we have been
> > > looking like few revisions together still some bugs :(
> > > 
> > > I thought SAGV vs No SAGV can't change if active planes 
> > > or data rate didn't change? Because it means we probably
> > > still have same ddb allocations, which means SAGV state
> > > will just stay the same.
> > 
> > SAGV can change due to watermarks/ddb allocations. The easiest
> > way to trip this up is to try to use the async flip wm0/ddb 
> > optimization. That immediately forgets to turn off SAGV and
> > we get underruns, whcih is how I noticed this. And I don't
> > immediately see any easy proof that this couldn't also happen
> > due to some other plane changes.
> 
> Thats the way it was initially implemented even before SAGV was added.

Yeah, it wasn't a problem as long as SAGV was not enabled.

> I think it can be dated back to the very first bw check was implemented.
> 
> commit c457d9cf256e942138a54a2e80349ee7fe20c391
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Fri May 24 18:36:14 2019 +0300
> 
>     drm/i915: Make sure we have enough memory bandwidth on ICL
> 
> +int intel_bw_atomic_check(struct intel_atomic_state *state)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> +       struct intel_bw_state *bw_state = NULL;
> +       unsigned int data_rate, max_data_rate;
> +       unsigned int num_active_planes;
> +       struct intel_crtc *crtc;
> +       int i;
> +
> +       /* FIXME earlier gens need some checks too */
> +       if (INTEL_GEN(dev_priv) < 11)
> +               return 0;
> +
> +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +                                           new_crtc_state, i) {
> +               unsigned int old_data_rate =
> +                       intel_bw_crtc_data_rate(old_crtc_state);
> +               unsigned int new_data_rate =
> +                       intel_bw_crtc_data_rate(new_crtc_state);
> +               unsigned int old_active_planes =
> +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> +               unsigned int new_active_planes =
> +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> +
> +               /*
> +                * Avoid locking the bw state when
> +                * nothing significant has changed.
> +                */
> +               if (old_data_rate == new_data_rate &&
> +                   old_active_planes == new_active_planes)
> +                       continue;
> +
> +               bw_state  = intel_atomic_get_bw_state(state);
> +               if (IS_ERR(bw_state))
> +                       return PTR_ERR(bw_state);
> 
> However, what can cause watermarks/ddb to change, besides plane state change
> and/or active planes change? We change watermarks, when we change ddb allocations
> and we change ddb allocations when active planes had changed and/or data rate
> had changed.

The bw code only cares about the aggregate numbers from all the planes.
The planes could still change in some funny way where eg. some plane
frees up some bandwidth, but the other planes gobble up the exact same
amount and thus the aggregate numbers the bw atomic check cares about
do not change but the watermarks/ddb do.

And as mentiioned, the async flip wm0/ddb optimization makes this trivial
to trip up since it will want to disable SAGV as there is not enough ddb
for the SAGV watermark. And async flip specifically isn't even allowed
to change anything that would affect the bandwidth utilization, and neither
is it allowed to enable/disable planes.

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > the number of active planes and the total data rates end up
> > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > early and forget to actually compute the new WGV point
> > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > This ends up poorly if we end up running with SAGV enabled
> > > > when we shouldn't. Usually ends up in underruns.
> > > > To fix this let's go through the QGV point mask computation
> > > > if anyone else already added the bw state for us.
> > > 
> > > Haven't been looking this in a while. Despite we have been
> > > looking like few revisions together still some bugs :(
> > > 
> > > I thought SAGV vs No SAGV can't change if active planes 
> > > or data rate didn't change? Because it means we probably
> > > still have same ddb allocations, which means SAGV state
> > > will just stay the same.
> > 
> > SAGV can change due to watermarks/ddb allocations. The easiest
> > way to trip this up is to try to use the async flip wm0/ddb 
> > optimization. That immediately forgets to turn off SAGV and
> > we get underruns, whcih is how I noticed this. And I don't
> > immediately see any easy proof that this couldn't also happen
> > due to some other plane changes.
> 
> Thats the way it was initially implemented even before SAGV was added.

Yeah, it wasn't a problem as long as SAGV was not enabled.

> I think it can be dated back to the very first bw check was implemented.
> 
> commit c457d9cf256e942138a54a2e80349ee7fe20c391
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Fri May 24 18:36:14 2019 +0300
> 
>     drm/i915: Make sure we have enough memory bandwidth on ICL
> 
> +int intel_bw_atomic_check(struct intel_atomic_state *state)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> +       struct intel_bw_state *bw_state = NULL;
> +       unsigned int data_rate, max_data_rate;
> +       unsigned int num_active_planes;
> +       struct intel_crtc *crtc;
> +       int i;
> +
> +       /* FIXME earlier gens need some checks too */
> +       if (INTEL_GEN(dev_priv) < 11)
> +               return 0;
> +
> +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +                                           new_crtc_state, i) {
> +               unsigned int old_data_rate =
> +                       intel_bw_crtc_data_rate(old_crtc_state);
> +               unsigned int new_data_rate =
> +                       intel_bw_crtc_data_rate(new_crtc_state);
> +               unsigned int old_active_planes =
> +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> +               unsigned int new_active_planes =
> +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> +
> +               /*
> +                * Avoid locking the bw state when
> +                * nothing significant has changed.
> +                */
> +               if (old_data_rate == new_data_rate &&
> +                   old_active_planes == new_active_planes)
> +                       continue;
> +
> +               bw_state  = intel_atomic_get_bw_state(state);
> +               if (IS_ERR(bw_state))
> +                       return PTR_ERR(bw_state);
> 
> However, what can cause watermarks/ddb to change, besides plane state change
> and/or active planes change? We change watermarks, when we change ddb allocations
> and we change ddb allocations when active planes had changed and/or data rate
> had changed.

The bw code only cares about the aggregate numbers from all the planes.
The planes could still change in some funny way where eg. some plane
frees up some bandwidth, but the other planes gobble up the exact same
amount and thus the aggregate numbers the bw atomic check cares about
do not change but the watermarks/ddb do.

And as mentiioned, the async flip wm0/ddb optimization makes this trivial
to trip up since it will want to disable SAGV as there is not enough ddb
for the SAGV watermark. And async flip specifically isn't even allowed
to change anything that would affect the bandwidth utilization, and neither
is it allowed to enable/disable planes.

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > the number of active planes and the total data rates end up
> > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > early and forget to actually compute the new WGV point
> > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > when we shouldn't. Usually ends up in underruns.
> > > > > To fix this let's go through the QGV point mask computation
> > > > > if anyone else already added the bw state for us.
> > > > 
> > > > Haven't been looking this in a while. Despite we have been
> > > > looking like few revisions together still some bugs :(
> > > > 
> > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > or data rate didn't change? Because it means we probably
> > > > still have same ddb allocations, which means SAGV state
> > > > will just stay the same.
> > > 
> > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > way to trip this up is to try to use the async flip wm0/ddb 
> > > optimization. That immediately forgets to turn off SAGV and
> > > we get underruns, whcih is how I noticed this. And I don't
> > > immediately see any easy proof that this couldn't also happen
> > > due to some other plane changes.
> > 
> > Thats the way it was initially implemented even before SAGV was added.
> 
> Yeah, it wasn't a problem as long as SAGV was not enabled.
> 
> > I think it can be dated back to the very first bw check was implemented.
> > 
> > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Fri May 24 18:36:14 2019 +0300
> > 
> >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > 
> > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > +{
> > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > +       struct intel_bw_state *bw_state = NULL;
> > +       unsigned int data_rate, max_data_rate;
> > +       unsigned int num_active_planes;
> > +       struct intel_crtc *crtc;
> > +       int i;
> > +
> > +       /* FIXME earlier gens need some checks too */
> > +       if (INTEL_GEN(dev_priv) < 11)
> > +               return 0;
> > +
> > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +                                           new_crtc_state, i) {
> > +               unsigned int old_data_rate =
> > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > +               unsigned int new_data_rate =
> > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > +               unsigned int old_active_planes =
> > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > +               unsigned int new_active_planes =
> > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > +
> > +               /*
> > +                * Avoid locking the bw state when
> > +                * nothing significant has changed.
> > +                */
> > +               if (old_data_rate == new_data_rate &&
> > +                   old_active_planes == new_active_planes)
> > +                       continue;
> > +
> > +               bw_state  = intel_atomic_get_bw_state(state);
> > +               if (IS_ERR(bw_state))
> > +                       return PTR_ERR(bw_state);
> > 
> > However, what can cause watermarks/ddb to change, besides plane state change
> > and/or active planes change? We change watermarks, when we change ddb allocations
> > and we change ddb allocations when active planes had changed and/or data rate
> > had changed.
> 
> The bw code only cares about the aggregate numbers from all the planes.
> The planes could still change in some funny way where eg. some plane
> frees up some bandwidth, but the other planes gobble up the exact same
> amount and thus the aggregate numbers the bw atomic check cares about
> do not change but the watermarks/ddb do.
> 
> And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> to trip up since it will want to disable SAGV as there is not enough ddb
> for the SAGV watermark. And async flip specifically isn't even allowed
> to change anything that would affect the bandwidth utilization, and neither
> is it allowed to enable/disable planes.

I think the whole idea of setting ddb to minimum in case of async flip optimization
was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
flip, however there is nothing about limiting ddb allocations.

Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
cause some unexpected side effects.

Also we are now forcing the recalculation to be done always no matter what and using
new bw state for that in a bit counterintuitive way, which I don't like. 
Not even sure that will always work, as we are not guaranteed to get a non-NULL
new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
done right after this one.

So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
- I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.

Stan

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

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > the number of active planes and the total data rates end up
> > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > early and forget to actually compute the new WGV point
> > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > when we shouldn't. Usually ends up in underruns.
> > > > > To fix this let's go through the QGV point mask computation
> > > > > if anyone else already added the bw state for us.
> > > > 
> > > > Haven't been looking this in a while. Despite we have been
> > > > looking like few revisions together still some bugs :(
> > > > 
> > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > or data rate didn't change? Because it means we probably
> > > > still have same ddb allocations, which means SAGV state
> > > > will just stay the same.
> > > 
> > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > way to trip this up is to try to use the async flip wm0/ddb 
> > > optimization. That immediately forgets to turn off SAGV and
> > > we get underruns, whcih is how I noticed this. And I don't
> > > immediately see any easy proof that this couldn't also happen
> > > due to some other plane changes.
> > 
> > Thats the way it was initially implemented even before SAGV was added.
> 
> Yeah, it wasn't a problem as long as SAGV was not enabled.
> 
> > I think it can be dated back to the very first bw check was implemented.
> > 
> > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Date:   Fri May 24 18:36:14 2019 +0300
> > 
> >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > 
> > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > +{
> > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > +       struct intel_bw_state *bw_state = NULL;
> > +       unsigned int data_rate, max_data_rate;
> > +       unsigned int num_active_planes;
> > +       struct intel_crtc *crtc;
> > +       int i;
> > +
> > +       /* FIXME earlier gens need some checks too */
> > +       if (INTEL_GEN(dev_priv) < 11)
> > +               return 0;
> > +
> > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +                                           new_crtc_state, i) {
> > +               unsigned int old_data_rate =
> > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > +               unsigned int new_data_rate =
> > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > +               unsigned int old_active_planes =
> > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > +               unsigned int new_active_planes =
> > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > +
> > +               /*
> > +                * Avoid locking the bw state when
> > +                * nothing significant has changed.
> > +                */
> > +               if (old_data_rate == new_data_rate &&
> > +                   old_active_planes == new_active_planes)
> > +                       continue;
> > +
> > +               bw_state  = intel_atomic_get_bw_state(state);
> > +               if (IS_ERR(bw_state))
> > +                       return PTR_ERR(bw_state);
> > 
> > However, what can cause watermarks/ddb to change, besides plane state change
> > and/or active planes change? We change watermarks, when we change ddb allocations
> > and we change ddb allocations when active planes had changed and/or data rate
> > had changed.
> 
> The bw code only cares about the aggregate numbers from all the planes.
> The planes could still change in some funny way where eg. some plane
> frees up some bandwidth, but the other planes gobble up the exact same
> amount and thus the aggregate numbers the bw atomic check cares about
> do not change but the watermarks/ddb do.
> 
> And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> to trip up since it will want to disable SAGV as there is not enough ddb
> for the SAGV watermark. And async flip specifically isn't even allowed
> to change anything that would affect the bandwidth utilization, and neither
> is it allowed to enable/disable planes.

I think the whole idea of setting ddb to minimum in case of async flip optimization
was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
flip, however there is nothing about limiting ddb allocations.

Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
cause some unexpected side effects.

Also we are now forcing the recalculation to be done always no matter what and using
new bw state for that in a bit counterintuitive way, which I don't like. 
Not even sure that will always work, as we are not guaranteed to get a non-NULL
new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
done right after this one.

So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
- I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.

Stan

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

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > the number of active planes and the total data rates end up
> > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > early and forget to actually compute the new WGV point
> > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > To fix this let's go through the QGV point mask computation
> > > > > > if anyone else already added the bw state for us.
> > > > > 
> > > > > Haven't been looking this in a while. Despite we have been
> > > > > looking like few revisions together still some bugs :(
> > > > > 
> > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > or data rate didn't change? Because it means we probably
> > > > > still have same ddb allocations, which means SAGV state
> > > > > will just stay the same.
> > > > 
> > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > optimization. That immediately forgets to turn off SAGV and
> > > > we get underruns, whcih is how I noticed this. And I don't
> > > > immediately see any easy proof that this couldn't also happen
> > > > due to some other plane changes.
> > > 
> > > Thats the way it was initially implemented even before SAGV was added.
> > 
> > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > 
> > > I think it can be dated back to the very first bw check was implemented.
> > > 
> > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Date:   Fri May 24 18:36:14 2019 +0300
> > > 
> > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > 
> > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > +       struct intel_bw_state *bw_state = NULL;
> > > +       unsigned int data_rate, max_data_rate;
> > > +       unsigned int num_active_planes;
> > > +       struct intel_crtc *crtc;
> > > +       int i;
> > > +
> > > +       /* FIXME earlier gens need some checks too */
> > > +       if (INTEL_GEN(dev_priv) < 11)
> > > +               return 0;
> > > +
> > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > +                                           new_crtc_state, i) {
> > > +               unsigned int old_data_rate =
> > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > +               unsigned int new_data_rate =
> > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > +               unsigned int old_active_planes =
> > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > +               unsigned int new_active_planes =
> > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > +
> > > +               /*
> > > +                * Avoid locking the bw state when
> > > +                * nothing significant has changed.
> > > +                */
> > > +               if (old_data_rate == new_data_rate &&
> > > +                   old_active_planes == new_active_planes)
> > > +                       continue;
> > > +
> > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > +               if (IS_ERR(bw_state))
> > > +                       return PTR_ERR(bw_state);
> > > 
> > > However, what can cause watermarks/ddb to change, besides plane state change
> > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > and we change ddb allocations when active planes had changed and/or data rate
> > > had changed.
> > 
> > The bw code only cares about the aggregate numbers from all the planes.
> > The planes could still change in some funny way where eg. some plane
> > frees up some bandwidth, but the other planes gobble up the exact same
> > amount and thus the aggregate numbers the bw atomic check cares about
> > do not change but the watermarks/ddb do.
> > 
> > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > to trip up since it will want to disable SAGV as there is not enough ddb
> > for the SAGV watermark. And async flip specifically isn't even allowed
> > to change anything that would affect the bandwidth utilization, and neither
> > is it allowed to enable/disable planes.
> 
> I think the whole idea of setting ddb to minimum in case of async flip optimization
> was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> flip, however there is nothing about limiting ddb allocations.

Reducing just the watermark doesn't really make sense 
if the goal is to keep the DBUF level to a minimum. Also
I don't think there is any proper docs for this thing. The
only thing we have just has some vague notes about using
"minimum watermarks", whatever that means.

> 
> Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> cause some unexpected side effects.

The bw_state vs. SAGV bug is there regardless of the wm0 optimization.

Also the SAGV watermark is not the minimum watermark (if that is
the doc really means by that), the normal WM0 is the minimum watermark.
So even if we interpret the doc to say that we should just disable all
watermark levels except the smallest one (normal WM0) without changing
the ddb allocations we would still end up disabling SAGV.

> Also we are now forcing the recalculation to be done always no matter what and using
> new bw state for that in a bit counterintuitive way, which I don't like. 
> Not even sure that will always work, as we are not guaranteed to get a non-NULL
> new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> done right after this one.

If there is no bw_state then bw_state->pipe_sagv_reject can't have
changed and there is nothing to recalculate.

> 
> So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.

Yes, NULL is perfectly fine.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > the number of active planes and the total data rates end up
> > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > early and forget to actually compute the new WGV point
> > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > To fix this let's go through the QGV point mask computation
> > > > > > if anyone else already added the bw state for us.
> > > > > 
> > > > > Haven't been looking this in a while. Despite we have been
> > > > > looking like few revisions together still some bugs :(
> > > > > 
> > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > or data rate didn't change? Because it means we probably
> > > > > still have same ddb allocations, which means SAGV state
> > > > > will just stay the same.
> > > > 
> > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > optimization. That immediately forgets to turn off SAGV and
> > > > we get underruns, whcih is how I noticed this. And I don't
> > > > immediately see any easy proof that this couldn't also happen
> > > > due to some other plane changes.
> > > 
> > > Thats the way it was initially implemented even before SAGV was added.
> > 
> > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > 
> > > I think it can be dated back to the very first bw check was implemented.
> > > 
> > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Date:   Fri May 24 18:36:14 2019 +0300
> > > 
> > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > 
> > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > +{
> > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > +       struct intel_bw_state *bw_state = NULL;
> > > +       unsigned int data_rate, max_data_rate;
> > > +       unsigned int num_active_planes;
> > > +       struct intel_crtc *crtc;
> > > +       int i;
> > > +
> > > +       /* FIXME earlier gens need some checks too */
> > > +       if (INTEL_GEN(dev_priv) < 11)
> > > +               return 0;
> > > +
> > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > +                                           new_crtc_state, i) {
> > > +               unsigned int old_data_rate =
> > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > +               unsigned int new_data_rate =
> > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > +               unsigned int old_active_planes =
> > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > +               unsigned int new_active_planes =
> > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > +
> > > +               /*
> > > +                * Avoid locking the bw state when
> > > +                * nothing significant has changed.
> > > +                */
> > > +               if (old_data_rate == new_data_rate &&
> > > +                   old_active_planes == new_active_planes)
> > > +                       continue;
> > > +
> > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > +               if (IS_ERR(bw_state))
> > > +                       return PTR_ERR(bw_state);
> > > 
> > > However, what can cause watermarks/ddb to change, besides plane state change
> > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > and we change ddb allocations when active planes had changed and/or data rate
> > > had changed.
> > 
> > The bw code only cares about the aggregate numbers from all the planes.
> > The planes could still change in some funny way where eg. some plane
> > frees up some bandwidth, but the other planes gobble up the exact same
> > amount and thus the aggregate numbers the bw atomic check cares about
> > do not change but the watermarks/ddb do.
> > 
> > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > to trip up since it will want to disable SAGV as there is not enough ddb
> > for the SAGV watermark. And async flip specifically isn't even allowed
> > to change anything that would affect the bandwidth utilization, and neither
> > is it allowed to enable/disable planes.
> 
> I think the whole idea of setting ddb to minimum in case of async flip optimization
> was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> flip, however there is nothing about limiting ddb allocations.

Reducing just the watermark doesn't really make sense 
if the goal is to keep the DBUF level to a minimum. Also
I don't think there is any proper docs for this thing. The
only thing we have just has some vague notes about using
"minimum watermarks", whatever that means.

> 
> Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> cause some unexpected side effects.

The bw_state vs. SAGV bug is there regardless of the wm0 optimization.

Also the SAGV watermark is not the minimum watermark (if that is
the doc really means by that), the normal WM0 is the minimum watermark.
So even if we interpret the doc to say that we should just disable all
watermark levels except the smallest one (normal WM0) without changing
the ddb allocations we would still end up disabling SAGV.

> Also we are now forcing the recalculation to be done always no matter what and using
> new bw state for that in a bit counterintuitive way, which I don't like. 
> Not even sure that will always work, as we are not guaranteed to get a non-NULL
> new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> done right after this one.

If there is no bw_state then bw_state->pipe_sagv_reject can't have
changed and there is nothing to recalculate.

> 
> So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.

Yes, NULL is perfectly fine.

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > 
> > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > the number of active planes and the total data rates end up
> > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > early and forget to actually compute the new WGV point
> > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > if anyone else already added the bw state for us.
> > > > > > 
> > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > looking like few revisions together still some bugs :(
> > > > > > 
> > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > or data rate didn't change? Because it means we probably
> > > > > > still have same ddb allocations, which means SAGV state
> > > > > > will just stay the same.
> > > > > 
> > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > immediately see any easy proof that this couldn't also happen
> > > > > due to some other plane changes.
> > > > 
> > > > Thats the way it was initially implemented even before SAGV was added.
> > > 
> > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > 
> > > > I think it can be dated back to the very first bw check was implemented.
> > > > 
> > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > 
> > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > 
> > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > +{
> > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > +       struct intel_bw_state *bw_state = NULL;
> > > > +       unsigned int data_rate, max_data_rate;
> > > > +       unsigned int num_active_planes;
> > > > +       struct intel_crtc *crtc;
> > > > +       int i;
> > > > +
> > > > +       /* FIXME earlier gens need some checks too */
> > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > +               return 0;
> > > > +
> > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > +                                           new_crtc_state, i) {
> > > > +               unsigned int old_data_rate =
> > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > +               unsigned int new_data_rate =
> > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > +               unsigned int old_active_planes =
> > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > +               unsigned int new_active_planes =
> > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > +
> > > > +               /*
> > > > +                * Avoid locking the bw state when
> > > > +                * nothing significant has changed.
> > > > +                */
> > > > +               if (old_data_rate == new_data_rate &&
> > > > +                   old_active_planes == new_active_planes)
> > > > +                       continue;
> > > > +
> > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > +               if (IS_ERR(bw_state))
> > > > +                       return PTR_ERR(bw_state);
> > > > 
> > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > had changed.
> > > 
> > > The bw code only cares about the aggregate numbers from all the planes.
> > > The planes could still change in some funny way where eg. some plane
> > > frees up some bandwidth, but the other planes gobble up the exact same
> > > amount and thus the aggregate numbers the bw atomic check cares about
> > > do not change but the watermarks/ddb do.
> > > 
> > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > to change anything that would affect the bandwidth utilization, and neither
> > > is it allowed to enable/disable planes.
> > 
> > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > flip, however there is nothing about limiting ddb allocations.
> 
> Reducing just the watermark doesn't really make sense 
> if the goal is to keep the DBUF level to a minimum. Also
> I don't think there is any proper docs for this thing. The
> only thing we have just has some vague notes about using
> "minimum watermarks", whatever that means.

Was it the goal? I thought limiting watermarks would by itself also
limit package C states, thus affecting memory clocks and latency.
Because it really doesn't say anything about keeping Dbuf allocations
to a minimum. 

> 
> > 
> > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > cause some unexpected side effects.
> 
> The bw_state vs. SAGV bug is there regardless of the wm0 optimization.

I agree there is a bug. The bug is such that initial bw checks were relying
on total data rate + active planes comparison, while it should have accounted
data rate per plane usage.

This should have been changed in SAGV patches, but probably had gone
unnoticed both by you and me.

> 
> Also the SAGV watermark is not the minimum watermark (if that is
> the doc really means by that), the normal WM0 is the minimum watermark.
> So even if we interpret the doc to say that we should just disable all
> watermark levels except the smallest one (normal WM0) without changing
> the ddb allocations we would still end up disabling SAGV.

Thats actually a good question. Did they mean, disable all "regular" wm levels
or the SAGV one also? Probably they meant what you say, but would be nice to know
exactly.

Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
check that plane allocations had changed. Thats just confusing.

May be for you as i915 guru, thats obvious however not for someone else, who might
touch the code and we are doing open source here.

Can we just add some check which explicitly does per plane data rate checks?

So that we know bail out from that first cycle not only when total_data_rate/active planes
had changed, but we check per plane data rate? 
That might actually save us also in future, if we ever get into such situation, when
bw_state doesn't change, but ddb allocations do.

I know you might say it shouldn't happen, but there is always some new stuff coming.

Stan

> 
> > Also we are now forcing the recalculation to be done always no matter what and using
> > new bw state for that in a bit counterintuitive way, which I don't like. 
> > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > done right after this one.
> 
> If there is no bw_state then bw_state->pipe_sagv_reject can't have
> changed and there is nothing to recalculate.
> 
> > 
> > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> 
> Yes, NULL is perfectly fine.
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > 
> > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > the number of active planes and the total data rates end up
> > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > early and forget to actually compute the new WGV point
> > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > if anyone else already added the bw state for us.
> > > > > > 
> > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > looking like few revisions together still some bugs :(
> > > > > > 
> > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > or data rate didn't change? Because it means we probably
> > > > > > still have same ddb allocations, which means SAGV state
> > > > > > will just stay the same.
> > > > > 
> > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > immediately see any easy proof that this couldn't also happen
> > > > > due to some other plane changes.
> > > > 
> > > > Thats the way it was initially implemented even before SAGV was added.
> > > 
> > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > 
> > > > I think it can be dated back to the very first bw check was implemented.
> > > > 
> > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > 
> > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > 
> > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > +{
> > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > +       struct intel_bw_state *bw_state = NULL;
> > > > +       unsigned int data_rate, max_data_rate;
> > > > +       unsigned int num_active_planes;
> > > > +       struct intel_crtc *crtc;
> > > > +       int i;
> > > > +
> > > > +       /* FIXME earlier gens need some checks too */
> > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > +               return 0;
> > > > +
> > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > +                                           new_crtc_state, i) {
> > > > +               unsigned int old_data_rate =
> > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > +               unsigned int new_data_rate =
> > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > +               unsigned int old_active_planes =
> > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > +               unsigned int new_active_planes =
> > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > +
> > > > +               /*
> > > > +                * Avoid locking the bw state when
> > > > +                * nothing significant has changed.
> > > > +                */
> > > > +               if (old_data_rate == new_data_rate &&
> > > > +                   old_active_planes == new_active_planes)
> > > > +                       continue;
> > > > +
> > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > +               if (IS_ERR(bw_state))
> > > > +                       return PTR_ERR(bw_state);
> > > > 
> > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > had changed.
> > > 
> > > The bw code only cares about the aggregate numbers from all the planes.
> > > The planes could still change in some funny way where eg. some plane
> > > frees up some bandwidth, but the other planes gobble up the exact same
> > > amount and thus the aggregate numbers the bw atomic check cares about
> > > do not change but the watermarks/ddb do.
> > > 
> > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > to change anything that would affect the bandwidth utilization, and neither
> > > is it allowed to enable/disable planes.
> > 
> > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > flip, however there is nothing about limiting ddb allocations.
> 
> Reducing just the watermark doesn't really make sense 
> if the goal is to keep the DBUF level to a minimum. Also
> I don't think there is any proper docs for this thing. The
> only thing we have just has some vague notes about using
> "minimum watermarks", whatever that means.

Was it the goal? I thought limiting watermarks would by itself also
limit package C states, thus affecting memory clocks and latency.
Because it really doesn't say anything about keeping Dbuf allocations
to a minimum. 

> 
> > 
> > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > cause some unexpected side effects.
> 
> The bw_state vs. SAGV bug is there regardless of the wm0 optimization.

I agree there is a bug. The bug is such that initial bw checks were relying
on total data rate + active planes comparison, while it should have accounted
data rate per plane usage.

This should have been changed in SAGV patches, but probably had gone
unnoticed both by you and me.

> 
> Also the SAGV watermark is not the minimum watermark (if that is
> the doc really means by that), the normal WM0 is the minimum watermark.
> So even if we interpret the doc to say that we should just disable all
> watermark levels except the smallest one (normal WM0) without changing
> the ddb allocations we would still end up disabling SAGV.

Thats actually a good question. Did they mean, disable all "regular" wm levels
or the SAGV one also? Probably they meant what you say, but would be nice to know
exactly.

Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
check that plane allocations had changed. Thats just confusing.

May be for you as i915 guru, thats obvious however not for someone else, who might
touch the code and we are doing open source here.

Can we just add some check which explicitly does per plane data rate checks?

So that we know bail out from that first cycle not only when total_data_rate/active planes
had changed, but we check per plane data rate? 
That might actually save us also in future, if we ever get into such situation, when
bw_state doesn't change, but ddb allocations do.

I know you might say it shouldn't happen, but there is always some new stuff coming.

Stan

> 
> > Also we are now forcing the recalculation to be done always no matter what and using
> > new bw state for that in a bit counterintuitive way, which I don't like. 
> > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > done right after this one.
> 
> If there is no bw_state then bw_state->pipe_sagv_reject can't have
> changed and there is nothing to recalculate.
> 
> > 
> > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> 
> Yes, NULL is perfectly fine.
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > 
> > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > if anyone else already added the bw state for us.
> > > > > > > 
> > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > looking like few revisions together still some bugs :(
> > > > > > > 
> > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > will just stay the same.
> > > > > > 
> > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > due to some other plane changes.
> > > > > 
> > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > 
> > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > 
> > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > 
> > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > 
> > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > 
> > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > +{
> > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > +       unsigned int data_rate, max_data_rate;
> > > > > +       unsigned int num_active_planes;
> > > > > +       struct intel_crtc *crtc;
> > > > > +       int i;
> > > > > +
> > > > > +       /* FIXME earlier gens need some checks too */
> > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > +               return 0;
> > > > > +
> > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > +                                           new_crtc_state, i) {
> > > > > +               unsigned int old_data_rate =
> > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > +               unsigned int new_data_rate =
> > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > +               unsigned int old_active_planes =
> > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > +               unsigned int new_active_planes =
> > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > +
> > > > > +               /*
> > > > > +                * Avoid locking the bw state when
> > > > > +                * nothing significant has changed.
> > > > > +                */
> > > > > +               if (old_data_rate == new_data_rate &&
> > > > > +                   old_active_planes == new_active_planes)
> > > > > +                       continue;
> > > > > +
> > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > +               if (IS_ERR(bw_state))
> > > > > +                       return PTR_ERR(bw_state);
> > > > > 
> > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > had changed.
> > > > 
> > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > The planes could still change in some funny way where eg. some plane
> > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > do not change but the watermarks/ddb do.
> > > > 
> > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > to change anything that would affect the bandwidth utilization, and neither
> > > > is it allowed to enable/disable planes.
> > > 
> > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > flip, however there is nothing about limiting ddb allocations.
> > 
> > Reducing just the watermark doesn't really make sense 
> > if the goal is to keep the DBUF level to a minimum. Also
> > I don't think there is any proper docs for this thing. The
> > only thing we have just has some vague notes about using
> > "minimum watermarks", whatever that means.
> 
> Was it the goal? I thought limiting watermarks would by itself also
> limit package C states, thus affecting memory clocks and latency.
> Because it really doesn't say anything about keeping Dbuf allocations
> to a minimum. 

The goal is to miminize the amount of data in the FIFO.

> 
> > 
> > > 
> > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > cause some unexpected side effects.
> > 
> > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> 
> I agree there is a bug. The bug is such that initial bw checks were relying
> on total data rate + active planes comparison, while it should have accounted
> data rate per plane usage.
> 
> This should have been changed in SAGV patches, but probably had gone
> unnoticed both by you and me.
> 
> > 
> > Also the SAGV watermark is not the minimum watermark (if that is
> > the doc really means by that), the normal WM0 is the minimum watermark.
> > So even if we interpret the doc to say that we should just disable all
> > watermark levels except the smallest one (normal WM0) without changing
> > the ddb allocations we would still end up disabling SAGV.
> 
> Thats actually a good question. Did they mean, disable all "regular" wm levels
> or the SAGV one also? Probably they meant what you say, but would be nice to know
> exactly.

They said neither. It's just "program minimum watermarks" which
could mean anything really. They do explicitly say "DBUF level
can also adversely affect flip performance." which I think is
the whole point of this exercise.

> 
> Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> check that plane allocations had changed. Thats just confusing.

We are not checking if plane allocations have changed. We are
trying to determine if anything in the bw_state has changed.
If we have said state already then something in it may have 
changed and we have to recalculate anything that may depend
on those changed things, namely pipe_sagv_reject->qgv_point_mask.

I think ideally we'd not even modify the bw_state directly from the
watermark code and we'd instead defer that to bw atomic check entirely.
But this SAGV vs. DDB business is your typical chicken vs. egg situation,
so I'm not sure that is possible to do. Would need to spend a few minutes
thinking about it I guess.

> 
> May be for you as i915 guru, thats obvious however not for someone else, who might
> touch the code and we are doing open source here.
> 
> Can we just add some check which explicitly does per plane data rate checks?

There is nothing interesting about per-plane data rates.

> So that we know bail out from that first cycle not only when total_data_rate/active planes
> had changed, but we check per plane data rate? 
> That might actually save us also in future, if we ever get into such situation, when
> bw_state doesn't change, but ddb allocations do.
> 
> I know you might say it shouldn't happen, but there is always some new stuff coming.
> 
> Stan
> 
> > 
> > > Also we are now forcing the recalculation to be done always no matter what and using
> > > new bw state for that in a bit counterintuitive way, which I don't like. 
> > > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > > done right after this one.
> > 
> > If there is no bw_state then bw_state->pipe_sagv_reject can't have
> > changed and there is nothing to recalculate.
> > 
> > > 
> > > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> > 
> > Yes, NULL is perfectly fine.
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > 
> > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > if anyone else already added the bw state for us.
> > > > > > > 
> > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > looking like few revisions together still some bugs :(
> > > > > > > 
> > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > will just stay the same.
> > > > > > 
> > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > due to some other plane changes.
> > > > > 
> > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > 
> > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > 
> > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > 
> > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > 
> > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > 
> > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > +{
> > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > +       unsigned int data_rate, max_data_rate;
> > > > > +       unsigned int num_active_planes;
> > > > > +       struct intel_crtc *crtc;
> > > > > +       int i;
> > > > > +
> > > > > +       /* FIXME earlier gens need some checks too */
> > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > +               return 0;
> > > > > +
> > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > +                                           new_crtc_state, i) {
> > > > > +               unsigned int old_data_rate =
> > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > +               unsigned int new_data_rate =
> > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > +               unsigned int old_active_planes =
> > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > +               unsigned int new_active_planes =
> > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > +
> > > > > +               /*
> > > > > +                * Avoid locking the bw state when
> > > > > +                * nothing significant has changed.
> > > > > +                */
> > > > > +               if (old_data_rate == new_data_rate &&
> > > > > +                   old_active_planes == new_active_planes)
> > > > > +                       continue;
> > > > > +
> > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > +               if (IS_ERR(bw_state))
> > > > > +                       return PTR_ERR(bw_state);
> > > > > 
> > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > had changed.
> > > > 
> > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > The planes could still change in some funny way where eg. some plane
> > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > do not change but the watermarks/ddb do.
> > > > 
> > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > to change anything that would affect the bandwidth utilization, and neither
> > > > is it allowed to enable/disable planes.
> > > 
> > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > flip, however there is nothing about limiting ddb allocations.
> > 
> > Reducing just the watermark doesn't really make sense 
> > if the goal is to keep the DBUF level to a minimum. Also
> > I don't think there is any proper docs for this thing. The
> > only thing we have just has some vague notes about using
> > "minimum watermarks", whatever that means.
> 
> Was it the goal? I thought limiting watermarks would by itself also
> limit package C states, thus affecting memory clocks and latency.
> Because it really doesn't say anything about keeping Dbuf allocations
> to a minimum. 

The goal is to miminize the amount of data in the FIFO.

> 
> > 
> > > 
> > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > cause some unexpected side effects.
> > 
> > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> 
> I agree there is a bug. The bug is such that initial bw checks were relying
> on total data rate + active planes comparison, while it should have accounted
> data rate per plane usage.
> 
> This should have been changed in SAGV patches, but probably had gone
> unnoticed both by you and me.
> 
> > 
> > Also the SAGV watermark is not the minimum watermark (if that is
> > the doc really means by that), the normal WM0 is the minimum watermark.
> > So even if we interpret the doc to say that we should just disable all
> > watermark levels except the smallest one (normal WM0) without changing
> > the ddb allocations we would still end up disabling SAGV.
> 
> Thats actually a good question. Did they mean, disable all "regular" wm levels
> or the SAGV one also? Probably they meant what you say, but would be nice to know
> exactly.

They said neither. It's just "program minimum watermarks" which
could mean anything really. They do explicitly say "DBUF level
can also adversely affect flip performance." which I think is
the whole point of this exercise.

> 
> Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> check that plane allocations had changed. Thats just confusing.

We are not checking if plane allocations have changed. We are
trying to determine if anything in the bw_state has changed.
If we have said state already then something in it may have 
changed and we have to recalculate anything that may depend
on those changed things, namely pipe_sagv_reject->qgv_point_mask.

I think ideally we'd not even modify the bw_state directly from the
watermark code and we'd instead defer that to bw atomic check entirely.
But this SAGV vs. DDB business is your typical chicken vs. egg situation,
so I'm not sure that is possible to do. Would need to spend a few minutes
thinking about it I guess.

> 
> May be for you as i915 guru, thats obvious however not for someone else, who might
> touch the code and we are doing open source here.
> 
> Can we just add some check which explicitly does per plane data rate checks?

There is nothing interesting about per-plane data rates.

> So that we know bail out from that first cycle not only when total_data_rate/active planes
> had changed, but we check per plane data rate? 
> That might actually save us also in future, if we ever get into such situation, when
> bw_state doesn't change, but ddb allocations do.
> 
> I know you might say it shouldn't happen, but there is always some new stuff coming.
> 
> Stan
> 
> > 
> > > Also we are now forcing the recalculation to be done always no matter what and using
> > > new bw state for that in a bit counterintuitive way, which I don't like. 
> > > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > > done right after this one.
> > 
> > If there is no bw_state then bw_state->pipe_sagv_reject can't have
> > changed and there is nothing to recalculate.
> > 
> > > 
> > > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> > 
> > Yes, NULL is perfectly fine.
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > 
> > > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > > if anyone else already added the bw state for us.
> > > > > > > > 
> > > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > > looking like few revisions together still some bugs :(
> > > > > > > > 
> > > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > > will just stay the same.
> > > > > > > 
> > > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > > due to some other plane changes.
> > > > > > 
> > > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > > 
> > > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > > 
> > > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > > 
> > > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > > 
> > > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > > 
> > > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > > +{
> > > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > > +       unsigned int data_rate, max_data_rate;
> > > > > > +       unsigned int num_active_planes;
> > > > > > +       struct intel_crtc *crtc;
> > > > > > +       int i;
> > > > > > +
> > > > > > +       /* FIXME earlier gens need some checks too */
> > > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > +                                           new_crtc_state, i) {
> > > > > > +               unsigned int old_data_rate =
> > > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > > +               unsigned int new_data_rate =
> > > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > > +               unsigned int old_active_planes =
> > > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > > +               unsigned int new_active_planes =
> > > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > > +
> > > > > > +               /*
> > > > > > +                * Avoid locking the bw state when
> > > > > > +                * nothing significant has changed.
> > > > > > +                */
> > > > > > +               if (old_data_rate == new_data_rate &&
> > > > > > +                   old_active_planes == new_active_planes)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > > +               if (IS_ERR(bw_state))
> > > > > > +                       return PTR_ERR(bw_state);
> > > > > > 
> > > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > > had changed.
> > > > > 
> > > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > > The planes could still change in some funny way where eg. some plane
> > > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > > do not change but the watermarks/ddb do.
> > > > > 
> > > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > > to change anything that would affect the bandwidth utilization, and neither
> > > > > is it allowed to enable/disable planes.
> > > > 
> > > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > > flip, however there is nothing about limiting ddb allocations.
> > > 
> > > Reducing just the watermark doesn't really make sense 
> > > if the goal is to keep the DBUF level to a minimum. Also
> > > I don't think there is any proper docs for this thing. The
> > > only thing we have just has some vague notes about using
> > > "minimum watermarks", whatever that means.
> > 
> > Was it the goal? I thought limiting watermarks would by itself also
> > limit package C states, thus affecting memory clocks and latency.
> > Because it really doesn't say anything about keeping Dbuf allocations
> > to a minimum. 
> 
> The goal is to miminize the amount of data in the FIFO.
> 
> > 
> > > 
> > > > 
> > > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > > cause some unexpected side effects.
> > > 
> > > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> > 
> > I agree there is a bug. The bug is such that initial bw checks were relying
> > on total data rate + active planes comparison, while it should have accounted
> > data rate per plane usage.
> > 
> > This should have been changed in SAGV patches, but probably had gone
> > unnoticed both by you and me.
> > 
> > > 
> > > Also the SAGV watermark is not the minimum watermark (if that is
> > > the doc really means by that), the normal WM0 is the minimum watermark.
> > > So even if we interpret the doc to say that we should just disable all
> > > watermark levels except the smallest one (normal WM0) without changing
> > > the ddb allocations we would still end up disabling SAGV.
> > 
> > Thats actually a good question. Did they mean, disable all "regular" wm levels
> > or the SAGV one also? Probably they meant what you say, but would be nice to know
> > exactly.
> 
> They said neither. It's just "program minimum watermarks" which
> could mean anything really. They do explicitly say "DBUF level
> can also adversely affect flip performance." which I think is
> the whole point of this exercise.
> 
> > 
> > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > check that plane allocations had changed. Thats just confusing.
> 
> We are not checking if plane allocations have changed. We are
> trying to determine if anything in the bw_state has changed.
> If we have said state already then something in it may have 
> changed and we have to recalculate anything that may depend
> on those changed things, namely pipe_sagv_reject->qgv_point_mask.

I think it is just not very intuitive that we use the fact whether
we can get new_bw_state or not, as a way to check if something had
changed.
Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
or "bw_state_changed". Because for anyone not quite familiar with
that state paradigm we use, that would look pretty confusing that first
we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
override it with intel_atomic_get_bw_state.
And whether we can get new_bw_state or not is just acting like a check,
that we don't have anything changed in bw_state.

Moreover indeed ideally intel_bw_atomic_check should probably handle all
that sagv stuff as well, i.e I would suggest moving pipe_reject_mask setting,
based skl_compute_wm results to that function.
I don't see any issue here because in skl_compute_wm we just calculate the 
sagv wm, then in intel_bw_atomic_check we just call intel_compute_sagv_mask,
which then calls tgl_crtc_can_enable_sagv for each crtc and sets this mask.

I think by boing this in intel_bw_atomic_check we would achieve both, what
you were willing to do, plus it would be more obvious, why things are happening
that way.

Stan

> 
> I think ideally we'd not even modify the bw_state directly from the
> watermark code and we'd instead defer that to bw atomic check entirely.
> But this SAGV vs. DDB business is your typical chicken vs. egg situation,
> so I'm not sure that is possible to do. Would need to spend a few minutes
> thinking about it I guess.
> 
> > 
> > May be for you as i915 guru, thats obvious however not for someone else, who might
> > touch the code and we are doing open source here.
> > 
> > Can we just add some check which explicitly does per plane data rate checks?
> 
> There is nothing interesting about per-plane data rates.
> 
> > So that we know bail out from that first cycle not only when total_data_rate/active planes
> > had changed, but we check per plane data rate? 
> > That might actually save us also in future, if we ever get into such situation, when
> > bw_state doesn't change, but ddb allocations do.
> > 
> > I know you might say it shouldn't happen, but there is always some new stuff coming.
> > 
> > Stan
> > 
> > > 
> > > > Also we are now forcing the recalculation to be done always no matter what and using
> > > > new bw state for that in a bit counterintuitive way, which I don't like. 
> > > > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > > > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > > > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > > > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > > > done right after this one.
> > > 
> > > If there is no bw_state then bw_state->pipe_sagv_reject can't have
> > > changed and there is nothing to recalculate.
> > > 
> > > > 
> > > > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > > > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > > > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> > > 
> > > Yes, NULL is perfectly fine.
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Lisovskiy, Stanislav]

On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > 
> > > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > > if anyone else already added the bw state for us.
> > > > > > > > 
> > > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > > looking like few revisions together still some bugs :(
> > > > > > > > 
> > > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > > will just stay the same.
> > > > > > > 
> > > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > > due to some other plane changes.
> > > > > > 
> > > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > > 
> > > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > > 
> > > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > > 
> > > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > > 
> > > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > > 
> > > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > > +{
> > > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > > +       unsigned int data_rate, max_data_rate;
> > > > > > +       unsigned int num_active_planes;
> > > > > > +       struct intel_crtc *crtc;
> > > > > > +       int i;
> > > > > > +
> > > > > > +       /* FIXME earlier gens need some checks too */
> > > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > +                                           new_crtc_state, i) {
> > > > > > +               unsigned int old_data_rate =
> > > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > > +               unsigned int new_data_rate =
> > > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > > +               unsigned int old_active_planes =
> > > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > > +               unsigned int new_active_planes =
> > > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > > +
> > > > > > +               /*
> > > > > > +                * Avoid locking the bw state when
> > > > > > +                * nothing significant has changed.
> > > > > > +                */
> > > > > > +               if (old_data_rate == new_data_rate &&
> > > > > > +                   old_active_planes == new_active_planes)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > > +               if (IS_ERR(bw_state))
> > > > > > +                       return PTR_ERR(bw_state);
> > > > > > 
> > > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > > had changed.
> > > > > 
> > > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > > The planes could still change in some funny way where eg. some plane
> > > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > > do not change but the watermarks/ddb do.
> > > > > 
> > > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > > to change anything that would affect the bandwidth utilization, and neither
> > > > > is it allowed to enable/disable planes.
> > > > 
> > > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > > flip, however there is nothing about limiting ddb allocations.
> > > 
> > > Reducing just the watermark doesn't really make sense 
> > > if the goal is to keep the DBUF level to a minimum. Also
> > > I don't think there is any proper docs for this thing. The
> > > only thing we have just has some vague notes about using
> > > "minimum watermarks", whatever that means.
> > 
> > Was it the goal? I thought limiting watermarks would by itself also
> > limit package C states, thus affecting memory clocks and latency.
> > Because it really doesn't say anything about keeping Dbuf allocations
> > to a minimum. 
> 
> The goal is to miminize the amount of data in the FIFO.
> 
> > 
> > > 
> > > > 
> > > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > > cause some unexpected side effects.
> > > 
> > > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> > 
> > I agree there is a bug. The bug is such that initial bw checks were relying
> > on total data rate + active planes comparison, while it should have accounted
> > data rate per plane usage.
> > 
> > This should have been changed in SAGV patches, but probably had gone
> > unnoticed both by you and me.
> > 
> > > 
> > > Also the SAGV watermark is not the minimum watermark (if that is
> > > the doc really means by that), the normal WM0 is the minimum watermark.
> > > So even if we interpret the doc to say that we should just disable all
> > > watermark levels except the smallest one (normal WM0) without changing
> > > the ddb allocations we would still end up disabling SAGV.
> > 
> > Thats actually a good question. Did they mean, disable all "regular" wm levels
> > or the SAGV one also? Probably they meant what you say, but would be nice to know
> > exactly.
> 
> They said neither. It's just "program minimum watermarks" which
> could mean anything really. They do explicitly say "DBUF level
> can also adversely affect flip performance." which I think is
> the whole point of this exercise.
> 
> > 
> > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > check that plane allocations had changed. Thats just confusing.
> 
> We are not checking if plane allocations have changed. We are
> trying to determine if anything in the bw_state has changed.
> If we have said state already then something in it may have 
> changed and we have to recalculate anything that may depend
> on those changed things, namely pipe_sagv_reject->qgv_point_mask.

I think it is just not very intuitive that we use the fact whether
we can get new_bw_state or not, as a way to check if something had
changed.
Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
or "bw_state_changed". Because for anyone not quite familiar with
that state paradigm we use, that would look pretty confusing that first
we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
override it with intel_atomic_get_bw_state.
And whether we can get new_bw_state or not is just acting like a check,
that we don't have anything changed in bw_state.

Moreover indeed ideally intel_bw_atomic_check should probably handle all
that sagv stuff as well, i.e I would suggest moving pipe_reject_mask setting,
based skl_compute_wm results to that function.
I don't see any issue here because in skl_compute_wm we just calculate the 
sagv wm, then in intel_bw_atomic_check we just call intel_compute_sagv_mask,
which then calls tgl_crtc_can_enable_sagv for each crtc and sets this mask.

I think by boing this in intel_bw_atomic_check we would achieve both, what
you were willing to do, plus it would be more obvious, why things are happening
that way.

Stan

> 
> I think ideally we'd not even modify the bw_state directly from the
> watermark code and we'd instead defer that to bw atomic check entirely.
> But this SAGV vs. DDB business is your typical chicken vs. egg situation,
> so I'm not sure that is possible to do. Would need to spend a few minutes
> thinking about it I guess.
> 
> > 
> > May be for you as i915 guru, thats obvious however not for someone else, who might
> > touch the code and we are doing open source here.
> > 
> > Can we just add some check which explicitly does per plane data rate checks?
> 
> There is nothing interesting about per-plane data rates.
> 
> > So that we know bail out from that first cycle not only when total_data_rate/active planes
> > had changed, but we check per plane data rate? 
> > That might actually save us also in future, if we ever get into such situation, when
> > bw_state doesn't change, but ddb allocations do.
> > 
> > I know you might say it shouldn't happen, but there is always some new stuff coming.
> > 
> > Stan
> > 
> > > 
> > > > Also we are now forcing the recalculation to be done always no matter what and using
> > > > new bw state for that in a bit counterintuitive way, which I don't like. 
> > > > Not even sure that will always work, as we are not guaranteed to get a non-NULL
> > > > new_bw_state object from calling intel_atomic_get_new_bw_state, for that purpose we
> > > > typically call intel_atomic_get_bw_state, which is supposed to do that and its called only
> > > > here and in cause of CDCLK recalculation, which is called in intel_cdclk_atomic_check and
> > > > done right after this one.
> > > 
> > > If there is no bw_state then bw_state->pipe_sagv_reject can't have
> > > changed and there is nothing to recalculate.
> > > 
> > > > 
> > > > So if we haven't called intel_atomic_get_bw_state beforehand, which we didn't because there are
> > > > 2 places, where new bw state was supposed to be created to be usable by intel_atomic_get_new_bw_state
> > > > - I think, we will(or might) get a NULL here, because intel_atomic_get_bw_state hasn't been called yet.
> > > 
> > > Yes, NULL is perfectly fine.
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 06:33:42PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > > On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > 
> > > > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > > > if anyone else already added the bw state for us.
> > > > > > > > > 
> > > > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > > > looking like few revisions together still some bugs :(
> > > > > > > > > 
> > > > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > > > will just stay the same.
> > > > > > > > 
> > > > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > > > due to some other plane changes.
> > > > > > > 
> > > > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > > > 
> > > > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > > > 
> > > > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > > > 
> > > > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > > > 
> > > > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > > > 
> > > > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > > > +       unsigned int data_rate, max_data_rate;
> > > > > > > +       unsigned int num_active_planes;
> > > > > > > +       struct intel_crtc *crtc;
> > > > > > > +       int i;
> > > > > > > +
> > > > > > > +       /* FIXME earlier gens need some checks too */
> > > > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > > > +               return 0;
> > > > > > > +
> > > > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > > +                                           new_crtc_state, i) {
> > > > > > > +               unsigned int old_data_rate =
> > > > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > > > +               unsigned int new_data_rate =
> > > > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > > > +               unsigned int old_active_planes =
> > > > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > > > +               unsigned int new_active_planes =
> > > > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > > > +
> > > > > > > +               /*
> > > > > > > +                * Avoid locking the bw state when
> > > > > > > +                * nothing significant has changed.
> > > > > > > +                */
> > > > > > > +               if (old_data_rate == new_data_rate &&
> > > > > > > +                   old_active_planes == new_active_planes)
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > > > +               if (IS_ERR(bw_state))
> > > > > > > +                       return PTR_ERR(bw_state);
> > > > > > > 
> > > > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > > > had changed.
> > > > > > 
> > > > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > > > The planes could still change in some funny way where eg. some plane
> > > > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > > > do not change but the watermarks/ddb do.
> > > > > > 
> > > > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > > > to change anything that would affect the bandwidth utilization, and neither
> > > > > > is it allowed to enable/disable planes.
> > > > > 
> > > > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > > > flip, however there is nothing about limiting ddb allocations.
> > > > 
> > > > Reducing just the watermark doesn't really make sense 
> > > > if the goal is to keep the DBUF level to a minimum. Also
> > > > I don't think there is any proper docs for this thing. The
> > > > only thing we have just has some vague notes about using
> > > > "minimum watermarks", whatever that means.
> > > 
> > > Was it the goal? I thought limiting watermarks would by itself also
> > > limit package C states, thus affecting memory clocks and latency.
> > > Because it really doesn't say anything about keeping Dbuf allocations
> > > to a minimum. 
> > 
> > The goal is to miminize the amount of data in the FIFO.
> > 
> > > 
> > > > 
> > > > > 
> > > > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > > > cause some unexpected side effects.
> > > > 
> > > > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> > > 
> > > I agree there is a bug. The bug is such that initial bw checks were relying
> > > on total data rate + active planes comparison, while it should have accounted
> > > data rate per plane usage.
> > > 
> > > This should have been changed in SAGV patches, but probably had gone
> > > unnoticed both by you and me.
> > > 
> > > > 
> > > > Also the SAGV watermark is not the minimum watermark (if that is
> > > > the doc really means by that), the normal WM0 is the minimum watermark.
> > > > So even if we interpret the doc to say that we should just disable all
> > > > watermark levels except the smallest one (normal WM0) without changing
> > > > the ddb allocations we would still end up disabling SAGV.
> > > 
> > > Thats actually a good question. Did they mean, disable all "regular" wm levels
> > > or the SAGV one also? Probably they meant what you say, but would be nice to know
> > > exactly.
> > 
> > They said neither. It's just "program minimum watermarks" which
> > could mean anything really. They do explicitly say "DBUF level
> > can also adversely affect flip performance." which I think is
> > the whole point of this exercise.
> > 
> > > 
> > > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > > check that plane allocations had changed. Thats just confusing.
> > 
> > We are not checking if plane allocations have changed. We are
> > trying to determine if anything in the bw_state has changed.
> > If we have said state already then something in it may have 
> > changed and we have to recalculate anything that may depend
> > on those changed things, namely pipe_sagv_reject->qgv_point_mask.
> 
> I think it is just not very intuitive that we use the fact whether
> we can get new_bw_state or not, as a way to check if something had
> changed.
> Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
> or "bw_state_changed". Because for anyone not quite familiar with
> that state paradigm we use, that would look pretty confusing that first
> we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
> override it with intel_atomic_get_bw_state.
> And whether we can get new_bw_state or not is just acting like a check,
> that we don't have anything changed in bw_state.

I think the only thing we'd achieve is something like this:

new_bw_state = NULL;
if (has_new_bw_state())
	new_bw_state = get_new_bw_state();
...
if (!new_bw_state)
	return 0;

instead of just

new_bw_state = get_new_bw_state();
...
if (!new_bw_state)
	return 0;

I don't know why that would be an improvement.

> Moreover indeed ideally intel_bw_atomic_check should probably handle all
> that sagv stuff as well, i.e I would suggest moving pipe_reject_mask setting,
> based skl_compute_wm results to that function.
> I don't see any issue here because in skl_compute_wm we just calculate the 
> sagv wm, then in intel_bw_atomic_check we just call intel_compute_sagv_mask,
> which then calls tgl_crtc_can_enable_sagv for each crtc and sets this mask.

It's not going to work unless we split the watermark computation
into two parts and spread it around the bandwidth check.

The current steps are as follows:
1. skl_build_pipe_wm()+skl_compute_ddb()
2. intel_compute_sagv_mask()
3. skl_wm_add_affected_planes()
4. intel_bw_atomic_check()

What you're after is essentially pulling intel_compute_sagv_mask() (or
most of it) into intel_bw_atomic_check(). Which means
skl_wm_add_affected_planes() would need a new higher level call site
instead of being called as part of skl_compute_wm(). We have no vfunc
for that atm.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 06:33:42PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > > On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > 
> > > > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > > > if anyone else already added the bw state for us.
> > > > > > > > > 
> > > > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > > > looking like few revisions together still some bugs :(
> > > > > > > > > 
> > > > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > > > will just stay the same.
> > > > > > > > 
> > > > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > > > due to some other plane changes.
> > > > > > > 
> > > > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > > > 
> > > > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > > > 
> > > > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > > > 
> > > > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > > > 
> > > > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > > > 
> > > > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > > > +{
> > > > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > > > +       unsigned int data_rate, max_data_rate;
> > > > > > > +       unsigned int num_active_planes;
> > > > > > > +       struct intel_crtc *crtc;
> > > > > > > +       int i;
> > > > > > > +
> > > > > > > +       /* FIXME earlier gens need some checks too */
> > > > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > > > +               return 0;
> > > > > > > +
> > > > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > > +                                           new_crtc_state, i) {
> > > > > > > +               unsigned int old_data_rate =
> > > > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > > > +               unsigned int new_data_rate =
> > > > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > > > +               unsigned int old_active_planes =
> > > > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > > > +               unsigned int new_active_planes =
> > > > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > > > +
> > > > > > > +               /*
> > > > > > > +                * Avoid locking the bw state when
> > > > > > > +                * nothing significant has changed.
> > > > > > > +                */
> > > > > > > +               if (old_data_rate == new_data_rate &&
> > > > > > > +                   old_active_planes == new_active_planes)
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > > > +               if (IS_ERR(bw_state))
> > > > > > > +                       return PTR_ERR(bw_state);
> > > > > > > 
> > > > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > > > had changed.
> > > > > > 
> > > > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > > > The planes could still change in some funny way where eg. some plane
> > > > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > > > do not change but the watermarks/ddb do.
> > > > > > 
> > > > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > > > to change anything that would affect the bandwidth utilization, and neither
> > > > > > is it allowed to enable/disable planes.
> > > > > 
> > > > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > > > flip, however there is nothing about limiting ddb allocations.
> > > > 
> > > > Reducing just the watermark doesn't really make sense 
> > > > if the goal is to keep the DBUF level to a minimum. Also
> > > > I don't think there is any proper docs for this thing. The
> > > > only thing we have just has some vague notes about using
> > > > "minimum watermarks", whatever that means.
> > > 
> > > Was it the goal? I thought limiting watermarks would by itself also
> > > limit package C states, thus affecting memory clocks and latency.
> > > Because it really doesn't say anything about keeping Dbuf allocations
> > > to a minimum. 
> > 
> > The goal is to miminize the amount of data in the FIFO.
> > 
> > > 
> > > > 
> > > > > 
> > > > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > > > cause some unexpected side effects.
> > > > 
> > > > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> > > 
> > > I agree there is a bug. The bug is such that initial bw checks were relying
> > > on total data rate + active planes comparison, while it should have accounted
> > > data rate per plane usage.
> > > 
> > > This should have been changed in SAGV patches, but probably had gone
> > > unnoticed both by you and me.
> > > 
> > > > 
> > > > Also the SAGV watermark is not the minimum watermark (if that is
> > > > the doc really means by that), the normal WM0 is the minimum watermark.
> > > > So even if we interpret the doc to say that we should just disable all
> > > > watermark levels except the smallest one (normal WM0) without changing
> > > > the ddb allocations we would still end up disabling SAGV.
> > > 
> > > Thats actually a good question. Did they mean, disable all "regular" wm levels
> > > or the SAGV one also? Probably they meant what you say, but would be nice to know
> > > exactly.
> > 
> > They said neither. It's just "program minimum watermarks" which
> > could mean anything really. They do explicitly say "DBUF level
> > can also adversely affect flip performance." which I think is
> > the whole point of this exercise.
> > 
> > > 
> > > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > > check that plane allocations had changed. Thats just confusing.
> > 
> > We are not checking if plane allocations have changed. We are
> > trying to determine if anything in the bw_state has changed.
> > If we have said state already then something in it may have 
> > changed and we have to recalculate anything that may depend
> > on those changed things, namely pipe_sagv_reject->qgv_point_mask.
> 
> I think it is just not very intuitive that we use the fact whether
> we can get new_bw_state or not, as a way to check if something had
> changed.
> Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
> or "bw_state_changed". Because for anyone not quite familiar with
> that state paradigm we use, that would look pretty confusing that first
> we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
> override it with intel_atomic_get_bw_state.
> And whether we can get new_bw_state or not is just acting like a check,
> that we don't have anything changed in bw_state.

I think the only thing we'd achieve is something like this:

new_bw_state = NULL;
if (has_new_bw_state())
	new_bw_state = get_new_bw_state();
...
if (!new_bw_state)
	return 0;

instead of just

new_bw_state = get_new_bw_state();
...
if (!new_bw_state)
	return 0;

I don't know why that would be an improvement.

> Moreover indeed ideally intel_bw_atomic_check should probably handle all
> that sagv stuff as well, i.e I would suggest moving pipe_reject_mask setting,
> based skl_compute_wm results to that function.
> I don't see any issue here because in skl_compute_wm we just calculate the 
> sagv wm, then in intel_bw_atomic_check we just call intel_compute_sagv_mask,
> which then calls tgl_crtc_can_enable_sagv for each crtc and sets this mask.

It's not going to work unless we split the watermark computation
into two parts and spread it around the bandwidth check.

The current steps are as follows:
1. skl_build_pipe_wm()+skl_compute_ddb()
2. intel_compute_sagv_mask()
3. skl_wm_add_affected_planes()
4. intel_bw_atomic_check()

What you're after is essentially pulling intel_compute_sagv_mask() (or
most of it) into intel_bw_atomic_check(). Which means
skl_wm_add_affected_planes() would need a new higher level call site
instead of being called as part of skl_compute_wm(). We have no vfunc
for that atm.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 06:52:51PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 06:33:42PM +0200, Lisovskiy, Stanislav wrote:
> > On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Tue, Feb 15, 2022 at 12:10:19PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Feb 15, 2022 at 10:59:57AM +0200, Lisovskiy, Stanislav wrote:
> > > > > > On Mon, Feb 14, 2022 at 10:26:39PM +0200, Ville Syrjälä wrote:
> > > > > > > On Mon, Feb 14, 2022 at 07:03:05PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > On Mon, Feb 14, 2022 at 12:24:57PM +0200, Ville Syrjälä wrote:
> > > > > > > > > On Mon, Feb 14, 2022 at 12:05:36PM +0200, Lisovskiy, Stanislav wrote:
> > > > > > > > > > On Mon, Feb 14, 2022 at 11:18:07AM +0200, Ville Syrjala wrote:
> > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > > 
> > > > > > > > > > > If the only thing that is changing is SAGV vs. no SAGV but
> > > > > > > > > > > the number of active planes and the total data rates end up
> > > > > > > > > > > unchanged we currently bail out of intel_bw_atomic_check()
> > > > > > > > > > > early and forget to actually compute the new WGV point
> > > > > > > > > > > mask and thus won't actually enable/disable SAGV as requested.
> > > > > > > > > > > This ends up poorly if we end up running with SAGV enabled
> > > > > > > > > > > when we shouldn't. Usually ends up in underruns.
> > > > > > > > > > > To fix this let's go through the QGV point mask computation
> > > > > > > > > > > if anyone else already added the bw state for us.
> > > > > > > > > > 
> > > > > > > > > > Haven't been looking this in a while. Despite we have been
> > > > > > > > > > looking like few revisions together still some bugs :(
> > > > > > > > > > 
> > > > > > > > > > I thought SAGV vs No SAGV can't change if active planes 
> > > > > > > > > > or data rate didn't change? Because it means we probably
> > > > > > > > > > still have same ddb allocations, which means SAGV state
> > > > > > > > > > will just stay the same.
> > > > > > > > > 
> > > > > > > > > SAGV can change due to watermarks/ddb allocations. The easiest
> > > > > > > > > way to trip this up is to try to use the async flip wm0/ddb 
> > > > > > > > > optimization. That immediately forgets to turn off SAGV and
> > > > > > > > > we get underruns, whcih is how I noticed this. And I don't
> > > > > > > > > immediately see any easy proof that this couldn't also happen
> > > > > > > > > due to some other plane changes.
> > > > > > > > 
> > > > > > > > Thats the way it was initially implemented even before SAGV was added.
> > > > > > > 
> > > > > > > Yeah, it wasn't a problem as long as SAGV was not enabled.
> > > > > > > 
> > > > > > > > I think it can be dated back to the very first bw check was implemented.
> > > > > > > > 
> > > > > > > > commit c457d9cf256e942138a54a2e80349ee7fe20c391
> > > > > > > > Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Date:   Fri May 24 18:36:14 2019 +0300
> > > > > > > > 
> > > > > > > >     drm/i915: Make sure we have enough memory bandwidth on ICL
> > > > > > > > 
> > > > > > > > +int intel_bw_atomic_check(struct intel_atomic_state *state)
> > > > > > > > +{
> > > > > > > > +       struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > > > > +       struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > > > > +       struct intel_bw_state *bw_state = NULL;
> > > > > > > > +       unsigned int data_rate, max_data_rate;
> > > > > > > > +       unsigned int num_active_planes;
> > > > > > > > +       struct intel_crtc *crtc;
> > > > > > > > +       int i;
> > > > > > > > +
> > > > > > > > +       /* FIXME earlier gens need some checks too */
> > > > > > > > +       if (INTEL_GEN(dev_priv) < 11)
> > > > > > > > +               return 0;
> > > > > > > > +
> > > > > > > > +       for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > > > > > > > +                                           new_crtc_state, i) {
> > > > > > > > +               unsigned int old_data_rate =
> > > > > > > > +                       intel_bw_crtc_data_rate(old_crtc_state);
> > > > > > > > +               unsigned int new_data_rate =
> > > > > > > > +                       intel_bw_crtc_data_rate(new_crtc_state);
> > > > > > > > +               unsigned int old_active_planes =
> > > > > > > > +                       intel_bw_crtc_num_active_planes(old_crtc_state);
> > > > > > > > +               unsigned int new_active_planes =
> > > > > > > > +                       intel_bw_crtc_num_active_planes(new_crtc_state);
> > > > > > > > +
> > > > > > > > +               /*
> > > > > > > > +                * Avoid locking the bw state when
> > > > > > > > +                * nothing significant has changed.
> > > > > > > > +                */
> > > > > > > > +               if (old_data_rate == new_data_rate &&
> > > > > > > > +                   old_active_planes == new_active_planes)
> > > > > > > > +                       continue;
> > > > > > > > +
> > > > > > > > +               bw_state  = intel_atomic_get_bw_state(state);
> > > > > > > > +               if (IS_ERR(bw_state))
> > > > > > > > +                       return PTR_ERR(bw_state);
> > > > > > > > 
> > > > > > > > However, what can cause watermarks/ddb to change, besides plane state change
> > > > > > > > and/or active planes change? We change watermarks, when we change ddb allocations
> > > > > > > > and we change ddb allocations when active planes had changed and/or data rate
> > > > > > > > had changed.
> > > > > > > 
> > > > > > > The bw code only cares about the aggregate numbers from all the planes.
> > > > > > > The planes could still change in some funny way where eg. some plane
> > > > > > > frees up some bandwidth, but the other planes gobble up the exact same
> > > > > > > amount and thus the aggregate numbers the bw atomic check cares about
> > > > > > > do not change but the watermarks/ddb do.
> > > > > > > 
> > > > > > > And as mentiioned, the async flip wm0/ddb optimization makes this trivial
> > > > > > > to trip up since it will want to disable SAGV as there is not enough ddb
> > > > > > > for the SAGV watermark. And async flip specifically isn't even allowed
> > > > > > > to change anything that would affect the bandwidth utilization, and neither
> > > > > > > is it allowed to enable/disable planes.
> > > > > > 
> > > > > > I think the whole idea of setting ddb to minimum in case of async flip optimization
> > > > > > was purely our idea - BSpec/HSD only mentions forbidding wm levels > 0 in case of async
> > > > > > flip, however there is nothing about limiting ddb allocations.
> > > > > 
> > > > > Reducing just the watermark doesn't really make sense 
> > > > > if the goal is to keep the DBUF level to a minimum. Also
> > > > > I don't think there is any proper docs for this thing. The
> > > > > only thing we have just has some vague notes about using
> > > > > "minimum watermarks", whatever that means.
> > > > 
> > > > Was it the goal? I thought limiting watermarks would by itself also
> > > > limit package C states, thus affecting memory clocks and latency.
> > > > Because it really doesn't say anything about keeping Dbuf allocations
> > > > to a minimum. 
> > > 
> > > The goal is to miminize the amount of data in the FIFO.
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > Was a bit suspicious about that whole change, to be honest - and yep, now it seems to
> > > > > > cause some unexpected side effects.
> > > > > 
> > > > > The bw_state vs. SAGV bug is there regardless of the wm0 optimization.
> > > > 
> > > > I agree there is a bug. The bug is such that initial bw checks were relying
> > > > on total data rate + active planes comparison, while it should have accounted
> > > > data rate per plane usage.
> > > > 
> > > > This should have been changed in SAGV patches, but probably had gone
> > > > unnoticed both by you and me.
> > > > 
> > > > > 
> > > > > Also the SAGV watermark is not the minimum watermark (if that is
> > > > > the doc really means by that), the normal WM0 is the minimum watermark.
> > > > > So even if we interpret the doc to say that we should just disable all
> > > > > watermark levels except the smallest one (normal WM0) without changing
> > > > > the ddb allocations we would still end up disabling SAGV.
> > > > 
> > > > Thats actually a good question. Did they mean, disable all "regular" wm levels
> > > > or the SAGV one also? Probably they meant what you say, but would be nice to know
> > > > exactly.
> > > 
> > > They said neither. It's just "program minimum watermarks" which
> > > could mean anything really. They do explicitly say "DBUF level
> > > can also adversely affect flip performance." which I think is
> > > the whole point of this exercise.
> > > 
> > > > 
> > > > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > > > check that plane allocations had changed. Thats just confusing.
> > > 
> > > We are not checking if plane allocations have changed. We are
> > > trying to determine if anything in the bw_state has changed.
> > > If we have said state already then something in it may have 
> > > changed and we have to recalculate anything that may depend
> > > on those changed things, namely pipe_sagv_reject->qgv_point_mask.
> > 
> > I think it is just not very intuitive that we use the fact whether
> > we can get new_bw_state or not, as a way to check if something had
> > changed.
> > Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
> > or "bw_state_changed". Because for anyone not quite familiar with
> > that state paradigm we use, that would look pretty confusing that first
> > we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
> > override it with intel_atomic_get_bw_state.
> > And whether we can get new_bw_state or not is just acting like a check,
> > that we don't have anything changed in bw_state.
> 
> I think the only thing we'd achieve is something like this:
> 
> new_bw_state = NULL;
> if (has_new_bw_state())
> 	new_bw_state = get_new_bw_state();
> ...
> if (!new_bw_state)
> 	return 0;
> 
> instead of just
> 
> new_bw_state = get_new_bw_state();
> ...
> if (!new_bw_state)
> 	return 0;
> 
> I don't know why that would be an improvement.

Though I suppose a comment might be in order pointing the
reader towards intel_compute_sagv_mask().

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

[Ville Syrjälä]

On Tue, Feb 15, 2022 at 06:58:35PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 15, 2022 at 06:52:51PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 15, 2022 at 06:33:42PM +0200, Lisovskiy, Stanislav wrote:
> > > On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
> > > > > Anyway my point here is that, we probably shouldn't use new_bw_state as a way to 
> > > > > check that plane allocations had changed. Thats just confusing.
> > > > 
> > > > We are not checking if plane allocations have changed. We are
> > > > trying to determine if anything in the bw_state has changed.
> > > > If we have said state already then something in it may have 
> > > > changed and we have to recalculate anything that may depend
> > > > on those changed things, namely pipe_sagv_reject->qgv_point_mask.
> > > 
> > > I think it is just not very intuitive that we use the fact whether
> > > we can get new_bw_state or not, as a way to check if something had
> > > changed.
> > > Would be nice to put it in somekind of a wrapper like "has_new_bw_state"
> > > or "bw_state_changed". Because for anyone not quite familiar with
> > > that state paradigm we use, that would look pretty confusing that first
> > > we get new_bw_state using intel_atomic_get_new_bw_state, then immediately
> > > override it with intel_atomic_get_bw_state.
> > > And whether we can get new_bw_state or not is just acting like a check,
> > > that we don't have anything changed in bw_state.
> > 
> > I think the only thing we'd achieve is something like this:
> > 
> > new_bw_state = NULL;
> > if (has_new_bw_state())
> > 	new_bw_state = get_new_bw_state();
> > ...
> > if (!new_bw_state)
> > 	return 0;
> > 
> > instead of just
> > 
> > new_bw_state = get_new_bw_state();
> > ...
> > if (!new_bw_state)
> > 	return 0;
> > 
> > I don't know why that would be an improvement.
> 
> Though I suppose a comment might be in order pointing the
> reader towards intel_compute_sagv_mask().

Although, I guess one idea would be to extract that data_rate
loop thing into a separate function and then we'd just end up with
something along the lines of:

ret = intel_bw_check_data_rate(state);
if (ret)
	return ret;

new_bw_state = intel_atomic_get_new_bw_state(state);
if (!new_bw_state)
	return 0;

...

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: SAGV fixes

[Patchwork]

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

== Series Details ==

Series: drm/i915: SAGV fixes
URL   : https://patchwork.freedesktop.org/series/100091/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11229 -> Patchwork_22262
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (48 -> 44)
------------------------------

  Additional (1): fi-icl-u2 
  Missing    (5): shard-tglu fi-bsw-cyan shard-rkl shard-dg1 bat-jsl-2 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-hsw-4770:        NOTRUN -> [SKIP][1] ([fdo#109271] / [fdo#109315]) +17 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-hsw-4770/igt@amdgpu/amd_basic@semaphore.html

  * igt@amdgpu/amd_cs_nop@fork-compute0:
    - fi-blb-e6850:       NOTRUN -> [SKIP][2] ([fdo#109271]) +17 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-blb-e6850/igt@amdgpu/amd_cs_nop@fork-compute0.html

  * igt@amdgpu/amd_cs_nop@fork-gfx0:
    - fi-icl-u2:          NOTRUN -> [SKIP][3] ([fdo#109315]) +17 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@amdgpu/amd_cs_nop@fork-gfx0.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-skl-6600u:       NOTRUN -> [FAIL][4] ([i915#4547])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-skl-6600u/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_huc_copy@huc-copy:
    - fi-icl-u2:          NOTRUN -> [SKIP][5] ([i915#2190])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-icl-u2:          NOTRUN -> [SKIP][6] ([i915#4613]) +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@i915_selftest@live@hangcheck:
    - fi-bdw-5557u:       NOTRUN -> [INCOMPLETE][7] ([i915#3921])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-bdw-5557u/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          NOTRUN -> [SKIP][8] ([fdo#111827]) +8 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_chamelium@vga-edid-read:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][9] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-bdw-5557u/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-icl-u2:          NOTRUN -> [SKIP][10] ([fdo#109278]) +2 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u2:          NOTRUN -> [SKIP][11] ([fdo#109285])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-cml-u2:          [PASS][12] -> [DMESG-WARN][13] ([i915#4269])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/fi-cml-u2/igt@kms_frontbuffer_tracking@basic.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-cml-u2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_psr@cursor_plane_move:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][14] ([fdo#109271]) +13 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-bdw-5557u/igt@kms_psr@cursor_plane_move.html

  * igt@prime_vgem@basic-userptr:
    - fi-icl-u2:          NOTRUN -> [SKIP][15] ([i915#3301])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-icl-u2/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-skl-6600u:       NOTRUN -> [FAIL][16] ([i915#4312])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-skl-6600u/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-bdw-5557u:       [INCOMPLETE][17] ([i915#146]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-bdw-5557u/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@coherency:
    - {fi-tgl-dsi}:       [INCOMPLETE][19] -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/fi-tgl-dsi/igt@i915_selftest@live@coherency.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-tgl-dsi/igt@i915_selftest@live@coherency.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [INCOMPLETE][21] ([i915#3303]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@requests:
    - fi-blb-e6850:       [DMESG-FAIL][23] ([i915#4528] / [i915#5026]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/fi-blb-e6850/igt@i915_selftest@live@requests.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/fi-blb-e6850/igt@i915_selftest@live@requests.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4269]: https://gitlab.freedesktop.org/drm/intel/issues/4269
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4897]: https://gitlab.freedesktop.org/drm/intel/issues/4897
  [i915#5026]: https://gitlab.freedesktop.org/drm/intel/issues/5026


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

  * Linux: CI_DRM_11229 -> Patchwork_22262

  CI-20190529: 20190529
  CI_DRM_11229: 633bb0541185395f3777b64cfd54101cda5fec15 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6345: ee18c0497ec2c74007e299c3fdd26f1613b9f514 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22262: 7c9b74a72d824df7cf67390792000de9a125f761 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7c9b74a72d82 drm/i915: Pimp icl+ sagv pre/post update
76e2a9e4aeab drm/i915: Split pre-icl vs. icl+ SAGV hooks apart
c57c1ef421bd drm/i915: Unconfuse pre-icl vs. icl+ intel_sagv_{pre, post}_plane_update()
6b6be873bc1c drm/i915: Widen the QGV point mask
413058930942 drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV
6d156a621c46 drm/i915: Correctly populate use_sagv_wm for all pipes

== Logs ==

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

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

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: SAGV fixes

[Patchwork]

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

== Series Details ==

Series: drm/i915: SAGV fixes
URL   : https://patchwork.freedesktop.org/series/100091/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11229_full -> Patchwork_22262_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_22262_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_22262_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (13 -> 13)
------------------------------

  No changes in participating hosts

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

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

### CI changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * boot:
    - {shard-rkl}:        ([PASS][1], [PASS][2], [PASS][3], [PASS][4], [PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][12]) -> ([PASS][13], [PASS][14], [FAIL][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-1/boot.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-1/boot.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-1/boot.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-2/boot.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-2/boot.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-4/boot.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-4/boot.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/boot.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/boot.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/boot.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-6/boot.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-6/boot.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-6/boot.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-6/boot.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-5/boot.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-5/boot.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/boot.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/boot.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/boot.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/boot.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/boot.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/boot.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/boot.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-1/boot.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-1/boot.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-1/boot.html

  

### IGT changes ###

#### Possible regressions ####

  * igt@kms_cursor_legacy@all-pipes-single-move:
    - shard-tglb:         [PASS][27] -> [INCOMPLETE][28] +9 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglb6/igt@kms_cursor_legacy@all-pipes-single-move.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb5/igt@kms_cursor_legacy@all-pipes-single-move.html

  * igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
    - shard-iclb:         [PASS][29] -> [FAIL][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb1/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@kms_cursor_legacy@cursor-vs-flip-varying-size.html

  * igt@kms_cursor_legacy@pipe-c-single-move:
    - shard-iclb:         [PASS][31] -> [INCOMPLETE][32] +10 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb1/igt@kms_cursor_legacy@pipe-c-single-move.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@kms_cursor_legacy@pipe-c-single-move.html

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
    - shard-snb:          [PASS][33] -> [DMESG-WARN][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-snb5/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-snb4/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_cursor_legacy@all-pipes-single-bo:
    - {shard-dg1}:        NOTRUN -> [INCOMPLETE][35] +1 similar issue
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-dg1-13/igt@kms_cursor_legacy@all-pipes-single-bo.html

  * igt@kms_cursor_legacy@pipe-a-forked-move:
    - {shard-rkl}:        ([PASS][36], [PASS][37]) -> ([INCOMPLETE][38], [INCOMPLETE][39])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/igt@kms_cursor_legacy@pipe-a-forked-move.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-4/igt@kms_cursor_legacy@pipe-a-forked-move.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/igt@kms_cursor_legacy@pipe-a-forked-move.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/igt@kms_cursor_legacy@pipe-a-forked-move.html

  * igt@kms_cursor_legacy@pipe-b-single-bo:
    - {shard-rkl}:        NOTRUN -> [INCOMPLETE][40] +1 similar issue
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-6/igt@kms_cursor_legacy@pipe-b-single-bo.html

  * igt@kms_cursor_legacy@pipe-c-forked-bo:
    - {shard-dg1}:        [PASS][41] -> [INCOMPLETE][42] +4 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-dg1-18/igt@kms_cursor_legacy@pipe-c-forked-bo.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-dg1-18/igt@kms_cursor_legacy@pipe-c-forked-bo.html

  * igt@kms_cursor_legacy@pipe-c-forked-move:
    - {shard-rkl}:        NOTRUN -> ([SKIP][43], [INCOMPLETE][44]) ([i915#4070])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/igt@kms_cursor_legacy@pipe-c-forked-move.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/igt@kms_cursor_legacy@pipe-c-forked-move.html

  * igt@kms_cursor_legacy@pipe-d-forked-move:
    - {shard-tglu}:       NOTRUN -> [INCOMPLETE][45] +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglu-2/igt@kms_cursor_legacy@pipe-d-forked-move.html

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

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

### CI changes ###

#### Possible fixes ####

  * boot:
    - shard-glk:          ([PASS][46], [FAIL][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52], [PASS][53], [PASS][54], [PASS][55], [PASS][56], [PASS][57], [PASS][58], [PASS][59], [PASS][60], [PASS][61], [PASS][62], [PASS][63], [PASS][64], [PASS][65], [PASS][66], [PASS][67], [PASS][68], [PASS][69], [PASS][70]) ([i915#4392]) -> ([PASS][71], [PASS][72], [PASS][73], [PASS][74], [PASS][75], [PASS][76], [PASS][77], [PASS][78], [PASS][79], [PASS][80], [PASS][81], [PASS][82], [PASS][83], [PASS][84], [PASS][85], [PASS][86], [PASS][87], [PASS][88], [PASS][89], [PASS][90], [PASS][91], [PASS][92], [PASS][93], [PASS][94], [PASS][95])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk1/boot.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk1/boot.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk1/boot.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk1/boot.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk2/boot.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk2/boot.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk2/boot.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk3/boot.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk3/boot.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk4/boot.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk4/boot.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk4/boot.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk5/boot.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk5/boot.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk6/boot.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk6/boot.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk6/boot.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk7/boot.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk7/boot.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk7/boot.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk8/boot.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk8/boot.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk8/boot.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk9/boot.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk9/boot.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk9/boot.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk9/boot.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk9/boot.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk9/boot.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk8/boot.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk8/boot.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk8/boot.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk7/boot.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk7/boot.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk6/boot.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk6/boot.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk6/boot.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/boot.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/boot.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk4/boot.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk4/boot.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk3/boot.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk3/boot.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk3/boot.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk2/boot.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk2/boot.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk2/boot.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/boot.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/boot.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_persistence@legacy-engines-hostile@render:
    - shard-glk:          [PASS][96] -> [FAIL][97] ([i915#2410])
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk3/igt@gem_ctx_persistence@legacy-engines-hostile@render.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk7/igt@gem_ctx_persistence@legacy-engines-hostile@render.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-glk:          NOTRUN -> [FAIL][98] ([i915#2842]) +1 similar issue
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-none@vecs0:
    - shard-apl:          NOTRUN -> [FAIL][99] ([i915#2842])
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@gem_exec_fair@basic-none@vecs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-tglb:         [PASS][100] -> [FAIL][101] ([i915#2842])
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglb1/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb2/igt@gem_exec_fair@basic-pace-share@rcs0.html
    - shard-glk:          [PASS][102] -> [FAIL][103] ([i915#2842])
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk5/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@vcs0:
    - shard-iclb:         [PASS][104] -> [FAIL][105] ([i915#2842])
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb2/igt@gem_exec_fair@basic-pace@vcs0.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb2/igt@gem_exec_fair@basic-pace@vcs0.html

  * igt@gem_exec_whisper@basic-fds:
    - shard-glk:          [PASS][106] -> [DMESG-WARN][107] ([i915#118])
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk6/igt@gem_exec_whisper@basic-fds.html
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk9/igt@gem_exec_whisper@basic-fds.html

  * igt@gem_lmem_swapping@heavy-random:
    - shard-kbl:          NOTRUN -> [SKIP][108] ([fdo#109271] / [i915#4613])
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@gem_lmem_swapping@heavy-random.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-kbl:          NOTRUN -> [FAIL][109] ([i915#454])
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rc6_residency@rc6-idle:
    - shard-iclb:         NOTRUN -> [WARN][110] ([i915#1804] / [i915#2684])
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@i915_pm_rc6_residency@rc6-idle.html

  * igt@i915_selftest@live@hangcheck:
    - shard-snb:          [PASS][111] -> [INCOMPLETE][112] ([i915#3921])
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-snb5/igt@i915_selftest@live@hangcheck.html
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-snb2/igt@i915_selftest@live@hangcheck.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [PASS][113] -> [DMESG-WARN][114] ([i915#180]) +1 similar issue
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-apl6/igt@i915_suspend@fence-restore-tiled2untiled.html
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip:
    - shard-apl:          NOTRUN -> [SKIP][115] ([fdo#109271] / [i915#3777]) +2 similar issues
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl4/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html
    - shard-glk:          NOTRUN -> [SKIP][116] ([fdo#109271] / [i915#3777]) +2 similar issues
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/igt@kms_big_fb@x-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html

  * igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_mc_ccs:
    - shard-iclb:         NOTRUN -> [SKIP][117] ([fdo#109278] / [i915#3886])
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_mc_ccs.html

  * igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_rc_ccs_cc:
    - shard-kbl:          NOTRUN -> [SKIP][118] ([fdo#109271] / [i915#3886]) +1 similar issue
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@kms_ccs@pipe-a-bad-rotation-90-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs:
    - shard-glk:          NOTRUN -> [SKIP][119] ([fdo#109271]) +71 similar issues
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs.html

  * igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc:
    - shard-apl:          NOTRUN -> [SKIP][120] ([fdo#109271] / [i915#3886])
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc.html
    - shard-glk:          NOTRUN -> [SKIP][121] ([fdo#109271] / [i915#3886]) +1 similar issue
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_chamelium@vga-hpd:
    - shard-apl:          NOTRUN -> [SKIP][122] ([fdo#109271] / [fdo#111827]) +4 similar issues
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl4/igt@kms_chamelium@vga-hpd.html

  * igt@kms_color_chamelium@pipe-c-ctm-0-25:
    - shard-kbl:          NOTRUN -> [SKIP][123] ([fdo#109271] / [fdo#111827]) +5 similar issues
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl3/igt@kms_color_chamelium@pipe-c-ctm-0-25.html

  * igt@kms_color_chamelium@pipe-d-ctm-0-25:
    - shard-glk:          NOTRUN -> [SKIP][124] ([fdo#109271] / [fdo#111827]) +5 similar issues
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/igt@kms_color_chamelium@pipe-d-ctm-0-25.html

  * igt@kms_content_protection@atomic:
    - shard-apl:          NOTRUN -> [TIMEOUT][125] ([i915#1319])
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@kms_content_protection@atomic.html

  * igt@kms_cursor_crc@pipe-d-cursor-max-size-onscreen:
    - shard-iclb:         NOTRUN -> [SKIP][126] ([fdo#109278])
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@kms_cursor_crc@pipe-d-cursor-max-size-onscreen.html

  * igt@kms_cursor_legacy@cursorb-vs-flipb-atomic-transitions-varying-size:
    - shard-tglb:         NOTRUN -> [SKIP][127] ([fdo#109274] / [fdo#111825])
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb3/igt@kms_cursor_legacy@cursorb-vs-flipb-atomic-transitions-varying-size.html

  * igt@kms_cursor_legacy@flip-vs-cursor-varying-size:
    - shard-tglb:         [PASS][128] -> [FAIL][129] ([i915#2346])
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglb3/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb7/igt@kms_cursor_legacy@flip-vs-cursor-varying-size.html

  * igt@kms_cursor_legacy@pipe-d-torture-bo:
    - shard-apl:          NOTRUN -> [SKIP][130] ([fdo#109271] / [i915#533])
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl4/igt@kms_cursor_legacy@pipe-d-torture-bo.html
    - shard-glk:          NOTRUN -> [SKIP][131] ([fdo#109271] / [i915#533])
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk1/igt@kms_cursor_legacy@pipe-d-torture-bo.html

  * igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1:
    - shard-skl:          [PASS][132] -> [FAIL][133] ([i915#2122])
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-skl2/igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-skl8/igt@kms_flip@plain-flip-ts-check-interruptible@a-edp1.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-blt:
    - shard-kbl:          NOTRUN -> [SKIP][134] ([fdo#109271]) +77 similar issues
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-indfb-draw-blt:
    - shard-tglb:         NOTRUN -> [SKIP][135] ([fdo#109280] / [fdo#111825]) +1 similar issue
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb3/igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-indfb-draw-blt.html

  * igt@kms_pipe_b_c_ivb@disable-pipe-b-enable-pipe-c:
    - shard-apl:          NOTRUN -> [SKIP][136] ([fdo#109271]) +62 similar issues
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl4/igt@kms_pipe_b_c_ivb@disable-pipe-b-enable-pipe-c.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-kbl:          [PASS][137] -> [DMESG-WARN][138] ([i915#180]) +3 similar issues
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-kbl6/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-apl:          NOTRUN -> [FAIL][139] ([fdo#108145] / [i915#265])
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl4/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [PASS][140] -> [FAIL][141] ([fdo#108145] / [i915#265])
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-kbl:          NOTRUN -> [FAIL][142] ([fdo#108145] / [i915#265])
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-kbl1/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb:
    - shard-glk:          NOTRUN -> [FAIL][143] ([i915#265])
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html
    - shard-apl:          NOTRUN -> [FAIL][144] ([i915#265])
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-tglb:         NOTRUN -> [FAIL][145] ([i915#132] / [i915#3467])
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb3/igt@kms_psr@psr2_no_drrs.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][146] -> [SKIP][147] ([fdo#109441]) +1 similar issue
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [147]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb5/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-glk:          [PASS][148] -> [FAIL][149] ([i915#31])
   [148]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk6/igt@kms_setmode@basic.html
   [149]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk8/igt@kms_setmode@basic.html

  * igt@kms_writeback@writeback-invalid-parameters:
    - shard-apl:          NOTRUN -> [SKIP][150] ([fdo#109271] / [i915#2437])
   [150]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@kms_writeback@writeback-invalid-parameters.html
    - shard-glk:          NOTRUN -> [SKIP][151] ([fdo#109271] / [i915#2437])
   [151]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/igt@kms_writeback@writeback-invalid-parameters.html

  * igt@kms_writeback@writeback-pixel-formats:
    - shard-iclb:         NOTRUN -> [SKIP][152] ([i915#2437])
   [152]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb7/igt@kms_writeback@writeback-pixel-formats.html

  * igt@nouveau_crc@pipe-c-source-outp-inactive:
    - shard-tglb:         NOTRUN -> [SKIP][153] ([i915#2530])
   [153]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb3/igt@nouveau_crc@pipe-c-source-outp-inactive.html

  * igt@perf@polling:
    - shard-skl:          [PASS][154] -> [FAIL][155] ([i915#1542])
   [154]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-skl7/igt@perf@polling.html
   [155]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-skl10/igt@perf@polling.html

  * igt@perf@polling-small-buf:
    - shard-skl:          [PASS][156] -> [FAIL][157] ([i915#1722])
   [156]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-skl10/igt@perf@polling-small-buf.html
   [157]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-skl10/igt@perf@polling-small-buf.html

  * igt@sysfs_clients@recycle:
    - shard-apl:          NOTRUN -> [SKIP][158] ([fdo#109271] / [i915#2994])
   [158]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@sysfs_clients@recycle.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - {shard-rkl}:        [SKIP][159] ([i915#2582]) -> [PASS][160]
   [159]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-1/igt@fbdev@read.html
   [160]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-6/igt@fbdev@read.html

  * igt@feature_discovery@psr2:
    - shard-iclb:         [SKIP][161] ([i915#658]) -> [PASS][162]
   [161]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb5/igt@feature_discovery@psr2.html
   [162]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb2/igt@feature_discovery@psr2.html

  * igt@gem_eio@kms:
    - shard-tglb:         [FAIL][163] ([i915#232]) -> [PASS][164]
   [163]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglb5/igt@gem_eio@kms.html
   [164]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglb7/igt@gem_eio@kms.html

  * igt@gem_eio@suspend:
    - {shard-rkl}:        [FAIL][165] ([i915#5115]) -> [PASS][166]
   [165]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-4/igt@gem_eio@suspend.html
   [166]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/igt@gem_eio@suspend.html

  * igt@gem_eio@unwedge-stress:
    - {shard-tglu}:       [TIMEOUT][167] ([i915#3063] / [i915#3648]) -> [PASS][168]
   [167]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglu-2/igt@gem_eio@unwedge-stress.html
   [168]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglu-3/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_balancer@parallel-balancer:
    - shard-iclb:         [SKIP][169] ([i915#4525]) -> [PASS][170]
   [169]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-iclb8/igt@gem_exec_balancer@parallel-balancer.html
   [170]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-iclb4/igt@gem_exec_balancer@parallel-balancer.html

  * igt@gem_exec_params@rel-constants-invalid:
    - {shard-rkl}:        [INCOMPLETE][171] ([i915#2295]) -> [PASS][172]
   [171]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/igt@gem_exec_params@rel-constants-invalid.html
   [172]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-4/igt@gem_exec_params@rel-constants-invalid.html

  * igt@gem_sync@basic-store-each:
    - {shard-rkl}:        [INCOMPLETE][173] -> [PASS][174]
   [173]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-rkl-5/igt@gem_sync@basic-store-each.html
   [174]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-rkl-2/igt@gem_sync@basic-store-each.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [DMESG-WARN][175] ([i915#180]) -> [PASS][176] +1 similar issue
   [175]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-apl2/igt@gem_workarounds@suspend-resume-context.html
   [176]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-apl1/igt@gem_workarounds@suspend-resume-context.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [DMESG-WARN][177] ([i915#1436] / [i915#716]) -> [PASS][178]
   [177]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-glk1/igt@gen9_exec_parse@allowed-all.html
   [178]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-glk5/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a:
    - {shard-tglu}:       [FAIL][179] ([i915#3825]) -> [PASS][180]
   [179]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-tglu-5/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html
   [180]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22262/shard-tglu-1/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html

  * igt@i915_pm_rpm@modeset-lpsp:
    - {shard-dg1}:        [SKIP][181] ([i915#1397]) -> [PASS][182]
   [181]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11229/shard-dg1-18/igt@i915_pm_rpm@modeset-lpsp.html
   [182]: https:/

== Logs ==

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

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

Re: [Intel-gfx] [PATCH 5/6] drm/i915: Split pre-icl vs. icl+ SAGV hooks apart

[Lisovskiy, Stanislav]

On Mon, Feb 14, 2022 at 11:18:10AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> To further reduce the confusion between the pre-icl vs. icl+
> SAGV codepaths let's do a full split.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 120 ++++++++++++++++++++------------
>  1 file changed, 77 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 068870b17c43..8b70cdc3b58b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3785,34 +3785,44 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
>  	return 0;
>  }
>  
> -void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> +static void skl_sagv_pre_plane_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	const struct intel_bw_state *new_bw_state =
> +		intel_atomic_get_new_bw_state(state);
> +
> +	if (!new_bw_state)
> +		return;
> +
> +	if (!intel_can_enable_sagv(i915, new_bw_state))
> +		intel_disable_sagv(i915);
> +}
> +
> +static void skl_sagv_post_plane_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	const struct intel_bw_state *new_bw_state =
> +		intel_atomic_get_new_bw_state(state);
> +
> +	if (!new_bw_state)
> +		return;
> +
> +	if (intel_can_enable_sagv(i915, new_bw_state))
> +		intel_enable_sagv(i915);
> +}
> +
> +static void icl_sagv_pre_plane_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_bw_state *new_bw_state;
> -	const struct intel_bw_state *old_bw_state;
> -	u32 new_mask = 0;
> +	const struct intel_bw_state *old_bw_state =
> +		intel_atomic_get_old_bw_state(state);
> +	const struct intel_bw_state *new_bw_state =
> +		intel_atomic_get_new_bw_state(state);
> +	u32 new_mask;
>  
> -	/*
> -	 * Just return if we can't control SAGV or don't have it.
> -	 * This is different from situation when we have SAGV but just can't
> -	 * afford it due to DBuf limitation - in case if SAGV is completely
> -	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> -	 * as it will throw an error. So have to check it here.
> -	 */
> -	if (!intel_has_sagv(dev_priv))
> -		return;
> -
> -	new_bw_state = intel_atomic_get_new_bw_state(state);
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11) {
> -		if (!intel_can_enable_sagv(dev_priv, new_bw_state))
> -			intel_disable_sagv(dev_priv);
> -		return;
> -	}
> -
> -	old_bw_state = intel_atomic_get_old_bw_state(state);
>  	/*
>  	 * Nothing to mask
>  	 */
> @@ -3837,34 +3847,18 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
>  	icl_pcode_restrict_qgv_points(dev_priv, new_mask);
>  }
>  
> -void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> +static void icl_sagv_post_plane_update(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	const struct intel_bw_state *new_bw_state;
> -	const struct intel_bw_state *old_bw_state;
> +	const struct intel_bw_state *old_bw_state =
> +		intel_atomic_get_old_bw_state(state);
> +	const struct intel_bw_state *new_bw_state =
> +		intel_atomic_get_new_bw_state(state);
>  	u32 new_mask = 0;
>  
> -	/*
> -	 * Just return if we can't control SAGV or don't have it.
> -	 * This is different from situation when we have SAGV but just can't
> -	 * afford it due to DBuf limitation - in case if SAGV is completely
> -	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> -	 * as it will throw an error. So have to check it here.
> -	 */
> -	if (!intel_has_sagv(dev_priv))
> -		return;
> -
> -	new_bw_state = intel_atomic_get_new_bw_state(state);
>  	if (!new_bw_state)
>  		return;
>  
> -	if (DISPLAY_VER(dev_priv) < 11) {
> -		if (intel_can_enable_sagv(dev_priv, new_bw_state))
> -			intel_enable_sagv(dev_priv);
> -		return;
> -	}
> -
> -	old_bw_state = intel_atomic_get_old_bw_state(state);
>  	/*
>  	 * Nothing to unmask
>  	 */
> @@ -3882,6 +3876,46 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
>  	icl_pcode_restrict_qgv_points(dev_priv, new_mask);
>  }
>  
> +void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +
> +	/*
> +	 * Just return if we can't control SAGV or don't have it.
> +	 * This is different from situation when we have SAGV but just can't
> +	 * afford it due to DBuf limitation - in case if SAGV is completely
> +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> +	 * as it will throw an error. So have to check it here.
> +	 */
> +	if (!intel_has_sagv(i915))
> +		return;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		icl_sagv_pre_plane_update(state);
> +	else
> +		skl_sagv_pre_plane_update(state);
> +}
> +
> +void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +
> +	/*
> +	 * Just return if we can't control SAGV or don't have it.
> +	 * This is different from situation when we have SAGV but just can't
> +	 * afford it due to DBuf limitation - in case if SAGV is completely
> +	 * disabled in a BIOS, we are not even allowed to send a PCode request,
> +	 * as it will throw an error. So have to check it here.
> +	 */
> +	if (!intel_has_sagv(i915))
> +		return;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		icl_sagv_post_plane_update(state);
> +	else
> +		skl_sagv_post_plane_update(state);
> +}
> +
>  static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -- 
> 2.34.1
>