[Intel-gfx] [PATCH 0/4] CDCLK churn: move checks to atomic check

[Intel-gfx] [PATCH 0/4] CDCLK churn: move checks to atomic check

[Anusha Srivatsa]

The intention is to check for squashing, crawling and modeset conditions
at atomic check phase and prepare for commit phase. This basically
means the in-flight cdclk state is available. intel_cdclk_can_squash(),
intel_cdclk_can_crawl() and intel_cdclk_needs_modeset() have changes
to accommodate this.

Anusha Srivatsa (4):
  drm/i915/display: Add CDCLK actions to intel_cdclk_state
  drm/i915/squash: s/intel_cdclk_can_squash/intel_cdclk_squash
  drm/i915/display: s/intel_cdclk_can_crawl/intel_cdclk_crawl
  drm/i915/display: Add cdclk checks to atomic check

 drivers/gpu/drm/i915/display/intel_cdclk.c | 150 +++++++++++++--------
 drivers/gpu/drm/i915/display/intel_cdclk.h |  13 ++
 2 files changed, 108 insertions(+), 55 deletions(-)

-- 
2.25.1

[Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Anusha Srivatsa]

This is a prep patch for what the rest of the series does.

Add existing actions that change cdclk - squash, crawl, modeset to
intel_cdclk_state so we have access to the cdclk values
that are in transition.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index b535cf6a7d9e..43835688ee02 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -15,6 +15,14 @@ struct drm_i915_private;
 struct intel_atomic_state;
 struct intel_crtc_state;
 
+enum cdclk_actions {
+	INTEL_CDCLK_MODESET = 0,
+	INTEL_CDCLK_SQUASH,
+	INTEL_CDCLK_CRAWL,
+	INTEL_CDCLK_NOOP,
+	MAX_CDCLK_ACTIONS
+};
+
 struct intel_cdclk_config {
 	unsigned int cdclk, vco, ref, bypass;
 	u8 voltage_level;
@@ -51,6 +59,11 @@ struct intel_cdclk_state {
 
 	/* bitmask of active pipes */
 	u8 active_pipes;
+
+	struct cdclk_step {
+		enum cdclk_actions action;
+		u32 cdclk;
+	} steps[MAX_CDCLK_ACTIONS];
 };
 
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
-- 
2.25.1

[Intel-gfx] [PATCH 2/4] drm/i915/squash: s/intel_cdclk_can_squash/intel_cdclk_squash

[Anusha Srivatsa]

Apart from checking if squashing can be performed,
accommodate accessing in-flight cdclk state for any changes
that are needed during commit phase.

v2: Move squashing bits to switch case.(Anusha)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 62 ++++++++++++++--------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 86a22c3766e5..f98fd48fe905 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1693,12 +1693,18 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 			  const struct intel_cdclk_config *cdclk_config,
 			  enum pipe pipe)
 {
+	struct intel_cdclk_state *cdclk_state = to_intel_cdclk_state(dev_priv->cdclk.obj.state);
+	struct intel_atomic_state *state = cdclk_state->base.state;
+	struct intel_cdclk_state *new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+	struct cdclk_step *cdclk_steps = new_cdclk_state->steps;
 	int cdclk = cdclk_config->cdclk;
 	int vco = cdclk_config->vco;
+	u32 squash_ctl = 0;
 	u32 val;
 	u16 waveform;
 	int clock;
 	int ret;
+	int i;
 
 	/* Inform power controller of upcoming frequency change. */
 	if (DISPLAY_VER(dev_priv) >= 11)
@@ -1742,21 +1748,27 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 
 	waveform = cdclk_squash_waveform(dev_priv, cdclk);
 
-	if (waveform)
+	if (waveform && has_cdclk_squasher(dev_priv)) {
 		clock = vco / 2;
-	else
+		for (i = 0; i < MAX_CDCLK_ACTIONS; i++) {
+			switch (cdclk_steps[i].action) {
+			case INTEL_CDCLK_SQUASH:
+				waveform =  cdclk_squash_waveform(dev_priv, cdclk_steps[i].cdclk);
+				squash_ctl = CDCLK_SQUASH_ENABLE |
+					     CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
+				intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
+				break;
+			case INTEL_CDCLK_NOOP:
+			case INTEL_CDCLK_CRAWL:
+			case INTEL_CDCLK_MODESET:
+				break;
+			default:
+				break;
+			}
+		}
+	} else
 		clock = cdclk;
 
-	if (has_cdclk_squasher(dev_priv)) {
-		u32 squash_ctl = 0;
-
-		if (waveform)
-			squash_ctl = CDCLK_SQUASH_ENABLE |
-				CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
-
-		intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
-	}
-
 	val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
 		bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
 		skl_cdclk_decimal(cdclk);
@@ -1966,10 +1978,11 @@ static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
 		a->ref == b->ref;
 }
 
