[Intel-gfx] [PATCH v2] drm/i915/mtl: C20 state verification

Kahola, Mika mika.kahola at intel.com
Tue Nov 7 14:05:29 UTC 2023


> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa at intel.com>
> Sent: Tuesday, November 7, 2023 2:52 PM
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at 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 at intel.com>
> 
> The patch looks good to me.
> 
> I had some minor suggestions further down. With those applied,
> 
> Reviewed-by: Gustavo Sousa <gustavo.sousa at 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
> >


More information about the Intel-gfx mailing list