From: "Kahola, Mika" <mika.kahola@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL state tracking
Date: Thu, 4 Mar 2021 10:52:30 +0000 [thread overview]
Message-ID: <e288be0df88240eeb3855a2ce8073dc7@intel.com> (raw)
In-Reply-To: <20210224144214.24803-4-ville.syrjala@linux.intel.com>
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Wednesday, February 24, 2021 4:42 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL
> state tracking
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> All the other places we have use pipes instead of crtc indices when tracking
> resource usage. Life is easier when we do it the same way always, so switch
> the dpll mgr to using pipes as well. Looks like it was actually mixing these up
> in some cases so it would not even have worked correctly except when the
> device has a contiguous set of pipes starting from pipe A.
> Granted, that is the typical case but supposedly it may not always hold on
> modern hw.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 40 ++++++++--------
> .../drm/i915/display/intel_display_debugfs.c | 4 +-
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 48 ++++++++++---------
> drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 8 ++--
> 4 files changed, 51 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index b34620545d3b..958c2a796bae 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9653,7 +9653,7 @@ verify_single_dpll_state(struct drm_i915_private
> *dev_priv,
> struct intel_crtc_state *new_crtc_state) {
> struct intel_dpll_hw_state dpll_hw_state;
> - unsigned int crtc_mask;
> + u8 pipe_mask;
> bool active;
>
> memset(&dpll_hw_state, 0, sizeof(dpll_hw_state)); @@ -9666,34
> +9666,34 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
> I915_STATE_WARN(!pll->on && pll->active_mask,
> "pll in active use but not on in sw tracking\n");
> I915_STATE_WARN(pll->on && !pll->active_mask,
> - "pll is on but not used by any active crtc\n");
> + "pll is on but not used by any active pipe\n");
> I915_STATE_WARN(pll->on != active,
> "pll on state mismatch (expected %i, found %i)\n",
> pll->on, active);
> }
>
> if (!crtc) {
> - I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
> - "more active pll users than references: %x vs
> %x\n",
> - pll->active_mask, pll->state.crtc_mask);
> + I915_STATE_WARN(pll->active_mask & ~pll-
> >state.pipe_mask,
> + "more active pll users than references: 0x%x
> vs 0x%x\n",
> + pll->active_mask, pll->state.pipe_mask);
>
> return;
> }
>
> - crtc_mask = drm_crtc_mask(&crtc->base);
> + pipe_mask = BIT(crtc->pipe);
>
> if (new_crtc_state->hw.active)
> - I915_STATE_WARN(!(pll->active_mask & crtc_mask),
> - "pll active mismatch (expected pipe %c in
> active mask 0x%02x)\n",
> + I915_STATE_WARN(!(pll->active_mask & pipe_mask),
> + "pll active mismatch (expected pipe %c in
> active mask 0x%x)\n",
> pipe_name(crtc->pipe), pll->active_mask);
> else
> - I915_STATE_WARN(pll->active_mask & crtc_mask,
> - "pll active mismatch (didn't expect pipe %c in
> active mask 0x%02x)\n",
> + I915_STATE_WARN(pll->active_mask & pipe_mask,
> + "pll active mismatch (didn't expect pipe %c in
> active mask
> +0x%x)\n",
> pipe_name(crtc->pipe), pll->active_mask);
>
> - I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
> - "pll enabled crtcs mismatch (expected 0x%x in
> 0x%02x)\n",
> - crtc_mask, pll->state.crtc_mask);
> + I915_STATE_WARN(!(pll->state.pipe_mask & pipe_mask),
> + "pll enabled crtcs mismatch (expected 0x%x in
> 0x%x)\n",
> + pipe_mask, pll->state.pipe_mask);
>
> I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
> &dpll_hw_state,
> @@ -9713,15 +9713,15 @@ verify_shared_dpll_state(struct intel_crtc *crtc,
>
> if (old_crtc_state->shared_dpll &&
> old_crtc_state->shared_dpll != new_crtc_state->shared_dpll) {
> - unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> + u8 pipe_mask = BIT(crtc->pipe);
> struct intel_shared_dpll *pll = old_crtc_state->shared_dpll;
>
> - I915_STATE_WARN(pll->active_mask & crtc_mask,
> - "pll active mismatch (didn't expect pipe %c in
> active mask)\n",
> - pipe_name(crtc->pipe));
> - I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
> - "pll enabled crtcs mismatch (found %x in
> enabled mask)\n",
> - pipe_name(crtc->pipe));
> + I915_STATE_WARN(pll->active_mask & pipe_mask,
> + "pll active mismatch (didn't expect pipe %c in
> active mask (0x%x))\n",
> + pipe_name(crtc->pipe), pll->active_mask);
> + I915_STATE_WARN(pll->state.pipe_mask & pipe_mask,
> + "pll enabled crtcs mismatch (found %x in
> enabled mask (0x%x))\n",
> + pipe_name(crtc->pipe), pll-
> >state.pipe_mask);
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 35f176ea8280..20194ccfec05 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -1094,8 +1094,8 @@ static int i915_shared_dplls_info(struct seq_file
> *m, void *unused)
>
> seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->info->name,
> pll->info->id);
> - seq_printf(m, " crtc_mask: 0x%08x, active: 0x%x, on: %s\n",
> - pll->state.crtc_mask, pll->active_mask, yesno(pll-
> >on));
> + seq_printf(m, " pipe_mask: 0x%x, active: 0x%x, on: %s\n",
> + pll->state.pipe_mask, pll->active_mask, yesno(pll-
> >on));
> seq_printf(m, " tracked hardware state:\n");
> seq_printf(m, " dpll: 0x%08x\n", pll->state.hw_state.dpll);
> seq_printf(m, " dpll_md: 0x%08x\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 529b1d569af2..a68ae90b07e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -176,7 +176,7 @@ void intel_prepare_shared_dpll(const struct
> intel_crtc_state *crtc_state)
> return;
>
> mutex_lock(&dev_priv->dpll.lock);
> - drm_WARN_ON(&dev_priv->drm, !pll->state.crtc_mask);
> + drm_WARN_ON(&dev_priv->drm, !pll->state.pipe_mask);
> if (!pll->active_mask) {
> drm_dbg(&dev_priv->drm, "setting up %s\n", pll->info-
> >name);
> drm_WARN_ON(&dev_priv->drm, pll->on); @@ -198,7
> +198,7 @@ void intel_enable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> - unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> + unsigned int pipe_mask = BIT(crtc->pipe);
> unsigned int old_mask;
>
> if (drm_WARN_ON(&dev_priv->drm, pll == NULL)) @@ -207,16
> +207,16 @@ void intel_enable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
> mutex_lock(&dev_priv->dpll.lock);
> old_mask = pll->active_mask;
>
> - if (drm_WARN_ON(&dev_priv->drm, !(pll->state.crtc_mask &
> crtc_mask)) ||
> - drm_WARN_ON(&dev_priv->drm, pll->active_mask & crtc_mask))
> + if (drm_WARN_ON(&dev_priv->drm, !(pll->state.pipe_mask &
> pipe_mask)) ||
> + drm_WARN_ON(&dev_priv->drm, pll->active_mask & pipe_mask))
> goto out;
>
> - pll->active_mask |= crtc_mask;
> + pll->active_mask |= pipe_mask;
>
> drm_dbg_kms(&dev_priv->drm,
> - "enable %s (active %x, on? %d) for crtc %d\n",
> + "enable %s (active 0x%x, on? %d) for [CRTC:%d:%s]\n",
> pll->info->name, pll->active_mask, pll->on,
> - crtc->base.base.id);
> + crtc->base.base.id, crtc->base.name);
>
> if (old_mask) {
> drm_WARN_ON(&dev_priv->drm, !pll->on); @@ -244,7
> +244,7 @@ void intel_disable_shared_dpll(const struct intel_crtc_state
> *crtc_state)
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct intel_shared_dpll *pll = crtc_state->shared_dpll;
> - unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
> + unsigned int pipe_mask = BIT(crtc->pipe);
>
> /* PCH only available on ILK+ */
> if (INTEL_GEN(dev_priv) < 5)
> @@ -254,18 +254,20 @@ void intel_disable_shared_dpll(const struct
> intel_crtc_state *crtc_state)
> return;
>
> mutex_lock(&dev_priv->dpll.lock);
> - if (drm_WARN_ON(&dev_priv->drm, !(pll->active_mask &
> crtc_mask)))
> + if (drm_WARN(&dev_priv->drm, !(pll->active_mask & pipe_mask),
> + "%s not used by [CRTC:%d:%s]\n", pll->info->name,
> + crtc->base.base.id, crtc->base.name))
> goto out;
>
> drm_dbg_kms(&dev_priv->drm,
> - "disable %s (active %x, on? %d) for crtc %d\n",
> + "disable %s (active 0x%x, on? %d) for [CRTC:%d:%s]\n",
> pll->info->name, pll->active_mask, pll->on,
> - crtc->base.base.id);
> + crtc->base.base.id, crtc->base.name);
>
> assert_shared_dpll_enabled(dev_priv, pll);
> drm_WARN_ON(&dev_priv->drm, !pll->on);
>
> - pll->active_mask &= ~crtc_mask;
> + pll->active_mask &= ~pipe_mask;
> if (pll->active_mask)
> goto out;
>
> @@ -296,7 +298,7 @@ intel_find_shared_dpll(struct intel_atomic_state
> *state,
> pll = &dev_priv->dpll.shared_dplls[i];
>
> /* Only want to check enabled timings first */
> - if (shared_dpll[i].crtc_mask == 0) {
> + if (shared_dpll[i].pipe_mask == 0) {
> if (!unused_pll)
> unused_pll = pll;
> continue;
> @@ -306,10 +308,10 @@ intel_find_shared_dpll(struct intel_atomic_state
> *state,
> &shared_dpll[i].hw_state,
> sizeof(*pll_state)) == 0) {
> drm_dbg_kms(&dev_priv->drm,
> - "[CRTC:%d:%s] sharing existing %s (crtc
> mask 0x%08x, active %x)\n",
> + "[CRTC:%d:%s] sharing existing %s (pipe
> mask 0x%x, active
> +0x%x)\n",
> crtc->base.base.id, crtc->base.name,
> pll->info->name,
> - shared_dpll[i].crtc_mask,
> + shared_dpll[i].pipe_mask,
> pll->active_mask);
> return pll;
> }
> @@ -338,13 +340,13 @@ intel_reference_shared_dpll(struct
> intel_atomic_state *state,
>
> shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
>
> - if (shared_dpll[id].crtc_mask == 0)
> + if (shared_dpll[id].pipe_mask == 0)
> shared_dpll[id].hw_state = *pll_state;
>
> drm_dbg(&i915->drm, "using %s for pipe %c\n", pll->info->name,
> pipe_name(crtc->pipe));
>
> - shared_dpll[id].crtc_mask |= 1 << crtc->pipe;
> + shared_dpll[id].pipe_mask |= BIT(crtc->pipe);
> }
>
> static void intel_unreference_shared_dpll(struct intel_atomic_state *state,
> @@ -354,7 +356,7 @@ static void intel_unreference_shared_dpll(struct
> intel_atomic_state *state,
> struct intel_shared_dpll_state *shared_dpll;
>
> shared_dpll = intel_atomic_get_shared_dpll_state(&state->base);
> - shared_dpll[pll->info->id].crtc_mask &= ~(1 << crtc->pipe);
> + shared_dpll[pll->info->id].pipe_mask &= ~BIT(crtc->pipe);
> }
>
> static void intel_put_dpll(struct intel_atomic_state *state, @@ -4597,19
> +4599,19 @@ static void readout_dpll_hw_state(struct drm_i915_private
> *i915,
>
> POWER_DOMAIN_DPLL_DC_OFF);
> }
>
> - pll->state.crtc_mask = 0;
> + pll->state.pipe_mask = 0;
> for_each_intel_crtc(&i915->drm, crtc) {
> struct intel_crtc_state *crtc_state =
> to_intel_crtc_state(crtc->base.state);
>
> if (crtc_state->hw.active && crtc_state->shared_dpll == pll)
> - pll->state.crtc_mask |= 1 << crtc->pipe;
> + pll->state.pipe_mask |= BIT(crtc->pipe);
> }
> - pll->active_mask = pll->state.crtc_mask;
> + pll->active_mask = pll->state.pipe_mask;
>
> drm_dbg_kms(&i915->drm,
> - "%s hw state readout: crtc_mask 0x%08x, on %i\n",
> - pll->info->name, pll->state.crtc_mask, pll->on);
> + "%s hw state readout: pipe_mask 0x%x, on %i\n",
> + pll->info->name, pll->state.pipe_mask, pll->on);
> }
>
> void intel_dpll_readout_hw_state(struct drm_i915_private *i915) diff --git
> a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index 2eb7618ef957..eb52e85022e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -241,9 +241,9 @@ struct intel_dpll_hw_state {
> */
> struct intel_shared_dpll_state {
> /**
> - * @crtc_mask: mask of CRTC using this DPLL, active or not
> + * @pipe_mask: mask of pipes using this DPLL, active or not
> */
> - unsigned crtc_mask;
> + u8 pipe_mask;
>
> /**
> * @hw_state: hardware configuration for the DPLL stored in @@ -
> 351,9 +351,9 @@ struct intel_shared_dpll {
> struct intel_shared_dpll_state state;
>
> /**
> - * @active_mask: mask of active CRTCs (i.e. DPMS on) using this DPLL
> + * @active_mask: mask of active pipes (i.e. DPMS on) using this DPLL
> */
> - unsigned active_mask;
> + u8 active_mask;
>
> /**
> * @on: is the PLL actually active? Disabled during modeset
> --
> 2.26.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-03-04 10:52 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-24 14:42 [Intel-gfx] [PATCH 0/6] drm/i915: Move DDI clock readout to encoder->get_config() Ville Syrjala
2021-02-24 14:42 ` [Intel-gfx] [PATCH 1/6] drm/i915: Call primary encoder's .get_config() from MST .get_config() Ville Syrjala
2021-03-04 10:42 ` Kahola, Mika
2021-02-24 14:42 ` [Intel-gfx] [PATCH 2/6] drm/i915: Do intel_dpll_readout_hw_state() after encoder readout Ville Syrjala
2021-02-25 16:12 ` [Intel-gfx] [PATCH v2 " Ville Syrjala
2021-03-08 11:43 ` Kahola, Mika
2021-03-04 10:43 ` [Intel-gfx] [PATCH " Kahola, Mika
2021-02-24 14:42 ` [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL state tracking Ville Syrjala
2021-03-04 10:52 ` Kahola, Mika [this message]
2021-02-24 14:42 ` [Intel-gfx] [PATCH 4/6] drm/i915: Move DDI clock readout to encoder->get_config() Ville Syrjala
2021-03-08 13:11 ` Kahola, Mika
2021-02-24 14:42 ` [Intel-gfx] [PATCH 5/6] drm/i915: Add encoder->is_clock_enabled() Ville Syrjala
2021-03-08 13:16 ` Kahola, Mika
2021-02-24 14:42 ` [Intel-gfx] [PATCH 6/6] drm/i915: Extend icl_sanitize_encoder_pll_mapping() to all DDI platforms Ville Syrjala
2021-03-08 13:17 ` Kahola, Mika
2021-02-24 18:59 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Move DDI clock readout to encoder->get_config() Patchwork
2021-02-24 19:28 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-02-25 16:37 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Move DDI clock readout to encoder->get_config() (rev2) Patchwork
2021-02-25 17:04 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-02-25 18:24 ` [Intel-gfx] ✓ Fi.CI.IGT: " 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=e288be0df88240eeb3855a2ce8073dc7@intel.com \
--to=mika.kahola@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.