| Message ID | 20231106083322.113442-1-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/mtl: C20 state verification | expand |
Quoting Mika Kahola (2023-11-06 05:33:22-03:00) >Add state verification for C20 as we have one >for C10. > >v2: use register values as u32 instead of u8 > >Signed-off-by: Mika Kahola <mika.kahola@intel.com> The patch looks good to me. I had some minor suggestions further down. With those applied, Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> >--- > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 107 ++++++++++++++---- > drivers/gpu/drm/i915/display/intel_cx0_phy.h | 2 +- > .../drm/i915/display/intel_modeset_verify.c | 2 +- > 3 files changed, 84 insertions(+), 27 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c >index b2ad4c6172f6..87329bd2272a 100644 >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c >@@ -3017,35 +3017,15 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder, > return ICL_PORT_DPLL_DEFAULT; > } > >-void intel_c10pll_state_verify(struct intel_atomic_state *state, >- struct intel_crtc *crtc) >+static void intel_c10pll_state_verify(const struct intel_crtc_state *state, >+ struct intel_crtc *crtc, >+ struct intel_encoder *encoder) > { >- struct drm_i915_private *i915 = to_i915(state->base.dev); >- const struct intel_crtc_state *new_crtc_state = >- intel_atomic_get_new_crtc_state(state, crtc); >+ struct drm_i915_private *i915 = to_i915(crtc->base.dev); > struct intel_c10pll_state mpllb_hw_state = {}; >- const struct intel_c10pll_state *mpllb_sw_state = &new_crtc_state->cx0pll_state.c10; >- struct intel_encoder *encoder; >- enum phy phy; >+ const struct intel_c10pll_state *mpllb_sw_state = &state->cx0pll_state.c10; > int i; > >- if (DISPLAY_VER(i915) < 14) >- return; >- >- if (!new_crtc_state->hw.active) >- return; >- >- /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ >- if (!intel_crtc_needs_modeset(new_crtc_state) && >- !intel_crtc_needs_fastset(new_crtc_state)) >- return; >- >- encoder = intel_get_crtc_new_encoder(state, new_crtc_state); >- phy = intel_port_to_phy(i915, encoder->port); >- >- if (!intel_is_c10phy(i915, phy)) >- return; >- > intel_c10pll_readout_hw_state(encoder, &mpllb_hw_state); > > for (i = 0; i < ARRAY_SIZE(mpllb_sw_state->pll); i++) { >@@ -3091,3 +3071,80 @@ int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder, > > return intel_c20pll_calc_port_clock(encoder, &pll_state->c20); > } >+ >+static void intel_c20pll_state_verify(const struct intel_crtc_state *state, >+ struct intel_crtc *crtc, >+ struct intel_encoder *encoder) >+{ >+ struct drm_i915_private *i915 = to_i915(crtc->base.dev); >+ struct intel_c20pll_state mpll_hw_state = {}; >+ const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20; >+ bool use_mplla; >+ int i; >+ >+ intel_c20pll_readout_hw_state(encoder, &mpll_hw_state); Suggestion: Maybe have intel_cx0pll_state_verify() call intel_cx0pll_readout_hw_state() and have this (and also intel_c10pll_state_verify()) receiving the pointer to the hardware state? This would have the benefit of having the C10 and C20 specific functions for hardware readout called from a single place. >+ >+ use_mplla = intel_c20_use_mplla(mpll_hw_state.clock); >+ if (use_mplla) { >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) { >+ I915_STATE_WARN(i915, mpll_hw_state.mplla[i] != mpll_sw_state->mplla[i], >+ "[CRTC:%d:%s] mismatch in C20MPLLA: Register[%d] (expected 0x%04x, found 0x%04x)", ^^^^^^^^^^^^^^^^^^^^^^ Maybe simply use C20 MPLLA[%d] here? >+ crtc->base.base.id, crtc->base.name, i, >+ mpll_sw_state->mplla[i], mpll_hw_state.mplla[i]); >+ } >+ } else { >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mpllb); i++) { >+ I915_STATE_WARN(i915, mpll_hw_state.mpllb[i] != mpll_sw_state->mpllb[i], >+ "[CRTC:%d:%s] mismatch in C20MPLLB: Register[%d] (expected 0x%04x, found 0x%04x)", ^^^^^^^^^^^^^^^^^^^^^^ Same suggestion as above here. >+ crtc->base.base.id, crtc->base.name, i, >+ mpll_sw_state->mpllb[i], mpll_hw_state.mpllb[i]); >+ } >+ } >+ >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->tx); i++) { >+ I915_STATE_WARN(i915, mpll_hw_state.tx[i] != mpll_sw_state->tx[i], >+ "[CRTC:%d:%s] mismatch in C20MPLL%s: Register TX[%i] (expected 0x%04x, found 0x%04x)", ^^^^^^^^^^^^^^^^^^^^^^^^^^ Do tx[i] values really belong to MPLLA or MPLLB? Or is it rather something independent of both that really belongs the C20 phy? If the latter is true, then maybe just use C20 TX[%d] here? >+ crtc->base.base.id, crtc->base.name, >+ use_mplla ? "A" : "B", >+ i, >+ mpll_sw_state->tx[i], mpll_hw_state.tx[i]); >+ } >+ >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->cmn); i++) { >+ I915_STATE_WARN(i915, mpll_hw_state.cmn[i] != mpll_sw_state->cmn[i], >+ "[CRTC:%d:%s] mismatch in C20MPLL%s: Register CMN[%i] (expected 0x%04x, found 0x%04x)", ^^^^^^^^^^^^^^^^^^^^^^^^^^^ I have the same questions as above here. >+ crtc->base.base.id, crtc->base.name, >+ use_mplla ? "A" : "B", >+ i, >+ mpll_sw_state->cmn[i], mpll_hw_state.cmn[i]); >+ } >+} >+ >+void intel_cx0pll_state_verify(struct intel_atomic_state *state, >+ struct intel_crtc *crtc) >+{ >+ struct drm_i915_private *i915 = to_i915(state->base.dev); >+ const struct intel_crtc_state *new_crtc_state = >+ intel_atomic_get_new_crtc_state(state, crtc); >+ struct intel_encoder *encoder; >+ enum phy phy; >+ >+ if (DISPLAY_VER(i915) < 14) >+ return; >+ >+ if (!new_crtc_state->hw.active) >+ return; >+ >+ /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ >+ if (!intel_crtc_needs_modeset(new_crtc_state) && >+ !intel_crtc_needs_fastset(new_crtc_state)) >+ return; >+ >+ encoder = intel_get_crtc_new_encoder(state, new_crtc_state); >+ phy = intel_port_to_phy(i915, encoder->port); >+ >+ if (intel_is_c10phy(i915, phy)) >+ intel_c10pll_state_verify(new_crtc_state, crtc, encoder); >+ else >+ intel_c20pll_state_verify(new_crtc_state, crtc, encoder); >+} >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h >index 222aed16e739..c6682677253a 100644 >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h >@@ -38,7 +38,7 @@ int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder, > > void intel_c10pll_dump_hw_state(struct drm_i915_private *dev_priv, > const struct intel_c10pll_state *hw_state); >-void intel_c10pll_state_verify(struct intel_atomic_state *state, >+void intel_cx0pll_state_verify(struct intel_atomic_state *state, > struct intel_crtc *crtc); > void intel_c20pll_dump_hw_state(struct drm_i915_private *i915, > const struct intel_c20pll_state *hw_state); >diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c >index 5e1c2c780412..076298a8d405 100644 >--- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c >+++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c >@@ -244,7 +244,7 @@ void intel_modeset_verify_crtc(struct intel_atomic_state *state, > verify_crtc_state(state, crtc); > intel_shared_dpll_state_verify(state, crtc); > intel_mpllb_state_verify(state, crtc); >- intel_c10pll_state_verify(state, crtc); >+ intel_cx0pll_state_verify(state, crtc); > } > > void intel_modeset_verify_disabled(struct intel_atomic_state *state) >-- >2.34.1 >
> -----Original Message----- > From: Sousa, Gustavo <gustavo.sousa@intel.com> > Sent: Tuesday, November 7, 2023 2:52 PM > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/mtl: C20 state verification > > Quoting Mika Kahola (2023-11-06 05:33:22-03:00) > >Add state verification for C20 as we have one for C10. > > > >v2: use register values as u32 instead of u8 > > > >Signed-off-by: Mika Kahola <mika.kahola@intel.com> > > The patch looks good to me. > > I had some minor suggestions further down. With those applied, > > Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com> > > >--- > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 107 ++++++++++++++---- > > drivers/gpu/drm/i915/display/intel_cx0_phy.h | 2 +- > > .../drm/i915/display/intel_modeset_verify.c | 2 +- > > 3 files changed, 84 insertions(+), 27 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >index b2ad4c6172f6..87329bd2272a 100644 > >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c > >@@ -3017,35 +3017,15 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder, > > return ICL_PORT_DPLL_DEFAULT; } > > > >-void intel_c10pll_state_verify(struct intel_atomic_state *state, > >- struct intel_crtc *crtc) > >+static void intel_c10pll_state_verify(const struct intel_crtc_state *state, > >+ struct intel_crtc *crtc, > >+ struct intel_encoder *encoder) > > { > >- struct drm_i915_private *i915 = to_i915(state->base.dev); > >- const struct intel_crtc_state *new_crtc_state = > >- intel_atomic_get_new_crtc_state(state, crtc); > >+ struct drm_i915_private *i915 = to_i915(crtc->base.dev); > > struct intel_c10pll_state mpllb_hw_state = {}; > >- const struct intel_c10pll_state *mpllb_sw_state = &new_crtc_state->cx0pll_state.c10; > >- struct intel_encoder *encoder; > >- enum phy phy; > >+ const struct intel_c10pll_state *mpllb_sw_state = > >+ &state->cx0pll_state.c10; > > int i; > > > >- if (DISPLAY_VER(i915) < 14) > >- return; > >- > >- if (!new_crtc_state->hw.active) > >- return; > >- > >- /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ > >- if (!intel_crtc_needs_modeset(new_crtc_state) && > >- !intel_crtc_needs_fastset(new_crtc_state)) > >- return; > >- > >- encoder = intel_get_crtc_new_encoder(state, new_crtc_state); > >- phy = intel_port_to_phy(i915, encoder->port); > >- > >- if (!intel_is_c10phy(i915, phy)) > >- return; > >- > > intel_c10pll_readout_hw_state(encoder, &mpllb_hw_state); > > > > for (i = 0; i < ARRAY_SIZE(mpllb_sw_state->pll); i++) { @@ > >-3091,3 +3071,80 @@ int intel_cx0pll_calc_port_clock(struct > >intel_encoder *encoder, > > > > return intel_c20pll_calc_port_clock(encoder, &pll_state->c20); > > } > >+ > >+static void intel_c20pll_state_verify(const struct intel_crtc_state *state, > >+ struct intel_crtc *crtc, > >+ struct intel_encoder *encoder) { > >+ struct drm_i915_private *i915 = to_i915(crtc->base.dev); > >+ struct intel_c20pll_state mpll_hw_state = {}; > >+ const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20; > >+ bool use_mplla; > >+ int i; > >+ > >+ intel_c20pll_readout_hw_state(encoder, &mpll_hw_state); > > Suggestion: > Maybe have intel_cx0pll_state_verify() call > intel_cx0pll_readout_hw_state() and have this (and also > intel_c10pll_state_verify()) receiving the pointer to the hardware > state? > > This would have the benefit of having the C10 and C20 specific > functions for hardware readout called from a single place. I had an idea to keep C20 related functions inside C20 state verification. But since we now have this abstraction in place, we could do like you suggested. I will modify the code accordingly. > > >+ > >+ use_mplla = intel_c20_use_mplla(mpll_hw_state.clock); > >+ if (use_mplla) { > >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) { > >+ I915_STATE_WARN(i915, mpll_hw_state.mplla[i] != mpll_sw_state->mplla[i], > >+ "[CRTC:%d:%s] mismatch in > >+ C20MPLLA: Register[%d] (expected 0x%04x, found 0x%04x)", > ^^^^^^^^^^^^^^^^^^^^^^ > > Maybe simply use C20 MPLLA[%d] here? Yes, this was just to keep this similar to C10. > > >+ crtc->base.base.id, crtc->base.name, i, > >+ mpll_sw_state->mplla[i], mpll_hw_state.mplla[i]); > >+ } > >+ } else { > >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mpllb); i++) { > >+ I915_STATE_WARN(i915, mpll_hw_state.mpllb[i] != mpll_sw_state->mpllb[i], > >+ "[CRTC:%d:%s] mismatch in > >+ C20MPLLB: Register[%d] (expected 0x%04x, found 0x%04x)", > ^^^^^^^^^^^^^^^^^^^^^^ > > Same suggestion as above here. Yes. > > >+ crtc->base.base.id, crtc->base.name, i, > >+ mpll_sw_state->mpllb[i], mpll_hw_state.mpllb[i]); > >+ } > >+ } > >+ > >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->tx); i++) { > >+ I915_STATE_WARN(i915, mpll_hw_state.tx[i] != mpll_sw_state->tx[i], > >+ "[CRTC:%d:%s] mismatch in C20MPLL%s: > >+ Register TX[%i] (expected 0x%04x, found 0x%04x)", > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Do tx[i] values really belong to MPLLA or MPLLB? Or is > it rather something independent of both that really > belongs the C20 phy? TX and CMN do not really belong to MPLLA or MPLLB as such. I will update the wording to fit better. Thanks for the review! -Mika- > > If the latter is true, then maybe just use C20 TX[%d] > here? > > >+ crtc->base.base.id, crtc->base.name, > >+ use_mplla ? "A" : "B", > >+ i, > >+ mpll_sw_state->tx[i], mpll_hw_state.tx[i]); > >+ } > >+ > >+ for (i = 0; i < ARRAY_SIZE(mpll_sw_state->cmn); i++) { > >+ I915_STATE_WARN(i915, mpll_hw_state.cmn[i] != mpll_sw_state->cmn[i], > >+ "[CRTC:%d:%s] mismatch in C20MPLL%s: > >+ Register CMN[%i] (expected 0x%04x, found 0x%04x)", > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > I have the same questions as above here. > > >+ crtc->base.base.id, crtc->base.name, > >+ use_mplla ? "A" : "B", > >+ i, > >+ mpll_sw_state->cmn[i], mpll_hw_state.cmn[i]); > >+ } > >+} > >+ > >+void intel_cx0pll_state_verify(struct intel_atomic_state *state, > >+ struct intel_crtc *crtc) { > >+ struct drm_i915_private *i915 = to_i915(state->base.dev); > >+ const struct intel_crtc_state *new_crtc_state = > >+ intel_atomic_get_new_crtc_state(state, crtc); > >+ struct intel_encoder *encoder; > >+ enum phy phy; > >+ > >+ if (DISPLAY_VER(i915) < 14) > >+ return; > >+ > >+ if (!new_crtc_state->hw.active) > >+ return; > >+ > >+ /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ > >+ if (!intel_crtc_needs_modeset(new_crtc_state) && > >+ !intel_crtc_needs_fastset(new_crtc_state)) > >+ return; > >+ > >+ encoder = intel_get_crtc_new_encoder(state, new_crtc_state); > >+ phy = intel_port_to_phy(i915, encoder->port); > >+ > >+ if (intel_is_c10phy(i915, phy)) > >+ intel_c10pll_state_verify(new_crtc_state, crtc, encoder); > >+ else > >+ intel_c20pll_state_verify(new_crtc_state, crtc, > >+encoder); } > >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > >b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > >index 222aed16e739..c6682677253a 100644 > >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h > >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h > >@@ -38,7 +38,7 @@ int intel_cx0pll_calc_port_clock(struct intel_encoder > >*encoder, > > > > void intel_c10pll_dump_hw_state(struct drm_i915_private *dev_priv, > > const struct intel_c10pll_state > >*hw_state); -void intel_c10pll_state_verify(struct intel_atomic_state > >*state, > >+void intel_cx0pll_state_verify(struct intel_atomic_state *state, > > struct intel_crtc *crtc); void > >intel_c20pll_dump_hw_state(struct drm_i915_private *i915, > > const struct intel_c20pll_state > >*hw_state); diff --git > >a/drivers/gpu/drm/i915/display/intel_modeset_verify.c > >b/drivers/gpu/drm/i915/display/intel_modeset_verify.c > >index 5e1c2c780412..076298a8d405 100644 > >--- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c > >+++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c > >@@ -244,7 +244,7 @@ void intel_modeset_verify_crtc(struct intel_atomic_state *state, > > verify_crtc_state(state, crtc); > > intel_shared_dpll_state_verify(state, crtc); > > intel_mpllb_state_verify(state, crtc); > >- intel_c10pll_state_verify(state, crtc); > >+ intel_cx0pll_state_verify(state, crtc); > > } > > > > void intel_modeset_verify_disabled(struct intel_atomic_state *state) > >-- > >2.34.1 > >
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c index b2ad4c6172f6..87329bd2272a 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c @@ -3017,35 +3017,15 @@ intel_mtl_port_pll_type(struct intel_encoder *encoder, return ICL_PORT_DPLL_DEFAULT; } -void intel_c10pll_state_verify(struct intel_atomic_state *state, - struct intel_crtc *crtc) +static void intel_c10pll_state_verify(const struct intel_crtc_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) { - struct drm_i915_private *i915 = to_i915(state->base.dev); - const struct intel_crtc_state *new_crtc_state = - intel_atomic_get_new_crtc_state(state, crtc); + struct drm_i915_private *i915 = to_i915(crtc->base.dev); struct intel_c10pll_state mpllb_hw_state = {}; - const struct intel_c10pll_state *mpllb_sw_state = &new_crtc_state->cx0pll_state.c10; - struct intel_encoder *encoder; - enum phy phy; + const struct intel_c10pll_state *mpllb_sw_state = &state->cx0pll_state.c10; int i; - if (DISPLAY_VER(i915) < 14) - return; - - if (!new_crtc_state->hw.active) - return; - - /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ - if (!intel_crtc_needs_modeset(new_crtc_state) && - !intel_crtc_needs_fastset(new_crtc_state)) - return; - - encoder = intel_get_crtc_new_encoder(state, new_crtc_state); - phy = intel_port_to_phy(i915, encoder->port); - - if (!intel_is_c10phy(i915, phy)) - return; - intel_c10pll_readout_hw_state(encoder, &mpllb_hw_state); for (i = 0; i < ARRAY_SIZE(mpllb_sw_state->pll); i++) { @@ -3091,3 +3071,80 @@ int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder, return intel_c20pll_calc_port_clock(encoder, &pll_state->c20); } + +static void intel_c20pll_state_verify(const struct intel_crtc_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) +{ + struct drm_i915_private *i915 = to_i915(crtc->base.dev); + struct intel_c20pll_state mpll_hw_state = {}; + const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20; + bool use_mplla; + int i; + + intel_c20pll_readout_hw_state(encoder, &mpll_hw_state); + + use_mplla = intel_c20_use_mplla(mpll_hw_state.clock); + if (use_mplla) { + for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) { + I915_STATE_WARN(i915, mpll_hw_state.mplla[i] != mpll_sw_state->mplla[i], + "[CRTC:%d:%s] mismatch in C20MPLLA: Register[%d] (expected 0x%04x, found 0x%04x)", + crtc->base.base.id, crtc->base.name, i, + mpll_sw_state->mplla[i], mpll_hw_state.mplla[i]); + } + } else { + for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mpllb); i++) { + I915_STATE_WARN(i915, mpll_hw_state.mpllb[i] != mpll_sw_state->mpllb[i], + "[CRTC:%d:%s] mismatch in C20MPLLB: Register[%d] (expected 0x%04x, found 0x%04x)", + crtc->base.base.id, crtc->base.name, i, + mpll_sw_state->mpllb[i], mpll_hw_state.mpllb[i]); + } + } + + for (i = 0; i < ARRAY_SIZE(mpll_sw_state->tx); i++) { + I915_STATE_WARN(i915, mpll_hw_state.tx[i] != mpll_sw_state->tx[i], + "[CRTC:%d:%s] mismatch in C20MPLL%s: Register TX[%i] (expected 0x%04x, found 0x%04x)", + crtc->base.base.id, crtc->base.name, + use_mplla ? "A" : "B", + i, + mpll_sw_state->tx[i], mpll_hw_state.tx[i]); + } + + for (i = 0; i < ARRAY_SIZE(mpll_sw_state->cmn); i++) { + I915_STATE_WARN(i915, mpll_hw_state.cmn[i] != mpll_sw_state->cmn[i], + "[CRTC:%d:%s] mismatch in C20MPLL%s: Register CMN[%i] (expected 0x%04x, found 0x%04x)", + crtc->base.base.id, crtc->base.name, + use_mplla ? "A" : "B", + i, + mpll_sw_state->cmn[i], mpll_hw_state.cmn[i]); + } +} + +void intel_cx0pll_state_verify(struct intel_atomic_state *state, + struct intel_crtc *crtc) +{ + struct drm_i915_private *i915 = to_i915(state->base.dev); + const struct intel_crtc_state *new_crtc_state = + intel_atomic_get_new_crtc_state(state, crtc); + struct intel_encoder *encoder; + enum phy phy; + + if (DISPLAY_VER(i915) < 14) + return; + + if (!new_crtc_state->hw.active) + return; + + /* intel_get_crtc_new_encoder() only works for modeset/fastset commits */ + if (!intel_crtc_needs_modeset(new_crtc_state) && + !intel_crtc_needs_fastset(new_crtc_state)) + return; + + encoder = intel_get_crtc_new_encoder(state, new_crtc_state); + phy = intel_port_to_phy(i915, encoder->port); + + if (intel_is_c10phy(i915, phy)) + intel_c10pll_state_verify(new_crtc_state, crtc, encoder); + else + intel_c20pll_state_verify(new_crtc_state, crtc, encoder); +} diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h b/drivers/gpu/drm/i915/display/intel_cx0_phy.h index 222aed16e739..c6682677253a 100644 --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h @@ -38,7 +38,7 @@ int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder, void intel_c10pll_dump_hw_state(struct drm_i915_private *dev_priv, const struct intel_c10pll_state *hw_state); -void intel_c10pll_state_verify(struct intel_atomic_state *state, +void intel_cx0pll_state_verify(struct intel_atomic_state *state, struct intel_crtc *crtc); void intel_c20pll_dump_hw_state(struct drm_i915_private *i915, const struct intel_c20pll_state *hw_state); diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c index 5e1c2c780412..076298a8d405 100644 --- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c +++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c @@ -244,7 +244,7 @@ void intel_modeset_verify_crtc(struct intel_atomic_state *state, verify_crtc_state(state, crtc); intel_shared_dpll_state_verify(state, crtc); intel_mpllb_state_verify(state, crtc); - intel_c10pll_state_verify(state, crtc); + intel_cx0pll_state_verify(state, crtc); } void intel_modeset_verify_disabled(struct intel_atomic_state *state)
Add state verification for C20 as we have one for C10. v2: use register values as u32 instead of u8 Signed-off-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/display/intel_cx0_phy.c | 107 ++++++++++++++---- drivers/gpu/drm/i915/display/intel_cx0_phy.h | 2 +- .../drm/i915/display/intel_modeset_verify.c | 2 +- 3 files changed, 84 insertions(+), 27 deletions(-)