[PATCH 0/2] drm/i915/display: Add comparison for pipe config for MTL+ platforms

[PATCH 0/2] drm/i915/display: Add comparison for pipe config for MTL+ platforms

[Mika Kahola]

Currently, we may bump into pll mismatch errors during the
state verification stage. This happens when we try to use
fastset instead of full modeset. Hence, we would need to add
a check for pipe configuration to ensure that the sw and the
hw configuration will match. In case of hw and sw mismatch,
we would need to disable fastset and use full modeset instead.

However, first we need to revert the patch that disables fastset
for C10.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Mika Kahola (2):
  drm/i915/display: Revert "drm/i915/display: Skip C10 state
    verification in case of fastset"
  drm/i915/display: Add compare config for MTL+ platforms

 drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 77 ++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_cx0_phy.h  |  2 +
 drivers/gpu/drm/i915/display/intel_display.c  | 39 ++++++++++
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  1 +
 4 files changed, 116 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/2] drm/i915/display: Revert "drm/i915/display: Skip C10 state verification in case of fastset"

[Mika Kahola]

This reverts commit a1d91c6e989d0e66b89aa911f2cd459d7bdebbe5.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 1b1ebafa49e8..c9e5bb6ecfd7 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3243,9 +3243,6 @@ static void intel_c10pll_state_verify(const struct intel_crtc_state *state,
 	const struct intel_c10pll_state *mpllb_sw_state = &state->dpll_hw_state.cx0pll.c10;
 	int i;
 
-	if (intel_crtc_needs_fastset(state))
-		return;
-
 	for (i = 0; i < ARRAY_SIZE(mpllb_sw_state->pll); i++) {
 		u8 expected = mpllb_sw_state->pll[i];
 
-- 
2.34.1

[PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms

[Mika Kahola]

Currently, we may bump into pll mismatch errors during the
state verification stage. This happens when we try to use
fastset instead of full modeset. Hence, we would need to add
a check for pipe configuration to ensure that the sw and the
hw configuration will match. In case of hw and sw mismatch,
we would need to disable fastset and use full modeset instead.

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 74 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_cx0_phy.h  |  2 +
 drivers/gpu/drm/i915/display/intel_display.c  | 39 ++++++++++
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  1 +
 4 files changed, 116 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index c9e5bb6ecfd7..f549753ab1cf 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -2038,6 +2038,7 @@ static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
 		if (crtc_state->port_clock == tables[i]->clock) {
 			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
 			intel_c10pll_update_pll(crtc_state, encoder);
+			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
 
 			return 0;
 		}
@@ -2277,6 +2278,7 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
 	for (i = 0; tables[i]; i++) {
 		if (crtc_state->port_clock == tables[i]->clock) {
 			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
+			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
 			return 0;
 		}
 	}
@@ -3272,6 +3274,78 @@ void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
 		intel_c20pll_readout_hw_state(encoder, &pll_state->c20);
 }
 
+static bool mtl_compare_hw_state_c10(const struct intel_c10pll_state *a,
+				     const struct intel_c10pll_state *b)
+{
+	return a->clock == b->clock ||
+	       a->tx == b->tx ||
+	       a->cmn == b->cmn ||
+	       a->pll[0] == b->pll[0] ||
+	       a->pll[1] == b->pll[1] ||
+	       a->pll[2] == b->pll[2] ||
+	       a->pll[3] == b->pll[3] ||
+	       a->pll[4] == b->pll[4] ||
+	       a->pll[5] == b->pll[5] ||
+	       a->pll[6] == b->pll[6] ||
+	       a->pll[7] == b->pll[7] ||
+	       a->pll[8] == b->pll[8] ||
+	       a->pll[9] == b->pll[9] ||
+	       a->pll[10] == b->pll[10] ||
+	       a->pll[11] == b->pll[11] ||
+	       a->pll[12] == b->pll[12] ||
+	       a->pll[13] == b->pll[13] ||
+	       a->pll[14] == b->pll[14] ||
+	       a->pll[15] == b->pll[15] ||
+	       a->pll[16] == b->pll[16] ||
+	       a->pll[17] == b->pll[17] ||
+	       a->pll[18] == b->pll[18] ||
+	       a->pll[19] == b->pll[19];
+}
+
+static bool mtl_compare_hw_state_c20(const struct intel_c20pll_state *a,
+				     const struct intel_c20pll_state *b)
+{
+	int i;
+
+	if (a->clock != b->clock)
+		return false;
+
+	for (i = 0; i < 3; i++) {
+		if (a->tx[i] != b->tx[i])
+			return false;
+	}
+
+	for (i = 4; i < 4; i++) {
+		if (a->cmn[i] != b->cmn[i])
+			return false;
+	}
+
+	if (a->tx[0] & C20_PHY_USE_MPLLB) {
+		for (i = 0; i < ARRAY_SIZE(a->mpllb); i++) {
+			if (a->mpllb[i] != b->mpllb[i])
+				return false;
+		}
+	} else {
+		for (i = 0; i < ARRAY_SIZE(a->mplla); i++) {
+			if (a->mplla[i] != b->mplla[i])
+				return false;
+		}
+	}
+
+	return true;
+}
+
+bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
+				   const struct intel_cx0pll_state *b)
+{
+	if (a->use_c10 && b->use_c10)
+		return mtl_compare_hw_state_c10(&a->c10,
+						&b->c10);
+	else
+		return mtl_compare_hw_state_c20(&a->c20,
+						&b->c20);
+}
+
 int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
 				 const struct intel_cx0pll_state *pll_state)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
