> -----Original Message-----
> From: Jani Nikula <jani.nik...@linux.intel.com>
> Sent: Wednesday, 1 October 2025 12.33
> To: Kahola, Mika <mika.kah...@intel.com>; intel-gfx@lists.freedesktop.org;
> intel...@lists.freedesktop.org
> Cc: Kahola, Mika <mika.kah...@intel.com>
> Subject: Re: [RFC PATCH 30/39] drm/i915/display: Add .dump_hw_state
>
> On Wed, 01 Oct 2025, Mika Kahola <mika.kah...@intel.com> wrote:
> > Add .dump_hw_state function pointer for MTL+ platforms to support dpll
> > framework. While at it, switch to use drm_printer structure to print
> > hw state information.
> >
> > Signed-off-by: Mika Kahola <mika.kah...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 68
> > +++++++++---------- drivers/gpu/drm/i915/display/intel_cx0_phy.h |
> > 3 +- drivers/gpu/drm/i915/display/intel_display.c | 5 +-
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 7 ++
> > 4 files changed, 45 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index f0cba843fed1..ec91186848d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2302,7 +2302,7 @@ static void intel_c10_pll_program(struct
> > intel_display *display,
> > intel_c10_msgbus_update_config(encoder, INTEL_CX0_LANE0, true); }
> >
> > -static void intel_c10pll_dump_hw_state(struct intel_display *display,
> > +static void intel_c10pll_dump_hw_state(struct drm_printer *p,
> > const struct intel_c10pll_state
> > *hw_state) {
> > bool fracen;
> > @@ -2311,33 +2311,33 @@ static void intel_c10pll_dump_hw_state(struct
> > intel_display *display,
> > unsigned int multiplier, tx_clk_div;
> >
> > fracen = hw_state->pll[0] & C10_PLL0_FRACEN;
> > - drm_dbg_kms(display->drm, "c10pll_hw_state: clock: %d, fracen: %s, ",
> > - hw_state->clock, str_yes_no(fracen));
> > + drm_printf(p, "c10pll_hw_state: clock: %d, fracen: %s, ",
> > + hw_state->clock, str_yes_no(fracen));
> >
> > if (fracen) {
> > frac_quot = hw_state->pll[12] << 8 | hw_state->pll[11];
> > frac_rem = hw_state->pll[14] << 8 | hw_state->pll[13];
> > frac_den = hw_state->pll[10] << 8 | hw_state->pll[9];
> > - drm_dbg_kms(display->drm, "quot: %u, rem: %u, den: %u,\n",
> > - frac_quot, frac_rem, frac_den);
> > + drm_printf(p, "quot: %u, rem: %u, den: %u,\n",
> > + frac_quot, frac_rem, frac_den);
> > }
> >
> > multiplier = (REG_FIELD_GET8(C10_PLL3_MULTIPLIERH_MASK,
> > hw_state->pll[3]) << 8 |
> > hw_state->pll[2]) / 2 + 16;
> > tx_clk_div = REG_FIELD_GET8(C10_PLL15_TXCLKDIV_MASK, hw_state->pll[15]);
> > - drm_dbg_kms(display->drm,
> > - "multiplier: %u, tx_clk_div: %u.\n", multiplier,
> > tx_clk_div);
> > + drm_printf(p,
> > + "multiplier: %u, tx_clk_div: %u.\n", multiplier, tx_clk_div);
> >
> > - drm_dbg_kms(display->drm, "c10pll_rawhw_state:");
> > - drm_dbg_kms(display->drm, "tx: 0x%x, cmn: 0x%x\n", hw_state->tx,
> > - hw_state->cmn);
> > + drm_printf(p, "c10pll_rawhw_state:");
> > + drm_printf(p, "tx: 0x%x, cmn: 0x%x\n", hw_state->tx,
> > + hw_state->cmn);
> >
> > BUILD_BUG_ON(ARRAY_SIZE(hw_state->pll) % 4);
> > for (i = 0; i < ARRAY_SIZE(hw_state->pll); i = i + 4)
> > - drm_dbg_kms(display->drm,
> > - "pll[%d] = 0x%x, pll[%d] = 0x%x, pll[%d] = 0x%x,
> > pll[%d] = 0x%x\n",
> > - i, hw_state->pll[i], i + 1, hw_state->pll[i + 1],
> > - i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i
> > + 3]);
> > + drm_printf(p,
> > + "pll[%d] = 0x%x, pll[%d] = 0x%x, pll[%d] = 0x%x,
> > pll[%d] = 0x%x\n",
> > + i, hw_state->pll[i], i + 1, hw_state->pll[i + 1],
> > + i + 2, hw_state->pll[i + 2], i + 3, hw_state->pll[i
> > + 3]);
> > }
> >
> > /*
> > @@ -2832,46 +2832,46 @@ static void intel_c20pll_readout_hw_state(struct
> > intel_encoder *encoder,
> > cx0pll_state->ssc_enabled = readout_ssc_state(encoder,
> > intel_c20phy_use_mpllb(pll_state));
> > }
> >
> > -static void intel_c20pll_dump_hw_state(struct intel_display *display,
> > +static void intel_c20pll_dump_hw_state(struct drm_printer *p,
> > const struct intel_c20pll_state
> > *hw_state) {
> > int i;
> >
> > - drm_dbg_kms(display->drm, "c20pll_hw_state clock: %d:\n",
> > hw_state->clock);
> > - drm_dbg_kms(display->drm,
> > - "tx[0] = 0x%.4x, tx[1] = 0x%.4x, tx[2] = 0x%.4x\n",
> > - hw_state->tx[0], hw_state->tx[1], hw_state->tx[2]);
> > - drm_dbg_kms(display->drm,
> > - "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3]
> > = 0x%.4x\n",
> > - hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2],
> > hw_state->cmn[3]);
> > + drm_printf(p, "c20pll_hw_state: clock: %d\n", hw_state->clock);
> > + drm_printf(p,
> > + "tx[0] = 0x%.4x, tx[1] = 0x%.4x, tx[2] = 0x%.4x\n",
> > + hw_state->tx[0], hw_state->tx[1], hw_state->tx[2]);
> > + drm_printf(p,
> > + "cmn[0] = 0x%.4x, cmn[1] = 0x%.4x, cmn[2] = 0x%.4x, cmn[3] =
> > 0x%.4x\n",
> > + hw_state->cmn[0], hw_state->cmn[1], hw_state->cmn[2],
> > +hw_state->cmn[3]);
> >
> > if (intel_c20phy_use_mpllb(hw_state)) {
> > for (i = 0; i < ARRAY_SIZE(hw_state->mpllb); i++)
> > - drm_dbg_kms(display->drm, "mpllb[%d] = 0x%.4x\n", i,
> > - hw_state->mpllb[i]);
> > + drm_printf(p, "mpllb[%d] = 0x%.4x\n", i,
> > + hw_state->mpllb[i]);
> > } else {
> > for (i = 0; i < ARRAY_SIZE(hw_state->mplla); i++)
> > - drm_dbg_kms(display->drm, "mplla[%d] = 0x%.4x\n", i,
> > - hw_state->mplla[i]);
> > + drm_printf(p, "mplla[%d] = 0x%.4x\n", i,
> > + hw_state->mplla[i]);
> >
> > /* For full coverage, also print the additional PLL B entry. */
> > WARN_ON(i + 1 != ARRAY_SIZE(hw_state->mpllb));
> > - drm_dbg_kms(display->drm, "mpllb[%d] = 0x%.4x\n", i,
> > hw_state->mpllb[i]);
> > + drm_printf(p, "mpllb[%d] = 0x%.4x\n", i, hw_state->mpllb[i]);
> > }
> > }
> >
> > -void intel_cx0pll_dump_hw_state(struct intel_display *display,
> > +void intel_cx0pll_dump_hw_state(struct drm_printer *p,
> > const struct intel_cx0pll_state *hw_state) {
> > - drm_dbg_kms(display->drm,
> > - "cx0pll_hw_state: lane_count: %d, ssc_enabled: %s, use_c10:
> > %s, tbt_mode: %s\n",
> > - hw_state->lane_count, str_yes_no(hw_state->ssc_enabled),
> > - str_yes_no(hw_state->use_c10),
> > str_yes_no(hw_state->tbt_mode));
> > + drm_printf(p,
> > + "cx0pll_hw_state: lane_count: %d, ssc_enabled: %s, use_c10:
> > %s, tbt_mode: %s\n",
> > + hw_state->lane_count, str_yes_no(hw_state->ssc_enabled),
> > + str_yes_no(hw_state->use_c10),
> > str_yes_no(hw_state->tbt_mode));
> >
> > if (hw_state->use_c10)
> > - intel_c10pll_dump_hw_state(display, &hw_state->c10);
> > + intel_c10pll_dump_hw_state(p, &hw_state->c10);
> > else
> > - intel_c20pll_dump_hw_state(display, &hw_state->c20);
> > + intel_c20pll_dump_hw_state(p, &hw_state->c20);
> > }
> >
> > static bool intel_c20_protocol_switch_valid(struct intel_encoder
> > *encoder) diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > index 7b88c3fe9de1..3d0073153463 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > @@ -6,6 +6,7 @@
> > #ifndef __INTEL_CX0_PHY_H__
> > #define __INTEL_CX0_PHY_H__
> >
> > +#include <drm/drm_print.h>
>
> Please dont include headers from headers when a simple forward declaration
> will do.
Thanks for noticing. I will switch to use forward declaration instead of
including a header.
>
> BR,
> Jani.
>
> > #include <linux/types.h>
> >
> > enum icl_port_dpll_id;
> > @@ -36,7 +37,7 @@ void intel_cx0pll_readout_hw_state(struct
> > intel_encoder *encoder, int intel_cx0pll_calc_port_clock(struct
> > intel_encoder *encoder,
> > const struct intel_cx0pll_state *pll_state);
> >
> > -void intel_cx0pll_dump_hw_state(struct intel_display *display,
> > +void intel_cx0pll_dump_hw_state(struct drm_printer *p,
> > const struct intel_cx0pll_state *hw_state);
> > bool
> > intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> > const struct intel_cx0pll_state *b); diff
> > --git
> > a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b57efd870774..1b3e6ccd8e37 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4939,15 +4939,14 @@ pipe_config_cx0pll_mismatch(struct drm_printer *p,
> > bool fastset,
> > const struct intel_cx0pll_state *a,
> > const struct intel_cx0pll_state *b) {
> > - struct intel_display *display = to_intel_display(crtc);
> > char *chipname = a->use_c10 ? "C10" : "C20";
> >
> > pipe_config_mismatch(p, fastset, crtc, name, chipname);
> >
> > drm_printf(p, "expected:\n");
> > - intel_cx0pll_dump_hw_state(display, a);
> > + intel_cx0pll_dump_hw_state(p, a);
> > drm_printf(p, "found:\n");
> > - intel_cx0pll_dump_hw_state(display, b);
> > + intel_cx0pll_dump_hw_state(p, b);
> > }
> >
> > static bool allow_vblank_delay_fastset(const struct intel_crtc_state
> > *old_crtc_state) diff --git
> > a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index 617139c6e3eb..3e27cc0004d5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -4426,6 +4426,12 @@ static int mtl_compute_dplls(struct
> > intel_atomic_state *state,
> > return mtl_compute_c20phy_dplls(state, crtc, encoder); }
> >
> > +static void mtl_dump_hw_state(struct drm_printer *p,
> > + const struct intel_dpll_hw_state *dpll_hw_state) {
> > + intel_cx0pll_dump_hw_state(p, &dpll_hw_state->cx0pll); }
> > +
> > __maybe_unused
> > static const struct intel_dpll_mgr mtl_pll_mgr = {
> > .dpll_info = mtl_plls,
> > @@ -4434,6 +4440,7 @@ static const struct intel_dpll_mgr mtl_pll_mgr = {
> > .put_dplls = icl_put_dplls,
> > .update_active_dpll = icl_update_active_dpll,
> > .update_ref_clks = icl_update_dpll_ref_clks,
> > + .dump_hw_state = mtl_dump_hw_state,
> > };
> >
> > /**
>
> --
> Jani Nikula, Intel
