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@xxxxxxxxx>
The patch looks good to me.
I had some minor suggestions further down. With those applied,
Reviewed-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>---
> 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
>