-static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
-				   const struct intel_cdclk_config *a,
-				   const struct intel_cdclk_config *b)
+static bool intel_cdclk_squash(struct drm_i915_private *dev_priv,
+			       const struct intel_cdclk_state *a,
+			       struct intel_cdclk_state *b)
 {
+	struct cdclk_step *cdclk_transition = b->steps;
 	/*
 	 * FIXME should store a bit more state in intel_cdclk_config
 	 * to differentiate squasher vs. cd2x divider properly. For
@@ -1978,11 +1991,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
 	 */
 	if (!has_cdclk_squasher(dev_priv))
 		return false;
+	
+	cdclk_transition[0].action = INTEL_CDCLK_SQUASH;
+	cdclk_transition[0].cdclk = b->actual.cdclk;
+	cdclk_transition[1].action = INTEL_CDCLK_NOOP;
+	cdclk_transition[1].cdclk = b->actual.cdclk;
 
-	return a->cdclk != b->cdclk &&
-		a->vco != 0 &&
-		a->vco == b->vco &&
-		a->ref == b->ref;
+	return a->actual.cdclk != b->actual.cdclk &&
+		a->actual.vco != 0 &&
+		a->actual.vco == b->actual.vco &&
+		a->actual.ref == b->actual.ref;
 }
 
 /**
@@ -2758,9 +2776,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 			pipe = INVALID_PIPE;
 	}
 
-	if (intel_cdclk_can_squash(dev_priv,
-				   &old_cdclk_state->actual,
-				   &new_cdclk_state->actual)) {
+	if (intel_cdclk_squash(dev_priv,
+			       old_cdclk_state,
+			       new_cdclk_state)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "Can change cdclk via squasher\n");
 	} else if (intel_cdclk_can_crawl(dev_priv,
-- 
2.25.1

[Intel-gfx] [PATCH 3/4] drm/i915/display: s/intel_cdclk_can_crawl/intel_cdclk_crawl

[Anusha Srivatsa]

Apart from checking if crawling can be performed,
accommodate accessing in-flight cdclk state for any changes
that are needed during commit phase.

v2: Move crawling steps to a switch case (anusha)
Cc: Matt Roper <matthew.d.roper@intel.com>

Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 45 +++++++++++++---------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index f98fd48fe905..7bba10635c5e 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -38,6 +38,7 @@
 #include "intel_psr.h"
 #include "vlv_sideband.h"
 
+#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)
 /**
  * DOC: CDCLK / RAWCLK
  *
@@ -1727,10 +1728,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 		return;
 	}
 
-	if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0) {
-		if (dev_priv->cdclk.hw.vco != vco)
-			adlp_cdclk_pll_crawl(dev_priv, vco);
-	} else if (DISPLAY_VER(dev_priv) >= 11) {
+	if (!ADLP_CDCLK_CRAWL(dev_priv, vco) && DISPLAY_VER(dev_priv) >= 11) {
 		if (dev_priv->cdclk.hw.vco != 0 &&
 		    dev_priv->cdclk.hw.vco != vco)
 			icl_cdclk_pll_disable(dev_priv);
@@ -1748,18 +1746,21 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 
 	waveform = cdclk_squash_waveform(dev_priv, cdclk);
 
-	if (waveform && has_cdclk_squasher(dev_priv)) {
-		clock = vco / 2;
+	if ((waveform && has_cdclk_squasher(dev_priv)) || ADLP_CDCLK_CRAWL(dev_priv, vco)) {
 		for (i = 0; i < MAX_CDCLK_ACTIONS; i++) {
 			switch (cdclk_steps[i].action) {
+			case INTEL_CDCLK_CRAWL:
+				adlp_cdclk_pll_crawl(dev_priv, vco);
+				clock = cdclk;
+				break;
 			case INTEL_CDCLK_SQUASH:
 				waveform =  cdclk_squash_waveform(dev_priv, cdclk_steps[i].cdclk);
 				squash_ctl = CDCLK_SQUASH_ENABLE |
 					     CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
 				intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
+				clock = vco / 2;
 				break;
 			case INTEL_CDCLK_NOOP:
-			case INTEL_CDCLK_CRAWL:
 			case INTEL_CDCLK_MODESET:
 				break;
 			default:
@@ -1956,10 +1957,11 @@ void intel_cdclk_uninit_hw(struct drm_i915_private *i915)
 		skl_cdclk_uninit_hw(i915);
 }
 
-static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
-				  const struct intel_cdclk_config *a,
-				  const struct intel_cdclk_config *b)
+static bool intel_cdclk_crawl(struct drm_i915_private *dev_priv,
+			      const struct intel_cdclk_state *a,
+			      struct intel_cdclk_state *b)
 {
+	struct cdclk_step *cdclk_transition = b->steps;
 	int a_div, b_div;
 
 	if (!HAS_CDCLK_CRAWL(dev_priv))
@@ -1969,13 +1971,18 @@ static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
 	 * The vco and cd2x divider will change independently
 	 * from each, so we disallow cd2x change when crawling.
 	 */
-	a_div = DIV_ROUND_CLOSEST(a->vco, a->cdclk);
-	b_div = DIV_ROUND_CLOSEST(b->vco, b->cdclk);
+	a_div = DIV_ROUND_CLOSEST(a->actual.vco, a->actual.cdclk);
+	b_div = DIV_ROUND_CLOSEST(b->actual.vco, b->actual.cdclk);
 
