Prepare dpll for async.

This makes it easier to verify correct dpll setup with only a single crtc. It is also useful to detect double dpll enable/disable.

Changes since v1:
- Rebase on top of Ander's dpll rework.
- Change debugfs active to a mask.
- Change enabled_crtcs and active_crtcs to unsigned.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c   |  4 ++--
 drivers/gpu/drm/i915/intel_display.c  | 43 +++++++++++++++++-----------------

(quoted diff) +0 -1

-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 35 +++++++++++++++-------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 +-
 4 files changed, 43 insertions(+), 41 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +2 -5

index d67ab7130c0f..f8fd30bd75bf 100644
@@ -3208,8 +3208,8 @@ static int i915_shared_dplls_info(struct seq_file *m,
 void *unused)
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
 		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->name, pll->id);
-		seq_printf(m, " crtc_mask: 0x%08x, active: %d, on: %s\n",
-			   pll->config.crtc_mask, pll->active, yesno(pll
->on));
+		seq_printf(m, " crtc_mask: 0x%08x, active: 0x%x, on: %s\n",
+			   pll->config.crtc_mask, pll->active_mask, yesno(pll
->on));
 		seq_printf(m, " tracked hardware state:\n");
 		seq_printf(m, " dpll:    0x%08x\n", pll
->config.hw_state.dpll);
 		seq_printf(m, " dpll_md: 0x%08x\n",

drivers/gpu/drm/i915/intel_display.c +8 -9

index 5a95dcf68df1..f8f02ec201b7 100644
@@ -12973,7 +12973,7 @@ check_shared_dpll_state(struct drm_device *dev)
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll =
 			intel_get_shared_dpll_by_id(dev_priv, i);
-		int enabled_crtcs = 0, active_crtcs = 0;
+		unsigned enabled_crtcs = 0, active_crtcs = 0;
 		bool active;
 
 		memset(&dpll_hw_state, 0, sizeof(dpll_hw_state));

@@ -12982,15 +12982,15 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		active = pll->funcs.get_hw_state(dev_priv, pll,
 &dpll_hw_state);
 
-		I915_STATE_WARN(pll->active > hweight32(pll
->config.crtc_mask),
-		     "more active pll users than references: %i vs %i\n",
-		     pll->active, hweight32(pll->config.crtc_mask));
-		I915_STATE_WARN(pll->active && !pll->on,
-		     "pll in active use but not on in sw tracking\n");
+		I915_STATE_WARN(pll->active_mask & ~pll->config.crtc_mask,
+		     "more active pll users than references: %x vs %x\n",
+		     pll->active_mask, pll->config.crtc_mask);
 
 		if (!(pll->flags & INTEL_DPLL_ALWAYS_ON)) {
-			I915_STATE_WARN(pll->on && !pll->active,
-			     "pll in on but not on in use in sw tracking\n");
+			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");
 			I915_STATE_WARN(pll->on != active,
 			     "pll on state mismatch (expected %i, found

%i)\n",

drivers/gpu/drm/i915/intel_display.c +10 -13

 			     pll->on, active);

@@ -12998,16 +12998,17 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		for_each_intel_crtc(dev, crtc) {
 			if (crtc->base.state->enable && crtc->config
->shared_dpll == pll)
-				enabled_crtcs++;
-			if (crtc->active && crtc->config->shared_dpll == pll)
-				active_crtcs++;
+				enabled_crtcs |= 1 << drm_crtc_index(&crtc
->base);
+			if (crtc->base.state->active && crtc->config
->shared_dpll == pll)
+				active_crtcs |= 1 << drm_crtc_index(&crtc
->base);
 		}
-		I915_STATE_WARN(pll->active != active_crtcs,
-		     "pll active crtcs mismatch (expected %i, found %i)\n",
-		     pll->active, active_crtcs);
-		I915_STATE_WARN(hweight32(pll->config.crtc_mask) !=
 enabled_crtcs,
-		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
-		     hweight32(pll->config.crtc_mask), enabled_crtcs);
+
+		I915_STATE_WARN(pll->active_mask != active_crtcs,
+		     "pll active crtcs mismatch (expected %x, found %x)\n",
+		     pll->active_mask, active_crtcs);
+		I915_STATE_WARN(pll->config.crtc_mask != enabled_crtcs,
+		     "pll enabled crtcs mismatch (expected %x, found %x)\n",
+		     pll->config.crtc_mask, enabled_crtcs);
 
 		I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state,

&dpll_hw_state,

drivers/gpu/drm/i915/intel_display.c +2 -4

 				       sizeof(dpll_hw_state)),