index 3e03af3e006c..180821df1834 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
@@ -39,6 +39,8 @@ void intel_c10pll_dump_hw_state(struct drm_i915_private *dev_priv,
 				const struct intel_c10pll_state *hw_state);
 void intel_cx0pll_state_verify(struct intel_atomic_state *state,
 			       struct intel_crtc *crtc);
+bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
+				   const struct intel_cx0pll_state *b);
 void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
 				const struct intel_c20pll_state *hw_state);
 void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index cce1420fb541..17b43b2ae0e9 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -66,6 +66,7 @@
 #include "intel_crtc.h"
 #include "intel_crtc_state_dump.h"
 #include "intel_cursor_regs.h"
+#include "intel_cx0_phy.h"
 #include "intel_ddi.h"
 #include "intel_de.h"
 #include "intel_display_driver.h"
@@ -5002,6 +5003,30 @@ pipe_config_pll_mismatch(struct drm_printer *p, bool fastset,
 	intel_dpll_dump_hw_state(i915, p, b);
 }
 
+static void
+pipe_config_cx0pll_mismatch(struct drm_printer *p, bool fastset,
+			    const struct intel_crtc *crtc,
+			    const char *name,
+			    const struct intel_cx0pll_state *a,
+			    const struct intel_cx0pll_state *b)
+{
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+
+	pipe_config_mismatch(p, fastset, crtc, name, " "); /* stupid -Werror=format-zero-length */
+
+	if (a->use_c10) {
+		drm_printf(p, "expected:\n");
+		intel_c10pll_dump_hw_state(i915, &a->c10);
+		drm_printf(p, "found:\n");
+		intel_c10pll_dump_hw_state(i915, &b->c10);
+	} else {
+		drm_printf(p, "expected:\n");
+		intel_c20pll_dump_hw_state(i915, &a->c20);
+		drm_printf(p, "found:\n");
+		intel_c20pll_dump_hw_state(i915, &b->c20);
+	}
+}
+
 bool
 intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 			  const struct intel_crtc_state *pipe_config,
@@ -5105,6 +5130,16 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
+	if (!intel_cx0pll_compare_hw_state(&current_config->name, \
+					   &pipe_config->name)) { \
+		pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
+					    &current_config->name, \
+					    &pipe_config->name); \
+		ret = false; \
+	} \
+} while (0)
+
 #define PIPE_CONF_CHECK_TIMINGS(name) do {     \
 	PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
 	PIPE_CONF_CHECK_I(name.crtc_htotal); \
@@ -5337,6 +5372,10 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv))
 		PIPE_CONF_CHECK_PLL(dpll_hw_state);
 
