From: Jani Nikula <jani.nikula@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v2 06/12] drm/i915: Convert intel_dpll_dump_hw_state() to drm_printer
Date: Thu, 29 Feb 2024 21:43:41 +0200 [thread overview]
Message-ID: <874jdr6ope.fsf@intel.com> (raw)
In-Reply-To: <20240229184049.31165-1-ville.syrjala@linux.intel.com>
On Thu, 29 Feb 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Utilize drm_printer in pipe_config_pll_mismatch() to avoid
> a bit of code duplication.
>
> To achieve this we need to plumb the printer all way to the
> dpll_mgr .dump_hw_state() functions. Those are also used by
> intel_crtc_state_dump() which needs to be adjusted as well.
>
> v2: Convert a few misplaecd drm_dbg_kms() calls (Rodrigo)
> Drop the redundant drm_debug_enabled() check here
> instead of later (Jani)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
At a fairly quick glance,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> ---
> .../drm/i915/display/intel_crtc_state_dump.c | 5 +-
> drivers/gpu/drm/i915/display/intel_display.c | 28 +++--
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 105 ++++++++----------
> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 2 +
> 4 files changed, 66 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 4bcf446c75f4..59d2b3d39951 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -205,9 +205,12 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> const struct intel_plane_state *plane_state;
> struct intel_plane *plane;
> + struct drm_printer p;
> char buf[64];
> int i;
>
> + p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, NULL);
> +
> drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] enable: %s [%s]\n",
> crtc->base.base.id, crtc->base.name,
> str_yes_no(pipe_config->hw.enable), context);
> @@ -356,7 +359,7 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> pipe_config->ips_enabled, pipe_config->double_wide,
> pipe_config->has_drrs);
>
> - intel_dpll_dump_hw_state(i915, &pipe_config->dpll_hw_state);
> + intel_dpll_dump_hw_state(i915, &p, &pipe_config->dpll_hw_state);
>
> if (IS_CHERRYVIEW(i915))
> drm_dbg_kms(&i915->drm,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e5010049d52e..962b640e7c74 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4927,26 +4927,24 @@ pipe_config_pll_mismatch(bool fastset,
> const struct intel_dpll_hw_state *b)
> {
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + struct drm_printer p;
>
> if (fastset) {
> - if (!drm_debug_enabled(DRM_UT_KMS))
> - return;
> + p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, NULL);
>
> - drm_dbg_kms(&i915->drm,
> - "[CRTC:%d:%s] fastset requirement not met in %s\n",
> - crtc->base.base.id, crtc->base.name, name);
> - drm_dbg_kms(&i915->drm, "expected:\n");
> - intel_dpll_dump_hw_state(i915, a);
> - drm_dbg_kms(&i915->drm, "found:\n");
> - intel_dpll_dump_hw_state(i915, b);
> + drm_printf(&p, "[CRTC:%d:%s] fastset requirement not met in %s\n",
> + crtc->base.base.id, crtc->base.name, name);
> } else {
> - drm_err(&i915->drm, "[CRTC:%d:%s] mismatch in %s buffer\n",
> - crtc->base.base.id, crtc->base.name, name);
> - drm_err(&i915->drm, "expected:\n");
> - intel_dpll_dump_hw_state(i915, a);
> - drm_err(&i915->drm, "found:\n");
> - intel_dpll_dump_hw_state(i915, b);
> + p = drm_err_printer(&i915->drm, NULL);
> +
> + drm_printf(&p, "[CRTC:%d:%s] mismatch in %s\n",
> + crtc->base.base.id, crtc->base.name, name);
> }
> +
> + drm_printf(&p, "expected:\n");
> + intel_dpll_dump_hw_state(i915, &p, a);
> + drm_printf(&p, "found:\n");
> + intel_dpll_dump_hw_state(i915, &p, b);
> }
>
> bool
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index ff480f171f75..9542e62186e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -107,7 +107,7 @@ struct intel_dpll_mgr {
> struct intel_crtc *crtc,
> struct intel_encoder *encoder);
> void (*update_ref_clks)(struct drm_i915_private *i915);
> - void (*dump_hw_state)(struct drm_i915_private *i915,
> + void (*dump_hw_state)(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state);
> bool (*compare_hw_state)(const struct intel_dpll_hw_state *a,
> const struct intel_dpll_hw_state *b);
> @@ -634,16 +634,15 @@ static int ibx_get_dpll(struct intel_atomic_state *state,
> return 0;
> }
>
> -static void ibx_dump_hw_state(struct drm_i915_private *i915,
> +static void ibx_dump_hw_state(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> - drm_dbg_kms(&i915->drm,
> - "dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> - "fp0: 0x%x, fp1: 0x%x\n",
> - hw_state->dpll,
> - hw_state->dpll_md,
> - hw_state->fp0,
> - hw_state->fp1);
> + drm_printf(p, "dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> + "fp0: 0x%x, fp1: 0x%x\n",
> + hw_state->dpll,
> + hw_state->dpll_md,
> + hw_state->fp0,
> + hw_state->fp1);
> }
>
> static bool ibx_compare_hw_state(const struct intel_dpll_hw_state *a,
> @@ -1225,11 +1224,11 @@ static void hsw_update_dpll_ref_clks(struct drm_i915_private *i915)
> i915->display.dpll.ref_clks.nssc = 135000;
> }
>
> -static void hsw_dump_hw_state(struct drm_i915_private *i915,
> +static void hsw_dump_hw_state(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> - drm_dbg_kms(&i915->drm, "dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
> - hw_state->wrpll, hw_state->spll);
> + drm_printf(p, "dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
> + hw_state->wrpll, hw_state->spll);
> }
>
> static bool hsw_compare_hw_state(const struct intel_dpll_hw_state *a,
> @@ -1939,14 +1938,11 @@ static void skl_update_dpll_ref_clks(struct drm_i915_private *i915)
> i915->display.dpll.ref_clks.nssc = i915->display.cdclk.hw.ref;
> }
>
> -static void skl_dump_hw_state(struct drm_i915_private *i915,
> +static void skl_dump_hw_state(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> - drm_dbg_kms(&i915->drm, "dpll_hw_state: "
> - "ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
> - hw_state->ctrl1,
> - hw_state->cfgcr1,
> - hw_state->cfgcr2);
> + drm_printf(p, "dpll_hw_state: ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
> + hw_state->ctrl1, hw_state->cfgcr1, hw_state->cfgcr2);
> }
>
> static bool skl_compare_hw_state(const struct intel_dpll_hw_state *a,
> @@ -2402,23 +2398,16 @@ static void bxt_update_dpll_ref_clks(struct drm_i915_private *i915)
> /* DSI non-SSC ref 19.2MHz */
> }
>
> -static void bxt_dump_hw_state(struct drm_i915_private *i915,
> +static void bxt_dump_hw_state(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> - drm_dbg_kms(&i915->drm, "dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> - "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> - "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
> - hw_state->ebb0,
> - hw_state->ebb4,
> - hw_state->pll0,
> - hw_state->pll1,
> - hw_state->pll2,
> - hw_state->pll3,
> - hw_state->pll6,
> - hw_state->pll8,
> - hw_state->pll9,
> - hw_state->pll10,
> - hw_state->pcsdw12);
> + drm_printf(p, "dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> + "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> + "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
> + hw_state->ebb0, hw_state->ebb4,
> + hw_state->pll0, hw_state->pll1, hw_state->pll2, hw_state->pll3,
> + hw_state->pll6, hw_state->pll8, hw_state->pll9, hw_state->pll10,
> + hw_state->pcsdw12);
> }
>
> static bool bxt_compare_hw_state(const struct intel_dpll_hw_state *a,
> @@ -4026,28 +4015,26 @@ static void icl_update_dpll_ref_clks(struct drm_i915_private *i915)
> i915->display.dpll.ref_clks.nssc = i915->display.cdclk.hw.ref;
> }
>
> -static void icl_dump_hw_state(struct drm_i915_private *i915,
> +static void icl_dump_hw_state(struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> - drm_dbg_kms(&i915->drm,
> - "dpll_hw_state: cfgcr0: 0x%x, cfgcr1: 0x%x, div0: 0x%x, "
> - "mg_refclkin_ctl: 0x%x, hg_clktop2_coreclkctl1: 0x%x, "
> - "mg_clktop2_hsclkctl: 0x%x, mg_pll_div0: 0x%x, "
> - "mg_pll_div2: 0x%x, mg_pll_lf: 0x%x, "
> - "mg_pll_frac_lock: 0x%x, mg_pll_ssc: 0x%x, "
> - "mg_pll_bias: 0x%x, mg_pll_tdc_coldst_bias: 0x%x\n",
> - hw_state->cfgcr0, hw_state->cfgcr1,
> - hw_state->div0,
> - hw_state->mg_refclkin_ctl,
> - hw_state->mg_clktop2_coreclkctl1,
> - hw_state->mg_clktop2_hsclkctl,
> - hw_state->mg_pll_div0,
> - hw_state->mg_pll_div1,
> - hw_state->mg_pll_lf,
> - hw_state->mg_pll_frac_lock,
> - hw_state->mg_pll_ssc,
> - hw_state->mg_pll_bias,
> - hw_state->mg_pll_tdc_coldst_bias);
> + drm_printf(p, "dpll_hw_state: cfgcr0: 0x%x, cfgcr1: 0x%x, div0: 0x%x, "
> + "mg_refclkin_ctl: 0x%x, hg_clktop2_coreclkctl1: 0x%x, "
> + "mg_clktop2_hsclkctl: 0x%x, mg_pll_div0: 0x%x, "
> + "mg_pll_div2: 0x%x, mg_pll_lf: 0x%x, "
> + "mg_pll_frac_lock: 0x%x, mg_pll_ssc: 0x%x, "
> + "mg_pll_bias: 0x%x, mg_pll_tdc_coldst_bias: 0x%x\n",
> + hw_state->cfgcr0, hw_state->cfgcr1, hw_state->div0,
> + hw_state->mg_refclkin_ctl,
> + hw_state->mg_clktop2_coreclkctl1,
> + hw_state->mg_clktop2_hsclkctl,
> + hw_state->mg_pll_div0,
> + hw_state->mg_pll_div1,
> + hw_state->mg_pll_lf,
> + hw_state->mg_pll_frac_lock,
> + hw_state->mg_pll_ssc,
> + hw_state->mg_pll_bias,
> + hw_state->mg_pll_tdc_coldst_bias);
> }
>
> static bool icl_compare_hw_state(const struct intel_dpll_hw_state *a,
> @@ -4514,22 +4501,24 @@ void intel_dpll_sanitize_state(struct drm_i915_private *i915)
> }
>
> /**
> - * intel_dpll_dump_hw_state - write hw_state to dmesg
> + * intel_dpll_dump_hw_state - dump hw_state
> * @i915: i915 drm device
> - * @hw_state: hw state to be written to the log
> + * @p: where to print the state to
> + * @hw_state: hw state to be dumped
> *
> - * Write the relevant values in @hw_state to dmesg using drm_dbg_kms.
> + * Dumo out the relevant values in @hw_state.
> */
> void intel_dpll_dump_hw_state(struct drm_i915_private *i915,
> + struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state)
> {
> if (i915->display.dpll.mgr) {
> - i915->display.dpll.mgr->dump_hw_state(i915, hw_state);
> + i915->display.dpll.mgr->dump_hw_state(p, hw_state);
> } else {
> /* fallback for platforms that don't use the shared dpll
> * infrastructure
> */
> - ibx_dump_hw_state(i915, hw_state);
> + ibx_dump_hw_state(p, hw_state);
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index cc0e1386309d..d4d97e40440a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -36,6 +36,7 @@
>
> enum tc_port;
> struct drm_i915_private;
> +struct drm_printer;
> struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> @@ -377,6 +378,7 @@ void intel_dpll_readout_hw_state(struct drm_i915_private *i915);
> void intel_dpll_sanitize_state(struct drm_i915_private *i915);
>
> void intel_dpll_dump_hw_state(struct drm_i915_private *i915,
> + struct drm_printer *p,
> const struct intel_dpll_hw_state *hw_state);
> bool intel_dpll_compare_hw_state(struct drm_i915_private *i915,
> const struct intel_dpll_hw_state *a,
--
Jani Nikula, Intel
next prev parent reply other threads:[~2024-02-29 19:43 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 16:40 [PATCH 00/12] drm/i915: Use drm_printer more Ville Syrjala
2024-02-15 16:40 ` [PATCH 01/12] drm/i915: Indicate which pipe failed the fastset check overall Ville Syrjala
2024-02-22 21:46 ` Rodrigo Vivi
2024-02-23 19:47 ` Ville Syrjälä
2024-02-26 14:57 ` Jani Nikula
2024-02-26 15:10 ` Andy Shevchenko
2024-02-26 15:35 ` Jani Nikula
2024-02-26 16:30 ` Andy Shevchenko
2024-02-26 16:35 ` Ville Syrjälä
2024-02-27 9:38 ` Rasmus Villemoes
2024-02-27 18:32 ` Ville Syrjälä
2024-02-28 8:32 ` Rasmus Villemoes
2024-02-28 9:55 ` Petr Mladek
2024-02-15 16:40 ` [PATCH 02/12] drm/i915: Include CRTC info in infoframe mismatch prints Ville Syrjala
2024-02-22 21:47 ` Rodrigo Vivi
2024-02-23 19:50 ` Ville Syrjälä
2024-02-15 16:40 ` [PATCH 03/12] drm/i915: Include CRTC info in VSC SDP " Ville Syrjala
2024-02-22 21:48 ` Rodrigo Vivi
2024-02-15 16:40 ` [PATCH 04/12] drm/i915: Convert pipe_config_infoframe_mismatch() to drm_printer Ville Syrjala
2024-02-22 21:50 ` Rodrigo Vivi
2024-02-15 16:40 ` [PATCH 05/12] drm/i915: Convert pipe_config_buffer_mismatch() " Ville Syrjala
2024-02-22 21:51 ` Rodrigo Vivi
2024-02-15 16:40 ` [PATCH 06/12] drm/i915: Convert intel_dpll_dump_hw_state() " Ville Syrjala
2024-02-22 21:54 ` Rodrigo Vivi
2024-02-23 19:57 ` Ville Syrjälä
2024-02-29 18:40 ` [PATCH v2 " Ville Syrjala
2024-02-29 19:43 ` Jani Nikula [this message]
2024-02-15 16:40 ` [PATCH 07/12] drm/i915: Use drm_printer more extensively in intel_crtc_state_dump() Ville Syrjala
2024-02-22 21:57 ` Rodrigo Vivi
2024-02-23 19:59 ` Ville Syrjälä
2024-02-15 16:40 ` [PATCH 08/12] drm/i915: Convert the remaining state dump to drm_printer Ville Syrjala
2024-03-05 9:12 ` Jani Nikula
2024-02-15 16:40 ` [PATCH 09/12] drm/i915: Skip intel_crtc_state_dump() if debugs aren't enabled Ville Syrjala
2024-02-29 15:20 ` Jani Nikula
2024-02-29 15:21 ` Jani Nikula
2024-02-15 16:40 ` [PATCH 10/12] drm/i915: Relocate pipe_config_mismatch() Ville Syrjala
2024-02-29 15:21 ` Jani Nikula
2024-02-15 16:40 ` [PATCH 11/12] drm/i915: Reuse pipe_config_mismatch() more Ville Syrjala
2024-02-29 15:28 ` Jani Nikula
2024-02-29 18:42 ` [PATCH v2 " Ville Syrjala
2024-02-15 16:40 ` [PATCH 12/12] drm/i915: Create the printer only once in intel_pipe_config_compare() Ville Syrjala
2024-02-29 15:29 ` Jani Nikula
2024-02-29 18:42 ` [PATCH v2 " Ville Syrjala
2024-02-16 18:03 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more Patchwork
2024-02-16 18:03 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-02-16 18:15 ` ✓ Fi.CI.BAT: success " Patchwork
2024-02-17 7:24 ` ✗ Fi.CI.IGT: failure " Patchwork
2024-02-29 12:08 ` [PATCH 00/12] " Jani Nikula
2024-02-29 23:02 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more (rev4) Patchwork
2024-02-29 23:02 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-02-29 23:19 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-05 21:28 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more (rev5) Patchwork
2024-03-05 21:28 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-03-05 21:46 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-06 12:07 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more (rev6) Patchwork
2024-03-06 12:07 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-03-06 12:13 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-08 8:37 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more (rev7) Patchwork
2024-03-08 8:37 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-03-08 8:52 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-13 19:41 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Use drm_printer more (rev8) Patchwork
2024-03-13 19:41 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-03-13 19:54 ` ✓ Fi.CI.BAT: success " Patchwork
2024-03-14 2:49 ` ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jdr6ope.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=rodrigo.vivi@intel.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).