@@ -15720,14 +15721,12 @@ static void intel_modeset_readout_hw_state(struct
 drm_device *dev)
 
 		pll->on = pll->funcs.get_hw_state(dev_priv, pll,
 						  &pll->config.hw_state);
-		pll->active = 0;
 		pll->config.crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && crtc->config->shared_dpll == pll)
 {
-				pll->active++;
+			if (crtc->active && crtc->config->shared_dpll == pll)
 				pll->config.crtc_mask |= 1 << crtc->pipe;
-			}
 		}
+		pll->active_mask = pll->config.crtc_mask;
 

DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on
%i\n",

drivers/gpu/drm/i915/intel_display.c +1 -2

 			      pll->name, pll->config.crtc_mask, pll->on);

@@ -15851,7 +15850,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
-		if (!pll->on || pll->active)
+		if (!pll->on || pll->active_mask)
 			continue;
 
 		DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll
->name);

drivers/gpu/drm/i915/intel_dpll_mgr.c +19 -17

index 4b636c47e8e3..e5f67efec32d 100644
@@ -90,7 +90,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 		return;
 
 	WARN_ON(!pll->config.crtc_mask);
-	if (pll->active == 0) {
+	if (pll->active_mask == 0) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
 		assert_shared_dpll_disabled(dev_priv, pll);

@@ -112,18 +112,23 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
+	unsigned old_mask = pll->active_mask;
 
 	if (WARN_ON(pll == NULL))
 		return;
 
-	if (WARN_ON(pll->config.crtc_mask == 0))
+	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
+	    WARN_ON(pll->active_mask & crtc_mask))
 		return;
 
-	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
-		      pll->name, pll->active, pll->on,
+	pll->active_mask |= crtc_mask;
+
+	DRM_DEBUG_KMS("enable %s (active %x, on? %d) for crtc %d\n",
+		      pll->name, pll->active_mask, pll->on,
 		      crtc->base.base.id);
 
-	if (pll->active++) {
+	if (old_mask) {
 		WARN_ON(!pll->on);
 		assert_shared_dpll_enabled(dev_priv, pll);
 		return;

@@ -142,6 +147,7 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
 
 	/* PCH only available on ILK+ */
 	if (INTEL_INFO(dev)->gen < 5)

@@ -150,21 +156,18 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (WARN_ON(!(pll->config.crtc_mask & (1 << drm_crtc_index(&crtc
->base)))))
+	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
 		return;
 
-	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
-		      pll->name, pll->active, pll->on,
+	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
+		      pll->name, pll->active_mask, pll->on,
 		      crtc->base.base.id);
 
-	if (WARN_ON(pll->active == 0)) {
-		assert_shared_dpll_disabled(dev_priv, pll);
-		return;
-	}
-
 	assert_shared_dpll_enabled(dev_priv, pll);
 	WARN_ON(!pll->on);
-	if (--pll->active)
+
+	pll->active_mask &= ~crtc_mask;
+	if (pll->active_mask)
 		return;
 
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);

@@ -197,10 +200,10 @@ intel_find_shared_dpll(struct intel_crtc *crtc,
 		if (memcmp(&crtc_state->dpll_hw_state,
 			   &shared_dpll[i].hw_state,
 			   sizeof(crtc_state->dpll_hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask
 0x%08x, ative %d)\n",
+			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask
 0x%08x, active %x)\n",
 				      crtc->base.base.id, pll->name,
 				      shared_dpll[i].crtc_mask,
-				      pll->active);
+				      pll->active_mask);
 			return pll;

drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0

 		}
 	}

drivers/gpu/drm/i915/intel_dpll_mgr.h +0 -0

index 1d341472f8b0..89c5ada1a315 100644

Warn for the wrong mask in enable only. Disable will have the wrong mask now because the new state is committed before disabling the old state.

Changes since v1:
- Use crtc_mask (Durgadoss)
- Rebase.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c  | 5 ++---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -3

index f8f02ec201b7..17d07a729cc0 100644
@@ -13536,7 +13536,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 	}
 
 	drm_atomic_helper_swap_state(dev, state);