+	/* FIXME convert MTL+ platforms over to dpll_mgr */
+	if (DISPLAY_VER(dev_priv) >= 14)
+		PIPE_CONF_CHECK_PLL_CX0(dpll_hw_state.cx0pll);
+
 	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
 	PIPE_CONF_CHECK_X(dsi_pll.div);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index f09e513ce05b..36baed75b89a 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -264,6 +264,7 @@ struct intel_cx0pll_state {
 		struct intel_c20pll_state c20;
 	};
 	bool ssc_enabled;
+	bool use_c10;
 };
 
 struct intel_dpll_hw_state {
-- 
2.34.1

Re: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms

[Jani Nikula]

On Wed, 22 May 2024, Mika Kahola <mika.kahola@intel.com> wrote:
> Currently, we may bump into pll mismatch errors during the
> state verification stage. This happens when we try to use
> fastset instead of full modeset. Hence, we would need to add
> a check for pipe configuration to ensure that the sw and the
> hw configuration will match. In case of hw and sw mismatch,
> we would need to disable fastset and use full modeset instead.
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 74 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_cx0_phy.h  |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c  | 39 ++++++++++
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  1 +
>  4 files changed, 116 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index c9e5bb6ecfd7..f549753ab1cf 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -2038,6 +2038,7 @@ static int intel_c10pll_calc_state(struct intel_crtc_state *crtc_state,
>  		if (crtc_state->port_clock == tables[i]->clock) {
>  			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
>  			intel_c10pll_update_pll(crtc_state, encoder);
> +			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;

The readout doesn't set use_c10 anywhere, does it?

>  
>  			return 0;
>  		}
> @@ -2277,6 +2278,7 @@ static int intel_c20pll_calc_state(struct intel_crtc_state *crtc_state,
>  	for (i = 0; tables[i]; i++) {
>  		if (crtc_state->port_clock == tables[i]->clock) {
>  			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> +			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
>  			return 0;
>  		}
>  	}
> @@ -3272,6 +3274,78 @@ void intel_cx0pll_readout_hw_state(struct intel_encoder *encoder,
>  		intel_c20pll_readout_hw_state(encoder, &pll_state->c20);
>  }
>  
> +static bool mtl_compare_hw_state_c10(const struct intel_c10pll_state *a,
> +				     const struct intel_c10pll_state *b)
> +{
> +	return a->clock == b->clock ||
> +	       a->tx == b->tx ||
> +	       a->cmn == b->cmn ||
> +	       a->pll[0] == b->pll[0] ||
> +	       a->pll[1] == b->pll[1] ||
> +	       a->pll[2] == b->pll[2] ||
> +	       a->pll[3] == b->pll[3] ||
> +	       a->pll[4] == b->pll[4] ||
> +	       a->pll[5] == b->pll[5] ||
> +	       a->pll[6] == b->pll[6] ||
> +	       a->pll[7] == b->pll[7] ||
> +	       a->pll[8] == b->pll[8] ||
> +	       a->pll[9] == b->pll[9] ||
> +	       a->pll[10] == b->pll[10] ||
> +	       a->pll[11] == b->pll[11] ||
> +	       a->pll[12] == b->pll[12] ||
> +	       a->pll[13] == b->pll[13] ||
> +	       a->pll[14] == b->pll[14] ||
> +	       a->pll[15] == b->pll[15] ||
> +	       a->pll[16] == b->pll[16] ||
> +	       a->pll[17] == b->pll[17] ||
> +	       a->pll[18] == b->pll[18] ||
> +	       a->pll[19] == b->pll[19];

How about memcmp(a->pll, b->pll, sizeof(a->pll)) == 0 instead?


> +}
> +
> +static bool mtl_compare_hw_state_c20(const struct intel_c20pll_state *a,
> +				     const struct intel_c20pll_state *b)
> +{
> +	int i;
> +
> +	if (a->clock != b->clock)
> +		return false;
> +
> +	for (i = 0; i < 3; i++) {
> +		if (a->tx[i] != b->tx[i])
> +			return false;
> +	}

memcmp with sizeof, so we don't have to hardcode the sizes.

> +
> +	for (i = 4; i < 4; i++) {

Typo, this does nothing... but memcmp.

> +		if (a->cmn[i] != b->cmn[i])
> +			return false;
> +	}
> +
> +	if (a->tx[0] & C20_PHY_USE_MPLLB) {
> +		for (i = 0; i < ARRAY_SIZE(a->mpllb); i++) {
> +			if (a->mpllb[i] != b->mpllb[i])
> +				return false;
> +		}
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(a->mplla); i++) {
> +			if (a->mplla[i] != b->mplla[i])
> +				return false;
> +		}
> +	}

And memcmp.

> +
> +	return true;
> +}
> +
> +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> +				   const struct intel_cx0pll_state *b)
> +{

Maybe this for starters?

if (a->use_c10 != b->use_c10)
	return false;

> +	if (a->use_c10 && b->use_c10)
> +		return mtl_compare_hw_state_c10(&a->c10,
> +						&b->c10);
> +	else
> +		return mtl_compare_hw_state_c20(&a->c20,
> +						&b->c20);
> +}
> +
>  int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
>  				 const struct intel_cx0pll_state *pll_state)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> index 3e03af3e006c..180821df1834 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> @@ -39,6 +39,8 @@ void intel_c10pll_dump_hw_state(struct drm_i915_private *dev_priv,
>  				const struct intel_c10pll_state *hw_state);
>  void intel_cx0pll_state_verify(struct intel_atomic_state *state,
>  			       struct intel_crtc *crtc);
> +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> +				   const struct intel_cx0pll_state *b);
>  void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
>  				const struct intel_c20pll_state *hw_state);
>  void intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index cce1420fb541..17b43b2ae0e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -66,6 +66,7 @@
>  #include "intel_crtc.h"
>  #include "intel_crtc_state_dump.h"
>  #include "intel_cursor_regs.h"
> +#include "intel_cx0_phy.h"
>  #include "intel_ddi.h"
>  #include "intel_de.h"
>  #include "intel_display_driver.h"
> @@ -5002,6 +5003,30 @@ pipe_config_pll_mismatch(struct drm_printer *p, bool fastset,
>  	intel_dpll_dump_hw_state(i915, p, b);
>  }
>  
> +static void
> +pipe_config_cx0pll_mismatch(struct drm_printer *p, bool fastset,
> +			    const struct intel_crtc *crtc,
> +			    const char *name,
> +			    const struct intel_cx0pll_state *a,
> +			    const struct intel_cx0pll_state *b)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +
> +	pipe_config_mismatch(p, fastset, crtc, name, " "); /* stupid -Werror=format-zero-length */

