> -----Original Message-----
> From: Intel-gfx <intel-gfx-boun...@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.syrj...@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.syrj...@linux.intel.com>
Reviewed-by: Mika Kahola <mika.kah...@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