-	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
+	dev_priv->wm.config = intel_state->wm_config;
+	intel_shared_dpll_commit(state);
 
 	if (intel_state->modeset) {
 		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,

@@ -13588,8 +13589,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 	intel_modeset_update_crtc_state(state);
 
 	if (intel_state->modeset) {
-		intel_shared_dpll_commit(state);
-
 		drm_atomic_helper_update_legacy_modeset_state(state->dev,
 state);
 

drivers/gpu/drm/i915/intel_display.c +0 -0

 		if (dev_priv->display.modeset_commit_cdclk &&

drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -0

index e5f67efec32d..213862e8cd04 100644

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c  | 6 +++---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_display.c +3 -3

index 17d07a729cc0..dd76df70c62a 100644
@@ -5276,6 +5276,9 @@ static unsigned long get_crtc_power_domains(struct
 drm_crtc *crtc,
 		mask |= BIT(intel_display_port_power_domain(intel_encoder));
 	}
 
+	if (crtc_state->shared_dpll)
+		mask |= BIT(POWER_DOMAIN_PLLS);
+
 	return mask;
 }

@@ -15729,9 +15732,6 @@ static void intel_modeset_readout_hw_state(struct
 drm_device *dev)
 
 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on
 %i\n",
 			      pll->name, pll->config.crtc_mask, pll->on);
-
-		if (pll->config.crtc_mask)
-			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}

drivers/gpu/drm/i915/intel_display.c +0 -0

 	for_each_intel_encoder(dev, encoder) {

drivers/gpu/drm/i915/intel_dpll_mgr.c +0 -2

index 213862e8cd04..a9084c7c3a36 100644
@@ -135,8 +135,6 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	}
 	WARN_ON(pll->on);
 
-	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
-
 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
 	pll->funcs.enable(dev_priv, pll);
 	pll->on = true;

With async modesets this is no longer protected with connection_mutex, so ensure that each pll has its own lock. The pll configuration state is still protected; it's only the pll updates that need locking against concurrency.

Changes since v1:
- Rebased.
- Fix locking to protect all accesses. (Durgadoss)

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 25 +++++++++++++++++++------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  2 ++
 2 files changed, 21 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dpll_mgr.c +19 -6

index a9084c7c3a36..e730b2001c07 100644
@@ -89,14 +89,16 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
+	mutex_lock(&pll->lock);
 	WARN_ON(!pll->config.crtc_mask);
-	if (pll->active_mask == 0) {
+	if (!pll->active_mask) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
 		assert_shared_dpll_disabled(dev_priv, pll);
 
 		pll->funcs.mode_set(dev_priv, pll);
 	}