Instead of working around something and adding comments like that, maybe
actually use it for something useful?

Something like, idk, "%s", a->c10 ? "c10" : "c20"

> +
> +	if (a->use_c10) {
> +		drm_printf(p, "expected:\n");
> +		intel_c10pll_dump_hw_state(i915, &a->c10);
> +		drm_printf(p, "found:\n");
> +		intel_c10pll_dump_hw_state(i915, &b->c10);
> +	} else {
> +		drm_printf(p, "expected:\n");
> +		intel_c20pll_dump_hw_state(i915, &a->c20);
> +		drm_printf(p, "found:\n");
> +		intel_c20pll_dump_hw_state(i915, &b->c20);
> +	}
> +		drm_printf(p, "found:\n");
> +		intel_c10pll_dump_hw_state(i915, &b->c10);
> +	} else {
> +		drm_printf(p, "expected:\n");
> +		intel_c20pll_dump_hw_state(i915, &a->c20);
> +		drm_printf(p, "found:\n");
> +		intel_c20pll_dump_hw_state(i915, &b->c20);
> +	}

I think I'd add a intel_cx0pll_dump_hw_state() to avoid looking into the
details like this at high level code. This becomes cleaner too.

> +}
> +
>  bool
>  intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			  const struct intel_crtc_state *pipe_config,
> @@ -5105,6 +5130,16 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	} \
>  } while (0)
>  
> +#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
> +	if (!intel_cx0pll_compare_hw_state(&current_config->name, \
> +					   &pipe_config->name)) { \
> +		pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
> +					    &current_config->name, \
> +					    &pipe_config->name); \
> +		ret = false; \
> +	} \
> +} while (0)
> +
>  #define PIPE_CONF_CHECK_TIMINGS(name) do {     \
>  	PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
>  	PIPE_CONF_CHECK_I(name.crtc_htotal); \
> @@ -5337,6 +5372,10 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv))
>  		PIPE_CONF_CHECK_PLL(dpll_hw_state);
>  
> +	/* FIXME convert MTL+ platforms over to dpll_mgr */
> +	if (DISPLAY_VER(dev_priv) >= 14)
> +		PIPE_CONF_CHECK_PLL_CX0(dpll_hw_state.cx0pll);
> +
>  	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
>  	PIPE_CONF_CHECK_X(dsi_pll.div);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index f09e513ce05b..36baed75b89a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -264,6 +264,7 @@ struct intel_cx0pll_state {
>  		struct intel_c20pll_state c20;
>  	};
>  	bool ssc_enabled;
> +	bool use_c10;
>  };
>  
>  struct intel_dpll_hw_state {

-- 
Jani Nikula, Intel

RE: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms

[Kahola, Mika]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Wednesday, May 22, 2024 12:39 PM
> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Kahola, Mika <mika.kahola@intel.com>
> Subject: Re: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms
> 
> On Wed, 22 May 2024, Mika Kahola <mika.kahola@intel.com> wrote:
> > Currently, we may bump into pll mismatch errors during the state
> > verification stage. This happens when we try to use fastset instead of
> > full modeset. Hence, we would need to add a check for pipe
> > configuration to ensure that the sw and the hw configuration will
> > match. In case of hw and sw mismatch, we would need to disable fastset
> > and use full modeset instead.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 74
> > +++++++++++++++++++  drivers/gpu/drm/i915/display/intel_cx0_phy.h  |
> > 2 +  drivers/gpu/drm/i915/display/intel_display.c  | 39 ++++++++++
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  1 +
> >  4 files changed, 116 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index c9e5bb6ecfd7..f549753ab1cf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2038,6 +2038,7 @@ static int intel_c10pll_calc_state(struct intel_crtc_state
> *crtc_state,
> >  		if (crtc_state->port_clock == tables[i]->clock) {
> >  			crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
> >  			intel_c10pll_update_pll(crtc_state, encoder);
> > +			crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
> 
> The readout doesn't set use_c10 anywhere, does it?
No, it is just used to select which C10 or C20 sw and hw configs are compared.
 
> 
> >
> >  			return 0;
> >  		}
> > @@ -2277,6 +2278,7 @@ static int intel_c20pll_calc_state(struct intel_crtc_state
> *crtc_state,
> >  	for (i = 0; tables[i]; i++) {
> >  		if (crtc_state->port_clock == tables[i]->clock) {
> >  			crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> > +			crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> >  			return 0;
> >  		}
> >  	}
> > @@ -3272,6 +3274,78 @@ void intel_cx0pll_readout_hw_state(struct
> intel_encoder *encoder,
> >  		intel_c20pll_readout_hw_state(encoder, &pll_state->c20);  }
> >
> > +static bool mtl_compare_hw_state_c10(const struct intel_c10pll_state *a,
> > +				     const struct intel_c10pll_state *b) {
> > +	return a->clock == b->clock ||
> > +	       a->tx == b->tx ||
> > +	       a->cmn == b->cmn ||
> > +	       a->pll[0] == b->pll[0] ||
> > +	       a->pll[1] == b->pll[1] ||
> > +	       a->pll[2] == b->pll[2] ||
> > +	       a->pll[3] == b->pll[3] ||
> > +	       a->pll[4] == b->pll[4] ||
> > +	       a->pll[5] == b->pll[5] ||
> > +	       a->pll[6] == b->pll[6] ||
> > +	       a->pll[7] == b->pll[7] ||
> > +	       a->pll[8] == b->pll[8] ||
> > +	       a->pll[9] == b->pll[9] ||
> > +	       a->pll[10] == b->pll[10] ||
> > +	       a->pll[11] == b->pll[11] ||
> > +	       a->pll[12] == b->pll[12] ||
> > +	       a->pll[13] == b->pll[13] ||
> > +	       a->pll[14] == b->pll[14] ||
> > +	       a->pll[15] == b->pll[15] ||
> > +	       a->pll[16] == b->pll[16] ||
> > +	       a->pll[17] == b->pll[17] ||
> > +	       a->pll[18] == b->pll[18] ||
> > +	       a->pll[19] == b->pll[19];
> 
> How about memcmp(a->pll, b->pll, sizeof(a->pll)) == 0 instead?
Yes, this is possible. I tried to mimic the comparison check done for some other platforms.

> 
> 
> > +}
> > +
> > +static bool mtl_compare_hw_state_c20(const struct intel_c20pll_state *a,
> > +				     const struct intel_c20pll_state *b) {
> > +	int i;
> > +
> > +	if (a->clock != b->clock)
> > +		return false;
> > +
> > +	for (i = 0; i < 3; i++) {
> > +		if (a->tx[i] != b->tx[i])
> > +			return false;
> > +	}
> 
> memcmp with sizeof, so we don't have to hardcode the sizes.
Yes.

> 
> > +
> > +	for (i = 4; i < 4; i++) {
> 
> Typo, this does nothing... but memcmp.
Yep.

> 
> > +		if (a->cmn[i] != b->cmn[i])
> > +			return false;
> > +	}
> > +
> > +	if (a->tx[0] & C20_PHY_USE_MPLLB) {
> > +		for (i = 0; i < ARRAY_SIZE(a->mpllb); i++) {
> > +			if (a->mpllb[i] != b->mpllb[i])
> > +				return false;
> > +		}
> > +	} else {
> > +		for (i = 0; i < ARRAY_SIZE(a->mplla); i++) {
> > +			if (a->mplla[i] != b->mplla[i])
> > +				return false;
> > +		}
> > +	}
> 
> And memcmp.
That one too.

> 
> > +
> > +	return true;
> > +}
> > +
> > +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> > +				   const struct intel_cx0pll_state *b) {
> 
> Maybe this for starters?
> 
> if (a->use_c10 != b->use_c10)
> 	return false;

Ok, let's do the check first before doing anything else.
> 
> > +	if (a->use_c10 && b->use_c10)
> > +		return mtl_compare_hw_state_c10(&a->c10,
> > +						&b->c10);
> > +	else
> > +		return mtl_compare_hw_state_c20(&a->c20,
> > +						&b->c20);
> > +}
> > +
> >  int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
> >  				 const struct intel_cx0pll_state *pll_state)  { diff --git
> > a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > index 3e03af3e006c..180821df1834 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > @@ -39,6 +39,8 @@ void intel_c10pll_dump_hw_state(struct drm_i915_private
> *dev_priv,
> >  				const struct intel_c10pll_state *hw_state);  void
> > intel_cx0pll_state_verify(struct intel_atomic_state *state,
> >  			       struct intel_crtc *crtc);
> > +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> > +				   const struct intel_cx0pll_state *b);
> >  void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
> >  				const struct intel_c20pll_state *hw_state);  void
> > intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, diff
> > --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index cce1420fb541..17b43b2ae0e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -66,6 +66,7 @@
> >  #include "intel_crtc.h"
> >  #include "intel_crtc_state_dump.h"
> >  #include "intel_cursor_regs.h"
> > +#include "intel_cx0_phy.h"
> >  #include "intel_ddi.h"
> >  #include "intel_de.h"
> >  #include "intel_display_driver.h"
> > @@ -5002,6 +5003,30 @@ pipe_config_pll_mismatch(struct drm_printer *p, bool
> fastset,
> >  	intel_dpll_dump_hw_state(i915, p, b);  }
> >
> > +static void
> > +pipe_config_cx0pll_mismatch(struct drm_printer *p, bool fastset,
> > +			    const struct intel_crtc *crtc,
> > +			    const char *name,
> > +			    const struct intel_cx0pll_state *a,
> > +			    const struct intel_cx0pll_state *b) {
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +
> > +	pipe_config_mismatch(p, fastset, crtc, name, " "); /* stupid
> > +-Werror=format-zero-length */
> 
> Instead of working around something and adding comments like that, maybe actually
> use it for something useful?
> 
> Something like, idk, "%s", a->c10 ? "c10" : "c20"

Ah, stupid copy and paste. I should have fixed this but missed.

> 
> > +
> > +	if (a->use_c10) {
> > +		drm_printf(p, "expected:\n");
> > +		intel_c10pll_dump_hw_state(i915, &a->c10);
> > +		drm_printf(p, "found:\n");
> > +		intel_c10pll_dump_hw_state(i915, &b->c10);
> > +	} else {
> > +		drm_printf(p, "expected:\n");
> > +		intel_c20pll_dump_hw_state(i915, &a->c20);
> > +		drm_printf(p, "found:\n");
> > +		intel_c20pll_dump_hw_state(i915, &b->c20);
> > +	}
> > +		drm_printf(p, "found:\n");
> > +		intel_c10pll_dump_hw_state(i915, &b->c10);
> > +	} else {
> > +		drm_printf(p, "expected:\n");
> > +		intel_c20pll_dump_hw_state(i915, &a->c20);
> > +		drm_printf(p, "found:\n");
> > +		intel_c20pll_dump_hw_state(i915, &b->c20);
> > +	}
> 
> I think I'd add a intel_cx0pll_dump_hw_state() to avoid looking into the details like
> this at high level code. This becomes cleaner too.

