Re: [Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL state tracking
> -Original Message-
> From: Intel-gfx 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ä
>
> 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ä
Reviewed-by: Mika Kahola
> ---
> 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(_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(>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(>state.hw_state,
> _hw_state,
> @@ -9713,15 +9713,15 @@ verify_shared_dpll_state(struct intel_crtc *crtc,
>
>
[Intel-gfx] [PATCH 3/6] drm/i915: Use pipes instead crtc indices in PLL state tracking
From: Ville Syrjälä
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ä
---
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(_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(>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(>state.hw_state,
_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(>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",
+
