drm/i915: Use pipes instead crtc indices in PLL state tracking

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>
Link: https://patchwork.freedesktop.org/patch/msgid/20210224144214.24803-4-ville.syrjala@linux.intel.comReviewed-by: Mika Kahola <mika.kahola@intel.com>

drivers/gpu/drm/i915/display/intel_display.c

@@ -9643,7 +9643,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));
@@ -9656,34 +9656,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,
@@ -9703,15 +9703,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);
 	}
 }
 

drivers/gpu/drm/i915/display/intel_display_debugfs.c

@@ -1098,8 +1098,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",

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_update_ref_clks(struct drm_i915_private *i915)

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