True. I rephrase this part. Anyway, there was a BAT issue that I need to solve that I wasn't able to trigger while I was testing this patch.

Thanks for the review and comments!

-Mika-
> 
> > +}
> > +
> >  bool
> >  intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  			  const struct intel_crtc_state *pipe_config, @@ -5105,6
> +5130,16
> > @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  	} \
> >  } while (0)
> >
> > +#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
> > +	if (!intel_cx0pll_compare_hw_state(&current_config->name, \
> > +					   &pipe_config->name)) { \
> > +		pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
> > +					    &current_config->name, \
> > +					    &pipe_config->name); \
> > +		ret = false; \
> > +	} \
> > +} while (0)
> > +
> >  #define PIPE_CONF_CHECK_TIMINGS(name) do {     \
> >  	PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
> >  	PIPE_CONF_CHECK_I(name.crtc_htotal); \ @@ -5337,6 +5372,10 @@
> > intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  	if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv))
> >  		PIPE_CONF_CHECK_PLL(dpll_hw_state);
> >
> > +	/* FIXME convert MTL+ platforms over to dpll_mgr */
> > +	if (DISPLAY_VER(dev_priv) >= 14)
> > +		PIPE_CONF_CHECK_PLL_CX0(dpll_hw_state.cx0pll);
> > +
> >  	PIPE_CONF_CHECK_X(dsi_pll.ctrl);
> >  	PIPE_CONF_CHECK_X(dsi_pll.div);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > index f09e513ce05b..36baed75b89a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > @@ -264,6 +264,7 @@ struct intel_cx0pll_state {
> >  		struct intel_c20pll_state c20;
> >  	};
> >  	bool ssc_enabled;
> > +	bool use_c10;
> >  };
> >
> >  struct intel_dpll_hw_state {
> 
> --
> Jani Nikula, Intel

✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Add comparison for pipe config for MTL+ platforms

[Patchwork]

== Series Details ==

Series: drm/i915/display: Add comparison for pipe config for MTL+ platforms
URL   : https://patchwork.freedesktop.org/series/133899/
State : warning

== Summary ==

Error: dim checkpatch failed
dcc6ac972817 drm/i915/display: Revert "drm/i915/display: Skip C10 state verification in case of fastset"
10d56e0532b7 drm/i915/display: Add compare config for MTL+ platforms
-:174: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name' - possible side-effects?
#174: FILE: drivers/gpu/drm/i915/display/intel_display.c:5133:
+#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
+	if (!intel_cx0pll_compare_hw_state(&current_config->name, \
+					   &pipe_config->name)) { \
+		pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
+					    &current_config->name, \
+					    &pipe_config->name); \
+		ret = false; \
+	} \
+} while (0)