-	return a->vco != 0 && b->vco != 0 &&
-		a->vco != b->vco &&
-		a_div == b_div &&
-		a->ref == b->ref;
+	cdclk_transition[0].action = INTEL_CDCLK_CRAWL;
+	cdclk_transition[0].cdclk = b->actual.cdclk;
+	cdclk_transition[1].action = INTEL_CDCLK_NOOP;
+	cdclk_transition[1].cdclk = b->actual.cdclk;
+
+	return a->actual.vco != 0 && b->actual.vco != 0 &&
+		a->actual.vco != b->actual.vco &&
+ 		a_div == b_div && 
+		a->actual.ref == b->actual.ref;
 }
 
 static bool intel_cdclk_squash(struct drm_i915_private *dev_priv,
@@ -2781,9 +2788,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 			       new_cdclk_state)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "Can change cdclk via squasher\n");
-	} else if (intel_cdclk_can_crawl(dev_priv,
-					 &old_cdclk_state->actual,
-					 &new_cdclk_state->actual)) {
+	} else if (intel_cdclk_crawl(dev_priv,
+				     old_cdclk_state,
+				     new_cdclk_state)) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "Can change cdclk via crawl\n");
 	} else if (pipe != INVALID_PIPE) {
-- 
2.25.1

[Intel-gfx] [PATCH 4/4] drm/i915/display: Add cdclk checks to atomic check

[Anusha Srivatsa]

Checking cdclk conditions during atomic check and preparing
for commit phase so we can have atomic commit as simple
as possible. Add the specific steps to be taken during
cdclk changes, prepare for squashing, crawling and modeset
scenarios.

v2: Add intel_cdclk_modeset() similar to intel_cdclk_squash()
and intel_cdclk_crawl().

Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 61 ++++++++++++++--------
 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 7bba10635c5e..cb58fc857484 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -38,7 +38,6 @@
 #include "intel_psr.h"
 #include "vlv_sideband.h"
 
-#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)
 /**
  * DOC: CDCLK / RAWCLK
  *
@@ -1728,27 +1727,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 		return;
 	}
 
-	if (!ADLP_CDCLK_CRAWL(dev_priv, vco) && DISPLAY_VER(dev_priv) >= 11) {
-		if (dev_priv->cdclk.hw.vco != 0 &&
-		    dev_priv->cdclk.hw.vco != vco)
-			icl_cdclk_pll_disable(dev_priv);
-
-		if (dev_priv->cdclk.hw.vco != vco)
-			icl_cdclk_pll_enable(dev_priv, vco);
-	} else {
-		if (dev_priv->cdclk.hw.vco != 0 &&
-		    dev_priv->cdclk.hw.vco != vco)
-			bxt_de_pll_disable(dev_priv);
-
-		if (dev_priv->cdclk.hw.vco != vco)
-			bxt_de_pll_enable(dev_priv, vco);
-	}
-
 	waveform = cdclk_squash_waveform(dev_priv, cdclk);
 
-	if ((waveform && has_cdclk_squasher(dev_priv)) || ADLP_CDCLK_CRAWL(dev_priv, vco)) {
-		for (i = 0; i < MAX_CDCLK_ACTIONS; i++) {
-			switch (cdclk_steps[i].action) {
+	for (i = 0; i < MAX_CDCLK_ACTIONS; i++) {
+		switch (cdclk_steps[i].action) {
 			case INTEL_CDCLK_CRAWL:
 				adlp_cdclk_pll_crawl(dev_priv, vco);
 				clock = cdclk;
@@ -1760,15 +1742,28 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
 				intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
 				clock = vco / 2;
 				break;
-			case INTEL_CDCLK_NOOP:
 			case INTEL_CDCLK_MODESET:
+				if (DISPLAY_VER(dev_priv) >= 11) {
+					if (dev_priv->cdclk.hw.vco != 0 &&
+					    dev_priv->cdclk.hw.vco != vco)
+						icl_cdclk_pll_disable(dev_priv);
+					if (dev_priv->cdclk.hw.vco != vco)
+						icl_cdclk_pll_enable(dev_priv, vco);
+				} else {
+					if (dev_priv->cdclk.hw.vco != 0 &&
+					    dev_priv->cdclk.hw.vco != vco)
+						bxt_de_pll_disable(dev_priv);
+					if (dev_priv->cdclk.hw.vco != vco)
+						bxt_de_pll_enable(dev_priv, vco);
+				}
+				clock = cdclk;
+				break;
+			case INTEL_CDCLK_NOOP:
 				break;
 			default:
 				break;
 			}
 		}
-	} else
-		clock = cdclk;
 
 	val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
 		bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
@@ -2010,6 +2005,24 @@ static bool intel_cdclk_squash(struct drm_i915_private *dev_priv,
 		a->actual.ref == b->actual.ref;
 }
 
+static void intel_cdclk_modeset(struct drm_i915_private *i915,
+				const struct intel_cdclk_config *a,
+				const struct intel_cdclk_config *b)
+{
+	struct intel_cdclk_state *new_cdclk_state;
+	struct cdclk_step *cdclk_transition;
+	struct intel_cdclk_state *cdclk_state =  to_intel_cdclk_state(i915->cdclk.obj.state);
+	struct intel_atomic_state *state = cdclk_state->base.state;
+
+	new_cdclk_state = intel_atomic_get_new_cdclk_state(state);
+	cdclk_transition = new_cdclk_state->steps;
+
+	cdclk_transition[0].action = INTEL_CDCLK_MODESET;
+	cdclk_transition[0].cdclk = b->cdclk;
+	cdclk_transition[1].action = INTEL_CDCLK_NOOP;
+	cdclk_transition[1].cdclk = b->cdclk;
+}
+
 /**
  * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
  *                             configurations requires a modeset on all pipes
@@ -2801,6 +2814,8 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 			    pipe_name(pipe));
 	} else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual,
 					     &new_cdclk_state->actual)) {
+		intel_cdclk_modeset(dev_priv, &old_cdclk_state->actual,
+				    &new_cdclk_state->actual);
 		/* All pipes must be switched off while we change the cdclk. */
 		ret = intel_modeset_all_pipes(state);
 		if (ret)
-- 
2.25.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for CDCLK churn: move checks to atomic check

[Patchwork]

== Series Details ==

Series: CDCLK churn: move checks to atomic check
URL   : https://patchwork.freedesktop.org/series/107522/
State : warning

== Summary ==

Error: dim checkpatch failed
c3061d754e21 drm/i915/display: Add CDCLK actions to intel_cdclk_state
d6438fdf45eb drm/i915/squash: s/intel_cdclk_can_squash/intel_cdclk_squash
-:43: CHECK:BRACES: braces {} should be used on all arms of this statement
#43: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1751:
+	if (waveform && has_cdclk_squasher(dev_priv)) {
[...]
+		}
[...]

-:62: CHECK:BRACES: Unbalanced braces around else statement
#62: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1769:
+	} else