+	mutex_unlock(&pll->lock);
 }
 
 /**

@@ -113,14 +115,17 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
 	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
-	unsigned old_mask = pll->active_mask;
+	unsigned old_mask;
 
 	if (WARN_ON(pll == NULL))
 		return;
 
+	mutex_lock(&pll->lock);
+	old_mask = pll->active_mask;
+
 	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
 	    WARN_ON(pll->active_mask & crtc_mask))
-		return;
+		goto out;
 
 	pll->active_mask |= crtc_mask;
 

@@ -131,13 +136,16 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (old_mask) {
 		WARN_ON(!pll->on);
 		assert_shared_dpll_enabled(dev_priv, pll);
-		return;
+		goto out;
 	}
 	WARN_ON(pll->on);
 
 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
 	pll->funcs.enable(dev_priv, pll);
 	pll->on = true;
+
+out:
+	mutex_unlock(&pll->lock);
 }
 
 void intel_disable_shared_dpll(struct intel_crtc *crtc)

@@ -154,8 +162,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
+	mutex_lock(&pll->lock);
 	if (WARN_ON(!(pll->active_mask & crtc_mask)))
-		return;
+		goto out;
 
 	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
 		      pll->name, pll->active_mask, pll->on,

@@ -166,11 +175,14 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 
 	pll->active_mask &= ~crtc_mask;
 	if (pll->active_mask)
-		return;
+		goto out;
 
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->funcs.disable(dev_priv, pll);
 	pll->on = false;
+
+out:
+	mutex_unlock(&pll->lock);
 }
 
 static struct intel_shared_dpll *

@@ -1742,6 +1754,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
 	for (i = 0; dpll_info[i].id >= 0; i++) {
 		WARN_ON(i != dpll_info[i].id);
 
+		mutex_init(&dev_priv->shared_dplls[i].lock);
 		dev_priv->shared_dplls[i].id = dpll_info[i].id;
 		dev_priv->shared_dplls[i].name = dpll_info[i].name;
 		dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs;

drivers/gpu/drm/i915/intel_dpll_mgr.h +0 -0

index 89c5ada1a315..fba8cd36ce0a 100644

Quoted 4 lines

With async modesets this is no longer protected with connection_mutex, so ensure that each pll has its own lock. The pll configuration state is still protected; it's only the pll updates that need locking against concurrency.

I think I need to look at your async branch, since I'm not really sure how async will work. But locking the individual plls might fail in SKL with the current code. The register DPLL_CTRL1 controls all 4 plls, and currently it is updated with a read-modify-write in the enable hook, so we can't update two plls concurrently.

Op 16-03-16 om 17:19 schreef Ander Conselvan De Oliveira:

Quoted 5 lines · Maarten Lankhorst

Quoted 4 lines

With async modesets this is no longer protected with connection_mutex, so ensure that each pll has its own lock. The pll configuration state is still protected; it's only the pll updates that need locking against concurrency.

Quoted 9 lines

I think I need to look at your async branch, since I'm not really sure how async
will work. But locking the individual plls might fail in SKL with the current
code. The register DPLL_CTRL1 controls all 4 plls, and currently it is updated
with a read-modify-write in the enable hook, so we can't update two plls concurrently.

Would making the dpll lock global help? I don't think in practice the locks will be contended much,
it's not a performance sensitive path.

With async modesets this is no longer protected with connection_mutex, so ensure that each pll has its own lock. The pll configuration state is still protected; it's only the pll updates that need locking against concurrency.

Changes since v1:
- Rebased.
- Fix locking to protect all accesses. (Durgadoss) Changes since v2:
- Make the dpll_lock global to protect concurrent updates to the same register, for example DPLL_CTRL1 on skl. (Ander)

Signed-off-by Maarten Lankhorst <[email protected]>

---

drivers/gpu/drm/i915/i915_drv.h +7 -0

index 06e4773ae7f6..a66732744494 100644
@@ -1854,6 +1854,13 @@ struct drm_i915_private {
 	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
 	const struct intel_dpll_mgr *dpll_mgr;
 
+	/*
+	 * dpll_lock serializes intel_{prepare,enable,disable}_shared_dpll.
+	 * Must be global rather than per dpll, because on some platforms
+	 * plls share registers.
+	 */
+	struct mutex dpll_lock;
+
 	unsigned int active_crtcs;
 	unsigned int min_pixclk[I915_MAX_PIPES];
 

drivers/gpu/drm/i915/intel_dpll_mgr.c +18 -6

index 19bfe6743ef2..1175eebfe03b 100644
@@ -89,14 +89,16 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
+	mutex_lock(&dev_priv->dpll_lock);
 	WARN_ON(!pll->config.crtc_mask);
-	if (pll->active_mask == 0) {
+	if (!pll->active_mask) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
 		assert_shared_dpll_disabled(dev_priv, pll);
 
 		pll->funcs.mode_set(dev_priv, pll);
 	}
+	mutex_unlock(&dev_priv->dpll_lock);
 }
 
 /**

@@ -113,14 +115,17 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
 	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
-	unsigned old_mask = pll->active_mask;
+	unsigned old_mask;
 
 	if (WARN_ON(pll == NULL))
 		return;
 
+	mutex_lock(&dev_priv->dpll_lock);
+	old_mask = pll->active_mask;
+
 	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
 	    WARN_ON(pll->active_mask & crtc_mask))
-		return;
+		goto out;
 
 	pll->active_mask |= crtc_mask;
 

@@ -131,13 +136,16 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (old_mask) {
 		WARN_ON(!pll->on);
 		assert_shared_dpll_enabled(dev_priv, pll);
-		return;
+		goto out;
 	}
 	WARN_ON(pll->on);
 
 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
 	pll->funcs.enable(dev_priv, pll);
 	pll->on = true;
+
+out:
+	mutex_unlock(&dev_priv->dpll_lock);
 }
 
 void intel_disable_shared_dpll(struct intel_crtc *crtc)

@@ -154,8 +162,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
+	mutex_lock(&dev_priv->dpll_lock);
 	if (WARN_ON(!(pll->active_mask & crtc_mask)))
-		return;
+		goto out;
 
 	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
 		      pll->name, pll->active_mask, pll->on,

@@ -166,11 +175,14 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 
 	pll->active_mask &= ~crtc_mask;
 	if (pll->active_mask)
-		return;
+		goto out;
 
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->funcs.disable(dev_priv, pll);
 	pll->on = false;
+
+out:
+	mutex_unlock(&dev_priv->dpll_lock);
 }
 
 static struct intel_shared_dpll *