-:174: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'name' may be better as '(name)' to avoid precedence issues
#174: FILE: drivers/gpu/drm/i915/display/intel_display.c:5133:
+#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
+	if (!intel_cx0pll_compare_hw_state(&current_config->name, \
+					   &pipe_config->name)) { \
+		pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
+					    &current_config->name, \
+					    &pipe_config->name); \
+		ret = false; \
+	} \
+} while (0)

total: 0 errors, 0 warnings, 2 checks, 170 lines checked

✗ Fi.CI.SPARSE: warning for drm/i915/display: Add comparison for pipe config for MTL+ platforms

[Patchwork]

== Series Details ==

Series: drm/i915/display: Add comparison for pipe config for MTL+ platforms
URL   : https://patchwork.freedesktop.org/series/133899/
State : warning

== Summary ==

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

✗ Fi.CI.BAT: failure for drm/i915/display: Add comparison for pipe config for MTL+ platforms

[Patchwork]

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

== Series Details ==

Series: drm/i915/display: Add comparison for pipe config for MTL+ platforms
URL   : https://patchwork.freedesktop.org/series/133899/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14798 -> Patchwork_133899v1
====================================================

Summary
-------

  **FAILURE**

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

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

Participating hosts (44 -> 40)
------------------------------

  Additional (1): bat-atsm-1 
  Missing    (5): fi-bsw-n3050 fi-snb-2520m fi-glk-j4005 fi-elk-e7500 bat-dg2-11 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-arls-2:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14798/bat-arls-2/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-arls-2/igt@i915_module_load@load.html
    - bat-mtlp-8:         [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14798/bat-mtlp-8/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-mtlp-8/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@kms_force_connector_basic@force-edid:
    - {bat-mtlp-9}:       [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14798/bat-mtlp-9/igt@kms_force_connector_basic@force-edid.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-mtlp-9/igt@kms_force_connector_basic@force-edid.html

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

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

### IGT changes ###

#### Issues hit ####

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

  * igt@gem_tiled_pread_basic:
    - bat-atsm-1:         NOTRUN -> [SKIP][8] ([i915#4079]) +1 other test skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-atsm-1/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-atsm-1:         NOTRUN -> [SKIP][9] ([i915#6621])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-atsm-1/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-adlp-11:        [PASS][10] -> [INCOMPLETE][11] ([i915#9413])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14798/bat-adlp-11/igt@i915_selftest@live@gt_lrc.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-adlp-11/igt@i915_selftest@live@gt_lrc.html

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

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

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

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

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

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-atsm-1:         NOTRUN -> [SKIP][17] ([i915#6094])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-atsm-1/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-atsm-1:         NOTRUN -> [SKIP][18] ([i915#4077]) +4 other tests skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-atsm-1/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-atsm-1:         NOTRUN -> [SKIP][19] +2 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-atsm-1/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - bat-adlp-9:         [INCOMPLETE][20] ([i915#9413]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14798/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133899v1/bat-adlp-9/igt@i915_selftest@live@gt_lrc.html

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

  [i915#1836]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1836
  [i915#1982]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1982
  [i915#4077]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4083
  [i915#6077]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6077
  [i915#6078]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6078
  [i915#6093]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6093
  [i915#6094]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6094
  [i915#6621]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6621
  [i915#7357]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7357
  [i915#9413]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9413


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

  * Linux: CI_DRM_14798 -> Patchwork_133899v1

  CI-20190529: 20190529
  CI_DRM_14798: b134db8544f8d5b8a960b368afe12820c3cbe8cd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7866: 2b7701838c3ebaa3c717b6521cafafe3b9ae4a4f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_133899v1: b134db8544f8d5b8a960b368afe12820c3cbe8cd @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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