-:97: ERROR:TRAILING_WHITESPACE: trailing whitespace
#97: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1994:
+^I$

total: 1 errors, 0 warnings, 2 checks, 103 lines checked
9f16f8721589 drm/i915/display: s/intel_cdclk_can_crawl/intel_cdclk_crawl
-:23: WARNING:LONG_LINE: line length of 108 exceeds 100 columns
#23: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:41:
+#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)

-:23: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'dev_priv' - possible side-effects?
#23: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:41:
+#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)

-:23: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'vco' - possible side-effects?
#23: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:41:
+#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)

-:23: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'vco' may be better as '(vco)' to avoid precedence issues
#23: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:41:
+#define ADLP_CDCLK_CRAWL(dev_priv, vco)	(HAS_CDCLK_CRAWL(dev_priv) && dev_priv->cdclk.hw.vco > 0 && vco > 0)

-:99: ERROR:TRAILING_WHITESPACE: trailing whitespace
#99: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1984:
+ ^I^Ia_div == b_div && $

-:99: ERROR:CODE_INDENT: code indent should use tabs where possible
#99: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1984:
+ ^I^Ia_div == b_div && $

-:99: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#99: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1984:
+ ^I^Ia_div == b_div && $

-:99: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#99: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1984:
+ ^I^Ia_div == b_div && $

total: 2 errors, 3 warnings, 3 checks, 92 lines checked
b882cdeeec1f drm/i915/display: Add cdclk checks to atomic check

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for CDCLK churn: move checks to atomic check

[Patchwork]

== Series Details ==

Series: CDCLK churn: move checks to atomic check
URL   : https://patchwork.freedesktop.org/series/107522/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for CDCLK churn: move checks to atomic check

[Patchwork]

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

== Series Details ==

Series: CDCLK churn: move checks to atomic check
URL   : https://patchwork.freedesktop.org/series/107522/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12004 -> Patchwork_107522v1
====================================================

Summary
-------

  **FAILURE**

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

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

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

  Additional (1): fi-tgl-dsi 
  Missing    (3): bat-adln-1 bat-dg2-10 bat-dg1-5 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-rkl-11600:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-rkl-11600/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-rkl-11600/igt@i915_module_load@load.html
    - fi-skl-6700k2:      [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-skl-6700k2/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-skl-6700k2/igt@i915_module_load@load.html
    - fi-cfl-8700k:       [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-cfl-8700k/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-8700k/igt@i915_module_load@load.html
    - fi-adl-ddr5:        [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-adl-ddr5/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-adl-ddr5/igt@i915_module_load@load.html
    - fi-cfl-guc:         [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-cfl-guc/igt@i915_module_load@load.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-guc/igt@i915_module_load@load.html
    - fi-bdw-5557u:       [PASS][11] -> [INCOMPLETE][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-bdw-5557u/igt@i915_module_load@load.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bdw-5557u/igt@i915_module_load@load.html
    - fi-cfl-8109u:       [PASS][13] -> [INCOMPLETE][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-cfl-8109u/igt@i915_module_load@load.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-8109u/igt@i915_module_load@load.html

  * igt@kms_busy@basic@flip:
    - fi-bsw-kefka:       [PASS][15] -> [INCOMPLETE][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-bsw-kefka/igt@kms_busy@basic@flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bsw-kefka/igt@kms_busy@basic@flip.html
    - fi-skl-6600u:       [PASS][17] -> [INCOMPLETE][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-skl-6600u/igt@kms_busy@basic@flip.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-skl-6600u/igt@kms_busy@basic@flip.html
    - fi-glk-j4005:       [PASS][19] -> [INCOMPLETE][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-glk-j4005/igt@kms_busy@basic@flip.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-glk-j4005/igt@kms_busy@basic@flip.html

  * igt@kms_force_connector_basic@force-connector-state:
    - fi-bdw-gvtdvm:      [PASS][21] -> [DMESG-WARN][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-bdw-gvtdvm/igt@kms_force_connector_basic@force-connector-state.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bdw-gvtdvm/igt@kms_force_connector_basic@force-connector-state.html

  
#### Warnings ####

  * igt@i915_module_load@load:
    - fi-rkl-guc:         [TIMEOUT][23] ([i915#6627]) -> [INCOMPLETE][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-rkl-guc/igt@i915_module_load@load.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-rkl-guc/igt@i915_module_load@load.html
    - bat-dg1-6:          [TIMEOUT][25] ([i915#6627]) -> [INCOMPLETE][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/bat-dg1-6/igt@i915_module_load@load.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-dg1-6/igt@i915_module_load@load.html

  
#### Suppressed ####

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

  * igt@debugfs_test@read_all_entries:
    - {bat-dg2-8}:        [INCOMPLETE][27] ([i915#6637]) -> [INCOMPLETE][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/bat-dg2-8/igt@debugfs_test@read_all_entries.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-dg2-8/igt@debugfs_test@read_all_entries.html

  * igt@i915_module_load@load:
    - {bat-jsl-3}:        [PASS][29] -> [INCOMPLETE][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/bat-jsl-3/igt@i915_module_load@load.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-jsl-3/igt@i915_module_load@load.html
    - {fi-tgl-dsi}:       NOTRUN -> [INCOMPLETE][31]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-tgl-dsi/igt@i915_module_load@load.html
    - {bat-dg2-9}:        [TIMEOUT][32] ([i915#6637]) -> [INCOMPLETE][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/bat-dg2-9/igt@i915_module_load@load.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-dg2-9/igt@i915_module_load@load.html
    - {bat-rpls-2}:       [TIMEOUT][34] ([i915#6627]) -> [INCOMPLETE][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/bat-rpls-2/igt@i915_module_load@load.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-rpls-2/igt@i915_module_load@load.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - {fi-jsl-1}:         [PASS][36] -> [INCOMPLETE][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {fi-ehl-2}:         [PASS][38] -> [INCOMPLETE][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-ehl-2/igt@i915_pm_rpm@basic-pci-d3-state.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-ehl-2/igt@i915_pm_rpm@basic-pci-d3-state.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@load:
    - fi-bdw-gvtdvm:      [PASS][40] -> [DMESG-WARN][41] ([i915#6540])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12004/fi-bdw-gvtdvm/igt@i915_module_load@load.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bdw-gvtdvm/igt@i915_module_load@load.html

  * igt@runner@aborted:
    - fi-skl-6700k2:      NOTRUN -> [FAIL][42] ([i915#4312] / [i915#6599])
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-skl-6700k2/igt@runner@aborted.html
    - fi-cfl-8109u:       NOTRUN -> [FAIL][43] ([i915#4312] / [i915#6599])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-8109u/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][44] ([i915#4312] / [i915#6599])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-skl-6600u/igt@runner@aborted.html
    - fi-bdw-5557u:       NOTRUN -> [FAIL][45] ([i915#4312])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bdw-5557u/igt@runner@aborted.html
    - fi-glk-j4005:       NOTRUN -> [FAIL][46] ([i915#4312] / [i915#6599])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-glk-j4005/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][47] ([i915#4312] / [i915#6599])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-rkl-guc/igt@runner@aborted.html
    - bat-dg1-6:          NOTRUN -> [FAIL][48] ([i915#4312])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/bat-dg1-6/igt@runner@aborted.html
    - fi-cfl-8700k:       NOTRUN -> [FAIL][49] ([i915#4312] / [i915#6599])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-8700k/igt@runner@aborted.html
    - fi-rkl-11600:       NOTRUN -> [FAIL][50] ([i915#4312] / [i915#6599])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-rkl-11600/igt@runner@aborted.html
    - fi-bdw-gvtdvm:      NOTRUN -> [FAIL][51] ([i915#4312])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bdw-gvtdvm/igt@runner@aborted.html
    - fi-bsw-kefka:       NOTRUN -> [FAIL][52] ([i915#4312])
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-bsw-kefka/igt@runner@aborted.html
    - fi-adl-ddr5:        NOTRUN -> [FAIL][53] ([i915#4312] / [i915#6599])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-adl-ddr5/igt@runner@aborted.html
    - fi-cfl-guc:         NOTRUN -> [FAIL][54] ([i915#4312] / [i915#6599])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107522v1/fi-cfl-guc/igt@runner@aborted.html

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

  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#6540]: https://gitlab.freedesktop.org/drm/intel/issues/6540
  [i915#6599]: https://gitlab.freedesktop.org/drm/intel/issues/6599
  [i915#6627]: https://gitlab.freedesktop.org/drm/intel/issues/6627
  [i915#6637]: https://gitlab.freedesktop.org/drm/intel/issues/6637


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

  * Linux: CI_DRM_12004 -> Patchwork_107522v1

  CI-20190529: 20190529
  CI_DRM_12004: 9a91db2ace2b69713b8ebbbc37554b53f23fa883 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6632: a0ac4d449e551fd5c78b56f85cd534330ea60507 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_107522v1: 9a91db2ace2b69713b8ebbbc37554b53f23fa883 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

a151aeaff7ef drm/i915/display: Add cdclk checks to atomic check
e5515080f499 drm/i915/display: s/intel_cdclk_can_crawl/intel_cdclk_crawl
e1d06c9c9d73 drm/i915/squash: s/intel_cdclk_can_squash/intel_cdclk_squash
61504643ab4f drm/i915/display: Add CDCLK actions to intel_cdclk_state

== Logs ==

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

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Navare, Manasi]

On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> This is a prep patch for what the rest of the series does.
> 
> Add existing actions that change cdclk - squash, crawl, modeset to
> intel_cdclk_state so we have access to the cdclk values
> that are in transition.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index b535cf6a7d9e..43835688ee02 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -15,6 +15,14 @@ struct drm_i915_private;
>  struct intel_atomic_state;
>  struct intel_crtc_state;
>  
> +enum cdclk_actions {
> +	INTEL_CDCLK_MODESET = 0,
> +	INTEL_CDCLK_SQUASH,
> +	INTEL_CDCLK_CRAWL,
> +	INTEL_CDCLK_NOOP,
> +	MAX_CDCLK_ACTIONS
> +};
> +
>  struct intel_cdclk_config {
>  	unsigned int cdclk, vco, ref, bypass;
>  	u8 voltage_level;
> @@ -51,6 +59,11 @@ struct intel_cdclk_state {
>  
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	struct cdclk_step {
> +		enum cdclk_actions action;
> +		u32 cdclk;
> +	} steps[MAX_CDCLK_ACTIONS];

If this is what you need to access later in bxt_set_cdclk , you needto
add this to intel_cdclk_config which is then part of cdclk_state and
that is what will get programmed in atomic_check and it gets sent to
bxt_set_cdclk in atomic_commit_tail.

This is the way ypu can access it in bxt_set_cdclk, you cannot access
the new_cdclk_state there, you need to use cdclk_config that is already
getting passed to it

Manasi

>  };
>  
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 2/4] drm/i915/squash: s/intel_cdclk_can_squash/intel_cdclk_squash

[Navare, Manasi]

On Fri, Aug 19, 2022 at 05:58:20PM -0700, Anusha Srivatsa wrote:
> Apart from checking if squashing can be performed,
> accommodate accessing in-flight cdclk state for any changes
> that are needed during commit phase.
> 
> v2: Move squashing bits to switch case.(Anusha)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 62 ++++++++++++++--------
>  1 file changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 86a22c3766e5..f98fd48fe905 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1693,12 +1693,18 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  			  const struct intel_cdclk_config *cdclk_config,
>  			  enum pipe pipe)
>  {
> +	struct intel_cdclk_state *cdclk_state = to_intel_cdclk_state(dev_priv->cdclk.obj.state);
> +	struct intel_atomic_state *state = cdclk_state->base.state;
> +	struct intel_cdclk_state *new_cdclk_state = intel_atomic_get_new_cdclk_state(state);

This will not work, make cdclk steps as part of cdlck_config struct and
access that through the passed cdclk_config

This already gets passed from the calling function intel_set_cdclk()
which is getting called from intel_set_cdclk_post_plane_update() from
commit_tail()
This is the correct way of programming req HW registers from cdclk state



Manasi

> +	struct cdclk_step *cdclk_steps = new_cdclk_state->steps;
>  	int cdclk = cdclk_config->cdclk;
>  	int vco = cdclk_config->vco;
> +	u32 squash_ctl = 0;
>  	u32 val;
>  	u16 waveform;
>  	int clock;
>  	int ret;
> +	int i;
>  
>  	/* Inform power controller of upcoming frequency change. */
>  	if (DISPLAY_VER(dev_priv) >= 11)
> @@ -1742,21 +1748,27 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  
>  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
>  
> -	if (waveform)
> +	if (waveform && has_cdclk_squasher(dev_priv)) {
>  		clock = vco / 2;
> -	else
> +		for (i = 0; i < MAX_CDCLK_ACTIONS; i++) {
> +			switch (cdclk_steps[i].action) {
> +			case INTEL_CDCLK_SQUASH:
> +				waveform =  cdclk_squash_waveform(dev_priv, cdclk_steps[i].cdclk);
> +				squash_ctl = CDCLK_SQUASH_ENABLE |
> +					     CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
> +				intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
> +				break;
> +			case INTEL_CDCLK_NOOP:
> +			case INTEL_CDCLK_CRAWL:
> +			case INTEL_CDCLK_MODESET:
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	} else
>  		clock = cdclk;
>  
> -	if (has_cdclk_squasher(dev_priv)) {
> -		u32 squash_ctl = 0;
> -
> -		if (waveform)
> -			squash_ctl = CDCLK_SQUASH_ENABLE |
> -				CDCLK_SQUASH_WINDOW_SIZE(0xf) | waveform;
> -
> -		intel_de_write(dev_priv, CDCLK_SQUASH_CTL, squash_ctl);
> -	}
> -
>  	val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
>  		bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
>  		skl_cdclk_decimal(cdclk);
> @@ -1966,10 +1978,11 @@ static bool intel_cdclk_can_crawl(struct drm_i915_private *dev_priv,
>  		a->ref == b->ref;
>  }
>  
> -static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> -				   const struct intel_cdclk_config *a,
> -				   const struct intel_cdclk_config *b)
> +static bool intel_cdclk_squash(struct drm_i915_private *dev_priv,
> +			       const struct intel_cdclk_state *a,
> +			       struct intel_cdclk_state *b)
>  {
> +	struct cdclk_step *cdclk_transition = b->steps;
>  	/*
>  	 * FIXME should store a bit more state in intel_cdclk_config
>  	 * to differentiate squasher vs. cd2x divider properly. For
> @@ -1978,11 +1991,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>  	 */
>  	if (!has_cdclk_squasher(dev_priv))
>  		return false;
> +	
> +	cdclk_transition[0].action = INTEL_CDCLK_SQUASH;
> +	cdclk_transition[0].cdclk = b->actual.cdclk;
> +	cdclk_transition[1].action = INTEL_CDCLK_NOOP;
> +	cdclk_transition[1].cdclk = b->actual.cdclk;
>  
> -	return a->cdclk != b->cdclk &&
> -		a->vco != 0 &&
> -		a->vco == b->vco &&
> -		a->ref == b->ref;
> +	return a->actual.cdclk != b->actual.cdclk &&
> +		a->actual.vco != 0 &&
> +		a->actual.vco == b->actual.vco &&
> +		a->actual.ref == b->actual.ref;
>  }
>  
>  /**
> @@ -2758,9 +2776,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>  			pipe = INVALID_PIPE;
>  	}
>  
> -	if (intel_cdclk_can_squash(dev_priv,
> -				   &old_cdclk_state->actual,
> -				   &new_cdclk_state->actual)) {
> +	if (intel_cdclk_squash(dev_priv,
> +			       old_cdclk_state,
> +			       new_cdclk_state)) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Can change cdclk via squasher\n");
>  	} else if (intel_cdclk_can_crawl(dev_priv,
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Ville Syrjälä]

On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> This is a prep patch for what the rest of the series does.
> 
> Add existing actions that change cdclk - squash, crawl, modeset to
> intel_cdclk_state so we have access to the cdclk values
> that are in transition.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index b535cf6a7d9e..43835688ee02 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -15,6 +15,14 @@ struct drm_i915_private;
>  struct intel_atomic_state;
>  struct intel_crtc_state;
>  
> +enum cdclk_actions {
> +	INTEL_CDCLK_MODESET = 0,
> +	INTEL_CDCLK_SQUASH,
> +	INTEL_CDCLK_CRAWL,
> +	INTEL_CDCLK_NOOP,
> +	MAX_CDCLK_ACTIONS
> +};

This whole actions thing feels overly complicated to me.
I think we should only need something like this:

if (new.squash > old.squash) {
	mid.vco = old.vco;
	mid.squash = new.squash;
} else {
	mid.vco = new.vco;
	mid.squash = old.squash;
}
/*
 * bunch of asserts here to make sure
 * the mid state looks sane.
 */
set_cdclk(mid);
set_cdclk(new);

And perhaps the current set_cdclk needs to get chunked up
into smaller pieces so we don't do all the pre/post stuff
more than once needlessly.

> +
>  struct intel_cdclk_config {
>  	unsigned int cdclk, vco, ref, bypass;
>  	u8 voltage_level;
> @@ -51,6 +59,11 @@ struct intel_cdclk_state {
>  
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	struct cdclk_step {
> +		enum cdclk_actions action;
> +		u32 cdclk;
> +	} steps[MAX_CDCLK_ACTIONS];
>  };
>  
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Ville Syrjälä]

On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote:
> On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> > This is a prep patch for what the rest of the series does.
> > 
> > Add existing actions that change cdclk - squash, crawl, modeset to
> > intel_cdclk_state so we have access to the cdclk values
> > that are in transition.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index b535cf6a7d9e..43835688ee02 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > @@ -15,6 +15,14 @@ struct drm_i915_private;
> >  struct intel_atomic_state;
> >  struct intel_crtc_state;
> >  
> > +enum cdclk_actions {
> > +	INTEL_CDCLK_MODESET = 0,
> > +	INTEL_CDCLK_SQUASH,
> > +	INTEL_CDCLK_CRAWL,
> > +	INTEL_CDCLK_NOOP,
> > +	MAX_CDCLK_ACTIONS
> > +};
> 
> This whole actions thing feels overly complicated to me.
> I think we should only need something like this:
> 
> if (new.squash > old.squash) {
> 	mid.vco = old.vco;
> 	mid.squash = new.squash;
> } else {
> 	mid.vco = new.vco;
> 	mid.squash = old.squash;
> }
> /*
>  * bunch of asserts here to make sure
>  * the mid state looks sane.
>  */
> set_cdclk(mid);
> set_cdclk(new);
> 
> And perhaps the current set_cdclk needs to get chunked up
> into smaller pieces so we don't do all the pre/post stuff
> more than once needlessly.

One idea might be to pass just a pair of flags to set_cdclk()
whether to skip the pre/post steps.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Srivatsa, Anusha]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, September 14, 2022 2:43 PM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to
> intel_cdclk_state
> 
> On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote:
> > On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> > > This is a prep patch for what the rest of the series does.
> > >
> > > Add existing actions that change cdclk - squash, crawl, modeset to
> > > intel_cdclk_state so we have access to the cdclk values that are in
> > > transition.
> > >
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > index b535cf6a7d9e..43835688ee02 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > @@ -15,6 +15,14 @@ struct drm_i915_private;  struct
> > > intel_atomic_state;  struct intel_crtc_state;
> > >
> > > +enum cdclk_actions {
> > > +	INTEL_CDCLK_MODESET = 0,
> > > +	INTEL_CDCLK_SQUASH,
> > > +	INTEL_CDCLK_CRAWL,
> > > +	INTEL_CDCLK_NOOP,
> > > +	MAX_CDCLK_ACTIONS
> > > +};
> >
> > This whole actions thing feels overly complicated to me.
> > I think we should only need something like this:
> >
> > if (new.squash > old.squash) {
> > 	mid.vco = old.vco;
> > 	mid.squash = new.squash;
> > } else {
> > 	mid.vco = new.vco;
> > 	mid.squash = old.squash;
> > }
> > /*
> >  * bunch of asserts here to make sure
> >  * the mid state looks sane.
> >  */
> > set_cdclk(mid);
> > set_cdclk(new);
> >
> > And perhaps the current set_cdclk needs to get chunked up into smaller
> > pieces so we don't do all the pre/post stuff more than once
> > needlessly.
> 
> One idea might be to pass just a pair of flags to set_cdclk() whether to skip
> the pre/post steps.

This is all considering that the new struct cdclk_step is embedded in cdclk_config and not cdclk_state. I am not understanding why cdclk-state is not accessible from bxt_set_cdclk. 
What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. 

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Ville Syrjälä]

On Wed, Sep 14, 2022 at 09:58:59PM +0000, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, September 14, 2022 2:43 PM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to
> > intel_cdclk_state
> > 
> > On Thu, Sep 15, 2022 at 12:22:53AM +0300, Ville Syrjälä wrote:
> > > On Fri, Aug 19, 2022 at 05:58:19PM -0700, Anusha Srivatsa wrote:
> > > > This is a prep patch for what the rest of the series does.
> > > >
> > > > Add existing actions that change cdclk - squash, crawl, modeset to
> > > > intel_cdclk_state so we have access to the cdclk values that are in
> > > > transition.
> > > >
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > > b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > > index b535cf6a7d9e..43835688ee02 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > > > @@ -15,6 +15,14 @@ struct drm_i915_private;  struct
> > > > intel_atomic_state;  struct intel_crtc_state;
> > > >
> > > > +enum cdclk_actions {
> > > > +	INTEL_CDCLK_MODESET = 0,
> > > > +	INTEL_CDCLK_SQUASH,
> > > > +	INTEL_CDCLK_CRAWL,
> > > > +	INTEL_CDCLK_NOOP,
> > > > +	MAX_CDCLK_ACTIONS
> > > > +};
> > >
> > > This whole actions thing feels overly complicated to me.
> > > I think we should only need something like this:
> > >
> > > if (new.squash > old.squash) {
> > > 	mid.vco = old.vco;
> > > 	mid.squash = new.squash;
> > > } else {
> > > 	mid.vco = new.vco;
> > > 	mid.squash = old.squash;
> > > }
> > > /*
> > >  * bunch of asserts here to make sure
> > >  * the mid state looks sane.
> > >  */
> > > set_cdclk(mid);
> > > set_cdclk(new);
> > >
> > > And perhaps the current set_cdclk needs to get chunked up into smaller
> > > pieces so we don't do all the pre/post stuff more than once
> > > needlessly.
> > 
> > One idea might be to pass just a pair of flags to set_cdclk() whether to skip
> > the pre/post steps.
> 
> This is all considering that the new struct cdclk_step is embedded in cdclk_config and not cdclk_state. I am not understanding why cdclk-state is not accessible from bxt_set_cdclk.

.set_cdclk() is lower level than that. It must be able to operate outside
atomic commits (eg. during display core init/uninit), and thus must be
able to do its work purely based on the passed in cdclk_config (which is
the lower level state structure, a pair of which are embedded inside the
atomic intel_cdclk_state).

> What if I add cdclk_state to the dev_priv? bxt_set_cdclk() anyway has dev_priv. 

We definitely don't want to hack around the core ideas
of how the atomic machinery works. That way leads
madness because no one will be able to understand how
anything works.

-- 
Ville Syrjälä
Intel

[Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Anusha Srivatsa]

This is a prep patch for what the rest of the series does.

Add existing actions that change cdclk - squash, crawl, modeset to
intel_cdclk_state so we have access to the cdclk values
that are in transition.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index b535cf6a7d9e..43835688ee02 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -15,6 +15,14 @@ struct drm_i915_private;
 struct intel_atomic_state;
 struct intel_crtc_state;
 
+enum cdclk_actions {
+	INTEL_CDCLK_MODESET = 0,
+	INTEL_CDCLK_SQUASH,
+	INTEL_CDCLK_CRAWL,
+	INTEL_CDCLK_NOOP,
+	MAX_CDCLK_ACTIONS
+};
+
 struct intel_cdclk_config {
 	unsigned int cdclk, vco, ref, bypass;
 	u8 voltage_level;
@@ -51,6 +59,11 @@ struct intel_cdclk_state {
 
 	/* bitmask of active pipes */
 	u8 active_pipes;
+
+	struct cdclk_step {
+		enum cdclk_actions action;
+		u32 cdclk;
+	} steps[MAX_CDCLK_ACTIONS];
 };
 
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
-- 
2.25.1

[Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Anusha Srivatsa]

This is a prep patch for what the rest of the series does.

Add existing actions that change cdclk - squash, crawl, modeset to
intel_cdclk_state so we have access to the cdclk values
that are in transition.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index df66f66fbad0..06d7f9f0b253 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -15,6 +15,14 @@ struct drm_i915_private;
 struct intel_atomic_state;
 struct intel_crtc_state;
 
+enum cdclk_actions {
+	INTEL_CDCLK_MODESET = 0,
+	INTEL_CDCLK_SQUASH,
+	INTEL_CDCLK_CRAWL,
+	INTEL_CDCLK_NOOP,
+	MAX_CDCLK_ACTIONS
+};
+
 struct intel_cdclk_config {
 	unsigned int cdclk, vco, ref, bypass;
 	u8 voltage_level;
@@ -49,6 +57,11 @@ struct intel_cdclk_state {
 
 	/* bitmask of active pipes */
 	u8 active_pipes;
+
+	struct cdclk_step {
+		enum cdclk_actions action;
+		u32 cdclk;
+	} steps[MAX_CDCLK_ACTIONS];
 };
 
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
-- 
2.25.1

[Intel-gfx] [PATCH 1/4] drm/i915/display: Add CDCLK actions to intel_cdclk_state

[Anusha Srivatsa]

This is a prep patch for what the rest of the series does.

Add existing actions that change cdclk - squash, crawl, modeset to
intel_cdclk_state so we have access to the cdclk values
that are in transition.

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index df66f66fbad0..06d7f9f0b253 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -15,6 +15,14 @@ struct drm_i915_private;
 struct intel_atomic_state;
 struct intel_crtc_state;
 
+enum cdclk_actions {
+	INTEL_CDCLK_MODESET = 0,
+	INTEL_CDCLK_SQUASH,
+	INTEL_CDCLK_CRAWL,
+	INTEL_CDCLK_NOOP,
+	MAX_CDCLK_ACTIONS
+};
+
 struct intel_cdclk_config {
 	unsigned int cdclk, vco, ref, bypass;
 	u8 voltage_level;
@@ -49,6 +57,11 @@ struct intel_cdclk_state {
 
 	/* bitmask of active pipes */
 	u8 active_pipes;
+
+	struct cdclk_step {
+		enum cdclk_actions action;
+		u32 cdclk;
+	} steps[MAX_CDCLK_ACTIONS];
 };
 
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
-- 
2.25.1