[PATCH 0/4] Prepare dpll for async.

[PATCH 0/4] Prepare dpll for async.

[Maarten Lankhorst]

With the conversion of the driver to support async updates dpll state
can no longer be updated in place.

dpll->config still contains the committed state, while concurrent access
is protected by dpll->mutex.

Maarten Lankhorst (4):
  drm/i915: Use a crtc mask instead of a refcount for dpll functions.
  drm/i915: Perform dpll commit first.
  drm/i915: Move pll power state to crtc power domains.
  drm/i915: Add locking to pll updates.

 drivers/gpu/drm/i915/i915_debugfs.c  |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |   5 +-
 drivers/gpu/drm/i915/intel_ddi.c     |   4 ++
 drivers/gpu/drm/i915/intel_display.c | 110 ++++++++++++++++++++---------------
 4 files changed, 70 insertions(+), 51 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Use a crtc mask instead of a refcount for dpll functions.

[Maarten Lankhorst]

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 68 ++++++++++++++++++++----------------
 3 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c6e64e2c951e..200e45922587 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3211,7 +3211,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 
 		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));
+			   pll->config.crtc_mask, hweight32(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",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10480939159c..c7401b50818c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -392,7 +392,7 @@ struct intel_shared_dpll_config {
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
 
-	int active; /* count of number of active CRTCs (i.e. DPMS on) */
+	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */
 	const char *name;
 	/* should match the index in the dev_priv->shared_dplls array */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a5d61084ae98..5dd59cae9f06 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1866,7 +1866,7 @@ static 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);
@@ -1888,6 +1888,7 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
 
 	if (WARN_ON(pll == NULL))
 		return;
@@ -1895,11 +1896,16 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll->config.crtc_mask == 0))
 		return;
 
+	if (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->name, hweight32(pll->active_mask), pll->on,
 		      crtc->base.base.id);
 
-	if (pll->active++) {
+	pll->active_mask |= crtc_mask;
+
+	if (pll->active_mask != crtc_mask) {
 		WARN_ON(!pll->on);
 		assert_shared_dpll_enabled(dev_priv, pll);
 		return;
@@ -1918,30 +1924,33 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
 
 	/* PCH only available on ILK+ */
-	if (INTEL_INFO(dev)->gen < 5)
+	if (INTEL_INFO(dev_priv)->gen < 5)
 		return;
 
 	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,
+	if (WARN_ON(!(pll->active_mask & crtc_mask)))
+		return;
+
+	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)) {
+	pll->active_mask &= ~crtc_mask;
+	if (pll->active_mask) {
 		assert_shared_dpll_disabled(dev_priv, pll);
 		return;
 	}
 
 	assert_shared_dpll_enabled(dev_priv, pll);
 	WARN_ON(!pll->on);
-	if (--pll->active)
-		return;
 
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->disable(dev_priv, pll);
@@ -4294,10 +4303,10 @@ struct intel_shared_dpll *intel_get_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 0x%08x)\n",
 				      crtc->base.base.id, pll->name,
 				      shared_dpll[i].crtc_mask,
-				      pll->active);
+				      pll->active_mask);
 			goto found;
 		}
 	}
@@ -12952,12 +12961,12 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		active = pll->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,
+		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);
+		I915_STATE_WARN(pll->active_mask && !pll->on,
 		     "pll in active use but not on in sw tracking\n");
-		I915_STATE_WARN(pll->on && !pll->active,
+		I915_STATE_WARN(pll->on && !pll->active_mask,
 		     "pll in on but not on in use in sw tracking\n");
 		I915_STATE_WARN(pll->on != active,
 		     "pll on state mismatch (expected %i, found %i)\n",
@@ -12965,16 +12974,16 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		for_each_intel_crtc(dev, crtc) {
 			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
-				enabled_crtcs++;
+				enabled_crtcs |= 1 << drm_crtc_index(&crtc->base);
 			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
-				active_crtcs++;
+				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,
 				       sizeof(dpll_hw_state)),
@@ -15760,14 +15769,13 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		pll->on = pll->get_hw_state(dev_priv, pll,
 					    &pll->config.hw_state);
-		pll->active = 0;
+		pll->active_mask = 0;
 		pll->config.crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
-				pll->active++;
+			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == 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",
 			      pll->name, pll->config.crtc_mask, pll->on);
@@ -15889,7 +15897,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);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Perform dpll commit first.

[Maarten Lankhorst]

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5dd59cae9f06..6e3b8a1f7dd3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1893,7 +1893,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	if (WARN_ON(pll->config.crtc_mask == 0))
+	if (WARN_ON(!(pll->config.crtc_mask & (1 << drm_crtc_index(&crtc->base)))))
 		return;
 
 	if (WARN_ON(pll->active_mask & crtc_mask))
@@ -1933,9 +1933,6 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
-		return;
-
 	if (WARN_ON(!(pll->active_mask & crtc_mask)))
 		return;
 
@@ -13521,7 +13518,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,
@@ -13573,8 +13571,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);
 
 		if (dev_priv->display.modeset_commit_cdclk &&
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Move pll power state to crtc power domains.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e3b8a1f7dd3..6f2dd3192bac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1912,8 +1912,6 @@ static 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->enable(dev_priv, pll);
 	pll->on = true;
@@ -1952,8 +1950,6 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->disable(dev_priv, pll);
 	pll->on = false;
-
-	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
 
 static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
@@ -5347,6 +5343,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 != DPLL_ID_PRIVATE)
+		mask |= BIT(POWER_DOMAIN_PLLS);
+
 	return mask;
 }
 
@@ -15775,9 +15774,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);
 	}
 
 	for_each_intel_encoder(dev, encoder) {
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Add locking to pll updates.

[Maarten Lankhorst]

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++++
 drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++----------
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c7401b50818c..e20dae7b1fa9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -391,9 +391,10 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
+	struct mutex lock;
 
 	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
-	bool on; /* is the PLL actually active? Disabled during modeset */
+	bool on, prepared; /* is the PLL actually active? Disabled during modeset */
 	const char *name;
 	/* should match the index in the dev_priv->shared_dplls array */
 	enum intel_dpll_id id;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 21a9b83f3bfc..e23be6e1b846 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2514,6 +2514,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
 	dev_priv->num_shared_dpll = 3;
 
 	for (i = 0; i < 2; i++) {
+		mutex_init(&dev_priv->shared_dplls[i].lock);
 		dev_priv->shared_dplls[i].id = i;
 		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
 		dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable;
@@ -2523,6 +2524,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
 	}
 
 	/* SPLL is special, but needs to be initialized anyway.. */
+	mutex_init(&dev_priv->shared_dplls[i].lock);
 	dev_priv->shared_dplls[i].id = i;
 	dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
 	dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable;
@@ -2650,6 +2652,7 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
 	dev_priv->num_shared_dpll = 3;
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		mutex_init(&dev_priv->shared_dplls[i].lock);
 		dev_priv->shared_dplls[i].id = i;
 		dev_priv->shared_dplls[i].name = skl_ddi_pll_names[i];
 		dev_priv->shared_dplls[i].disable = skl_ddi_pll_disable;
@@ -2978,6 +2981,7 @@ static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv)
 	dev_priv->num_shared_dpll = 3;
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		mutex_init(&dev_priv->shared_dplls[i].lock);
 		dev_priv->shared_dplls[i].id = i;
 		dev_priv->shared_dplls[i].name = bxt_ddi_pll_names[i];
 		dev_priv->shared_dplls[i].disable = bxt_ddi_pll_disable;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6f2dd3192bac..6b17b77106d2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1865,14 +1865,18 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	WARN_ON(!pll->config.crtc_mask);
-	if (pll->active_mask == 0) {
+	mutex_lock(&pll->lock);
+	if (pll->active_mask == 0 && !pll->prepared) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
+
+		pll->prepared = true;
 		WARN_ON(pll->on);
+
 		assert_shared_dpll_disabled(dev_priv, pll);
 
 		pll->mode_set(dev_priv, pll);
 	}
+	mutex_unlock(&pll->lock);
 }
 
 /**
@@ -1903,18 +1907,22 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 		      pll->name, hweight32(pll->active_mask), pll->on,
 		      crtc->base.base.id);
 
+	mutex_lock(&pll->lock);
 	pll->active_mask |= crtc_mask;
 
 	if (pll->active_mask != crtc_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->enable(dev_priv, pll);
 	pll->on = true;
+
+out:
+	mutex_unlock(&pll->lock);
 }
 
 static void intel_disable_shared_dpll(struct intel_crtc *crtc)
@@ -1938,18 +1946,21 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 		      pll->name, pll->active_mask, pll->on,
 		      crtc->base.base.id);
 
+	mutex_lock(&pll->lock);
 	pll->active_mask &= ~crtc_mask;
-	if (pll->active_mask) {
-		assert_shared_dpll_disabled(dev_priv, pll);
-		return;
-	}
-
 	assert_shared_dpll_enabled(dev_priv, pll);
+
+	if (pll->active_mask)
+		goto out;
+
 	WARN_ON(!pll->on);
 
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->disable(dev_priv, pll);
-	pll->on = false;
+	pll->on = pll->prepared = false;
+
+out:
+	mutex_unlock(&pll->lock);
 }
 
 static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
@@ -13772,6 +13783,8 @@ static void ibx_pch_dpll_init(struct drm_device *dev)
 	dev_priv->num_shared_dpll = 2;
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		mutex_init(&dev_priv->shared_dplls[i].lock);
+
 		dev_priv->shared_dplls[i].id = i;
 		dev_priv->shared_dplls[i].name = ibx_pch_dpll_names[i];
 		dev_priv->shared_dplls[i].mode_set = ibx_pch_dpll_mode_set;
@@ -15764,6 +15777,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		pll->on = pll->get_hw_state(dev_priv, pll,
 					    &pll->config.hw_state);
+		pll->prepared = pll->on;
 		pll->active_mask = 0;
 		pll->config.crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
@@ -15895,7 +15909,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 		DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll->name);
 
 		pll->disable(dev_priv, pll);
-		pll->on = false;
+		pll->prepared = pll->on = false;
 	}
 
 	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Prepare dpll for async.

[Patchwork]

== Series Details ==

Series: Prepare dpll for async.
URL   : https://patchwork.freedesktop.org/series/3914/
State : failure

== Summary ==

Series 3914v1 Prepare dpll for async.
http://patchwork.freedesktop.org/api/1.0/series/3914/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                fail       -> DMESG-FAIL (hsw-gt2)
                skip       -> FAIL       (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (skl-i7k-2) UNSTABLE
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (skl-i7k-2) UNSTABLE
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (byt-nuc)
Test prime_self_import:
        Subgroup basic-with_fd_dup:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:166  pass:155  dwarn:0   dfail:0   fail:0   skip:11 
bsw-nuc-2        total:169  pass:138  dwarn:0   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:143  dwarn:1   dfail:0   fail:0   skip:25 
hsw-brixbox      total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14 
hsw-gt2          total:169  pass:158  dwarn:0   dfail:1   fail:0   skip:10 
ilk-hp8440p      total:169  pass:119  dwarn:0   dfail:1   fail:0   skip:49 
ivb-t430s        total:169  pass:154  dwarn:0   dfail:0   fail:1   skip:14 
skl-i7k-2        total:169  pass:151  dwarn:2   dfail:0   fail:0   skip:16 
snb-dellxps      total:169  pass:146  dwarn:0   dfail:0   fail:1   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1493/

59c2aa9790ada24e1c13cd582a91fea33dc75b00 drm-intel-nightly: 2016y-02m-29d-09h-14m-18s UTC integration manifest
e66c69c50d9751261f98daea58b69c4f8fd245c2 drm/i915: Add locking to pll updates.
5254b77fcd77151118bcd256b8d80e02992050f9 drm/i915: Move pll power state to crtc power domains.
277f6d02b131b90cd59ad54b77e64a4596c9cb08 drm/i915: Perform dpll commit first.
5d0fea454f32d37f404bc32763626c1645a73479 drm/i915: Use a crtc mask instead of a refcount for dpll functions.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Use a crtc mask instead of a refcount for dpll functions.

[R, Durgadoss]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Maarten Lankhorst
>Sent: Monday, February 29, 2016 6:22 PM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Use a crtc mask instead of a refcount for dpll functions.
>
>This makes it easier to verify correct dpll setup with only a single crtc.
>It it also useful to detect double dpll enable/disable.

s/it/is

Actually, a welcome change ;-)

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
> drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> drivers/gpu/drm/i915/intel_display.c | 68 ++++++++++++++++++++----------------
> 3 files changed, 40 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index c6e64e2c951e..200e45922587 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -3211,7 +3211,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>
> 		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));
>+			   pll->config.crtc_mask, hweight32(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",
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 10480939159c..c7401b50818c 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -392,7 +392,7 @@ struct intel_shared_dpll_config {
> struct intel_shared_dpll {
> 	struct intel_shared_dpll_config config;
>
>-	int active; /* count of number of active CRTCs (i.e. DPMS on) */
>+	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
> 	bool on; /* is the PLL actually active? Disabled during modeset */
> 	const char *name;
> 	/* should match the index in the dev_priv->shared_dplls array */
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index a5d61084ae98..5dd59cae9f06 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -1866,7 +1866,7 @@ static 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);
>@@ -1888,6 +1888,7 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
>+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
>
> 	if (WARN_ON(pll == NULL))
> 		return;
>@@ -1895,11 +1896,16 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> 	if (WARN_ON(pll->config.crtc_mask == 0))
> 		return;
>
>+	if (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->name, hweight32(pll->active_mask), pll->on,
> 		      crtc->base.base.id);
>
>-	if (pll->active++) {
>+	pll->active_mask |= crtc_mask;
>+
>+	if (pll->active_mask != crtc_mask) {
> 		WARN_ON(!pll->on);
> 		assert_shared_dpll_enabled(dev_priv, pll);
> 		return;
>@@ -1918,30 +1924,33 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
>+	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
>
> 	/* PCH only available on ILK+ */
>-	if (INTEL_INFO(dev)->gen < 5)
>+	if (INTEL_INFO(dev_priv)->gen < 5)
> 		return;
>
> 	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,
>+	if (WARN_ON(!(pll->active_mask & crtc_mask)))
>+		return;
>+
>+	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)) {
>+	pll->active_mask &= ~crtc_mask;
>+	if (pll->active_mask) {
> 		assert_shared_dpll_disabled(dev_priv, pll);
> 		return;
> 	}
>
> 	assert_shared_dpll_enabled(dev_priv, pll);
> 	WARN_ON(!pll->on);
>-	if (--pll->active)
>-		return;
>
> 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> 	pll->disable(dev_priv, pll);
>@@ -4294,10 +4303,10 @@ struct intel_shared_dpll *intel_get_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
>0x%08x)\n",
> 				      crtc->base.base.id, pll->name,
> 				      shared_dpll[i].crtc_mask,
>-				      pll->active);
>+				      pll->active_mask);
> 			goto found;
> 		}
> 	}
>@@ -12952,12 +12961,12 @@ check_shared_dpll_state(struct drm_device *dev)
>
> 		active = pll->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,
>+		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);
>+		I915_STATE_WARN(pll->active_mask && !pll->on,
> 		     "pll in active use but not on in sw tracking\n");
>-		I915_STATE_WARN(pll->on && !pll->active,
>+		I915_STATE_WARN(pll->on && !pll->active_mask,
> 		     "pll in on but not on in use in sw tracking\n");
> 		I915_STATE_WARN(pll->on != active,
> 		     "pll on state mismatch (expected %i, found %i)\n",
>@@ -12965,16 +12974,16 @@ check_shared_dpll_state(struct drm_device *dev)
>
> 		for_each_intel_crtc(dev, crtc) {
> 			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
>-				enabled_crtcs++;
>+				enabled_crtcs |= 1 << drm_crtc_index(&crtc->base);
> 			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
>-				active_crtcs++;
>+				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,
> 				       sizeof(dpll_hw_state)),
>@@ -15760,14 +15769,13 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> 		pll->on = pll->get_hw_state(dev_priv, pll,
> 					    &pll->config.hw_state);
>-		pll->active = 0;
>+		pll->active_mask = 0;
> 		pll->config.crtc_mask = 0;
> 		for_each_intel_crtc(dev, crtc) {
>-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
>-				pll->active++;
>+			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == 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",
> 			      pll->name, pll->config.crtc_mask, pll->on);
>@@ -15889,7 +15897,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);
>--
>2.1.0
>
>_______________________________________________
>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

Re: [PATCH 2/4] drm/i915: Perform dpll commit first.

[R, Durgadoss]

Hi Maarten,

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Maarten Lankhorst
>Sent: Monday, February 29, 2016 6:22 PM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH 2/4] drm/i915: Perform dpll commit first.
>
>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.
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_display.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 5dd59cae9f06..6e3b8a1f7dd3 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -1893,7 +1893,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> 	if (WARN_ON(pll == NULL))
> 		return;
>
>-	if (WARN_ON(pll->config.crtc_mask == 0))
>+	if (WARN_ON(!(pll->config.crtc_mask & (1 << drm_crtc_index(&crtc->base)))))

Since we have crtc_mask initialized from patch 1, Why not use the same after the '&' ?

Thanks,
Durga

> 		return;
>
> 	if (WARN_ON(pll->active_mask & crtc_mask))
>@@ -1933,9 +1933,6 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
> 	if (pll == NULL)
> 		return;
>
>-	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
>-		return;
>-
> 	if (WARN_ON(!(pll->active_mask & crtc_mask)))
> 		return;
>
>@@ -13521,7 +13518,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,
>@@ -13573,8 +13571,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);
>
> 		if (dev_priv->display.modeset_commit_cdclk &&
>--
>2.1.0
>
>_______________________________________________
>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

Re: [PATCH 3/4] drm/i915: Move pll power state to crtc power domains.

[R, Durgadoss]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Maarten Lankhorst
>Sent: Monday, February 29, 2016 6:22 PM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH 3/4] drm/i915: Move pll power state to crtc power domains.
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_display.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 6e3b8a1f7dd3..6f2dd3192bac 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -1912,8 +1912,6 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> 	}
> 	WARN_ON(pll->on);
>
>-	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>-

Looking from upfront link train perspective, my initial thought was that
direct users of this _enable/disable_shared_dpll may have issues..
But then realized, before we get to _dpll functions, we do a get on
Port power domains, so, hopefully, that should keep us going.

So,
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

> 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
> 	pll->enable(dev_priv, pll);
> 	pll->on = true;
>@@ -1952,8 +1950,6 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
> 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> 	pll->disable(dev_priv, pll);
> 	pll->on = false;
>-
>-	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> }
>
> static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
>@@ -5347,6 +5343,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 != DPLL_ID_PRIVATE)
>+		mask |= BIT(POWER_DOMAIN_PLLS);
>+
> 	return mask;
> }
>
>@@ -15775,9 +15774,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);
> 	}
>
> 	for_each_intel_encoder(dev, encoder) {
>--
>2.1.0
>
>_______________________________________________
>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

Re: [PATCH 4/4] drm/i915: Add locking to pll updates.

[R, Durgadoss]

Hi Maarten,

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Maarten Lankhorst
>Sent: Monday, February 29, 2016 6:22 PM
>To: intel-gfx@lists.freedesktop.org
>Subject: [Intel-gfx] [PATCH 4/4] drm/i915: Add locking to pll updates.
>
>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.
>
>Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>---
> drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
> drivers/gpu/drm/i915/intel_ddi.c     |  4 ++++
> drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++----------
> 3 files changed, 30 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index c7401b50818c..e20dae7b1fa9 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -391,9 +391,10 @@ struct intel_shared_dpll_config {
>
> struct intel_shared_dpll {
> 	struct intel_shared_dpll_config config;
>+	struct mutex lock;
>
> 	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
>-	bool on; /* is the PLL actually active? Disabled during modeset */
>+	bool on, prepared; /* is the PLL actually active? Disabled during modeset */
> 	const char *name;
> 	/* should match the index in the dev_priv->shared_dplls array */
> 	enum intel_dpll_id id;
>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>index 21a9b83f3bfc..e23be6e1b846 100644
>--- a/drivers/gpu/drm/i915/intel_ddi.c
>+++ b/drivers/gpu/drm/i915/intel_ddi.c
>@@ -2514,6 +2514,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
> 	dev_priv->num_shared_dpll = 3;
>
> 	for (i = 0; i < 2; i++) {
>+		mutex_init(&dev_priv->shared_dplls[i].lock);
> 		dev_priv->shared_dplls[i].id = i;
> 		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> 		dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable;
>@@ -2523,6 +2524,7 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
> 	}
>
> 	/* SPLL is special, but needs to be initialized anyway.. */
>+	mutex_init(&dev_priv->shared_dplls[i].lock);
> 	dev_priv->shared_dplls[i].id = i;
> 	dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> 	dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable;
>@@ -2650,6 +2652,7 @@ static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
> 	dev_priv->num_shared_dpll = 3;
>
> 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>+		mutex_init(&dev_priv->shared_dplls[i].lock);
> 		dev_priv->shared_dplls[i].id = i;
> 		dev_priv->shared_dplls[i].name = skl_ddi_pll_names[i];
> 		dev_priv->shared_dplls[i].disable = skl_ddi_pll_disable;
>@@ -2978,6 +2981,7 @@ static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv)
> 	dev_priv->num_shared_dpll = 3;
>
> 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>+		mutex_init(&dev_priv->shared_dplls[i].lock);
> 		dev_priv->shared_dplls[i].id = i;
> 		dev_priv->shared_dplls[i].name = bxt_ddi_pll_names[i];
> 		dev_priv->shared_dplls[i].disable = bxt_ddi_pll_disable;
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 6f2dd3192bac..6b17b77106d2 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -1865,14 +1865,18 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> 	if (WARN_ON(pll == NULL))
> 		return;
>
>-	WARN_ON(!pll->config.crtc_mask);
>-	if (pll->active_mask == 0) {
>+	mutex_lock(&pll->lock);
>+	if (pll->active_mask == 0 && !pll->prepared) {
> 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>+
>+		pll->prepared = true;
> 		WARN_ON(pll->on);
>+
> 		assert_shared_dpll_disabled(dev_priv, pll);
>
> 		pll->mode_set(dev_priv, pll);
> 	}
>+	mutex_unlock(&pll->lock);
> }
>
> /**
>@@ -1903,18 +1907,22 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
> 		      pll->name, hweight32(pll->active_mask), pll->on,

Don't we need the lock for this access of 'active_mask' and the WARN_ON() before also ?

Thanks,
Durga

> 		      crtc->base.base.id);
>
>+	mutex_lock(&pll->lock);
> 	pll->active_mask |= crtc_mask;
>
> 	if (pll->active_mask != crtc_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->enable(dev_priv, pll);
> 	pll->on = true;
>+
>+out:
>+	mutex_unlock(&pll->lock);
> }
>
> static void intel_disable_shared_dpll(struct intel_crtc *crtc)
>@@ -1938,18 +1946,21 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
> 		      pll->name, pll->active_mask, pll->on,
> 		      crtc->base.base.id);
>
>+	mutex_lock(&pll->lock);
> 	pll->active_mask &= ~crtc_mask;
>-	if (pll->active_mask) {
>-		assert_shared_dpll_disabled(dev_priv, pll);
>-		return;
>-	}
>-
> 	assert_shared_dpll_enabled(dev_priv, pll);
>+
>+	if (pll->active_mask)
>+		goto out;
>+
> 	WARN_ON(!pll->on);
>
> 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> 	pll->disable(dev_priv, pll);
>-	pll->on = false;
>+	pll->on = pll->prepared = false;
>+
>+out:
>+	mutex_unlock(&pll->lock);
> }
>
> static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
>@@ -13772,6 +13783,8 @@ static void ibx_pch_dpll_init(struct drm_device *dev)
> 	dev_priv->num_shared_dpll = 2;
>
> 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>+		mutex_init(&dev_priv->shared_dplls[i].lock);
>+
> 		dev_priv->shared_dplls[i].id = i;
> 		dev_priv->shared_dplls[i].name = ibx_pch_dpll_names[i];
> 		dev_priv->shared_dplls[i].mode_set = ibx_pch_dpll_mode_set;
>@@ -15764,6 +15777,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> 		pll->on = pll->get_hw_state(dev_priv, pll,
> 					    &pll->config.hw_state);
>+		pll->prepared = pll->on;
> 		pll->active_mask = 0;
> 		pll->config.crtc_mask = 0;
> 		for_each_intel_crtc(dev, crtc) {
>@@ -15895,7 +15909,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> 		DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll->name);
>
> 		pll->disable(dev_priv, pll);
>-		pll->on = false;
>+		pll->prepared = pll->on = false;
> 	}
>
> 	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>--
>2.1.0
>
>_______________________________________________
>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

Re: [PATCH 1/4] drm/i915: Use a crtc mask instead of a refcount for dpll functions.

[Ander Conselvan De Oliveira]

On Mon, 2016-02-29 at 13:52 +0100, Maarten Lankhorst wrote:
> This makes it easier to verify correct dpll setup with only a single crtc.
> It it also useful to detect double dpll enable/disable.

I have a goal of being able to get a reference for a shared pll for an encoder
without a crtc, so that the upfront link training code doesn't need to go around
the shared pll inteface. This patch will make things a bit trickier.

I haven't yet figured out what the reference mechanism needs to be. So far I
thought about going back to ref count or including an encoder mask. But that's
for later, so see comments below.

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 68 ++++++++++++++++++++---------------
> -
>  3 files changed, 40 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index c6e64e2c951e..200e45922587 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3211,7 +3211,7 @@ static int i915_shared_dplls_info(struct seq_file *m,
> void *unused)
>  
>  		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));
> +			   pll->config.crtc_mask, hweight32(pll->active_mask), 

Just print the mask as was done below and avoid hweight32?

> 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",
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 10480939159c..c7401b50818c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -392,7 +392,7 @@ struct intel_shared_dpll_config {
>  struct intel_shared_dpll {
>  	struct intel_shared_dpll_config config;
>  
> -	int active; /* count of number of active CRTCs (i.e. DPMS on) */
> +	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
>  	bool on; /* is the PLL actually active? Disabled during modeset */
>  	const char *name;
>  	/* should match the index in the dev_priv->shared_dplls array */
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a5d61084ae98..5dd59cae9f06 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1866,7 +1866,7 @@ static 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);
> @@ -1888,6 +1888,7 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
> +	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
>  
>  	if (WARN_ON(pll == NULL))
>  		return;
> @@ -1895,11 +1896,16 @@ static void intel_enable_shared_dpll(struct intel_crtc
> *crtc)
>  	if (WARN_ON(pll->config.crtc_mask == 0))
>  		return;
>  
> +	if (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->name, hweight32(pll->active_mask), pll->on,
>  		      crtc->base.base.id);
>  
> -	if (pll->active++) {
> +	pll->active_mask |= crtc_mask;
> +
> +	if (pll->active_mask != crtc_mask) {
>  		WARN_ON(!pll->on);
>  		assert_shared_dpll_enabled(dev_priv, pll);
>  		return;
> @@ -1918,30 +1924,33 @@ static 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 = intel_crtc_to_shared_dpll(crtc);
> +	unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
>  
>  	/* PCH only available on ILK+ */
> -	if (INTEL_INFO(dev)->gen < 5)
> +	if (INTEL_INFO(dev_priv)->gen < 5)
>  		return;
>  
>  	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,
> +	if (WARN_ON(!(pll->active_mask & crtc_mask)))
> +		return;
> +
> +	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)) {
> +	pll->active_mask &= ~crtc_mask;
> +	if (pll->active_mask) {
>  		assert_shared_dpll_disabled(dev_priv, pll);

Shouldn't the above be changed to assert enabled, as the pll is still active for
another crtc?

>  		return;
>  	}
>  
>  	assert_shared_dpll_enabled(dev_priv, pll);
>  	WARN_ON(!pll->on);
> -	if (--pll->active)
> -		return;
>  
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
>  	pll->disable(dev_priv, pll);
> @@ -4294,10 +4303,10 @@ struct intel_shared_dpll *intel_get_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 0x%08x)\n",
>  				      crtc->base.base.id, pll->name,
>  				      shared_dpll[i].crtc_mask,
> -				      pll->active);
> +				      pll->active_mask);
>  			goto found;
>  		}
>  	}
> @@ -12952,12 +12961,12 @@ check_shared_dpll_state(struct drm_device *dev)
>  
>  		active = pll->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,
> +		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);
> +		I915_STATE_WARN(pll->active_mask && !pll->on,
>  		     "pll in active use but not on in sw tracking\n");
> -		I915_STATE_WARN(pll->on && !pll->active,
> +		I915_STATE_WARN(pll->on && !pll->active_mask,
>  		     "pll in on but not on in use in sw tracking\n");
>  		I915_STATE_WARN(pll->on != active,
>  		     "pll on state mismatch (expected %i, found %i)\n",
> @@ -12965,16 +12974,16 @@ check_shared_dpll_state(struct drm_device *dev)
>  
>  		for_each_intel_crtc(dev, crtc) {
>  			if (crtc->base.state->enable &&
> intel_crtc_to_shared_dpll(crtc) == pll)
> -				enabled_crtcs++;
> +				enabled_crtcs |= 1 << drm_crtc_index(&crtc
> ->base);
>  			if (crtc->active && intel_crtc_to_shared_dpll(crtc)
> == pll)
> -				active_crtcs++;
> +				active_crtcs |= 1 << drm_crtc_index(&crtc
> ->base);

Maybe change the type of active_crtcs and enabled_crtcs to unsigned, as they are
being used as masks.

With the above addressed,

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


>  		}
> -		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,
>  				       sizeof(dpll_hw_state)),
> @@ -15760,14 +15769,13 @@ static void intel_modeset_readout_hw_state(struct
> drm_device *dev)
>  
>  		pll->on = pll->get_hw_state(dev_priv, pll,
>  					    &pll->config.hw_state);
> -		pll->active = 0;
> +		pll->active_mask = 0;
>  		pll->config.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc)
> == pll) {
> -				pll->active++;
> +			if (crtc->active && intel_crtc_to_shared_dpll(crtc)
> == 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",
>  			      pll->name, pll->config.crtc_mask, pll->on);
> @@ -15889,7 +15897,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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Perform dpll commit first.

[Ander Conselvan De Oliveira]

On Mon, 2016-02-29 at 13:52 +0100, Maarten Lankhorst wrote:
> 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.

Why do we want to perform dpll commit first? That should be in the commit
message.

With that and what Durga said:

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5dd59cae9f06..6e3b8a1f7dd3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1893,7 +1893,7 @@ static void intel_enable_shared_dpll(struct intel_crtc
> *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> -	if (WARN_ON(pll->config.crtc_mask == 0))
> +	if (WARN_ON(!(pll->config.crtc_mask & (1 << drm_crtc_index(&crtc
> ->base)))))
>  		return;
>  
>  	if (WARN_ON(pll->active_mask & crtc_mask))
> @@ -1933,9 +1933,6 @@ static void intel_disable_shared_dpll(struct intel_crtc
> *crtc)
>  	if (pll == NULL)
>  		return;
>  
> -	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
> -		return;
> -
>  	if (WARN_ON(!(pll->active_mask & crtc_mask)))
>  		return;
>  
> @@ -13521,7 +13518,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,
> @@ -13573,8 +13571,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);
>  
>  		if (dev_priv->display.modeset_commit_cdclk &&
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Move pll power state to crtc power domains.

[Ander Conselvan De Oliveira]

On Tue, 2016-03-01 at 17:03 +0000, R, Durgadoss wrote:
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Maarten Lankhorst
> > Sent: Monday, February 29, 2016 6:22 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 3/4] drm/i915: Move pll power state to crtc
> > power domains.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 6e3b8a1f7dd3..6f2dd3192bac 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1912,8 +1912,6 @@ static void intel_enable_shared_dpll(struct intel_crtc
> > *crtc)
> > 	}
> > 	WARN_ON(pll->on);
> > 
> > -	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> > -
> 
> Looking from upfront link train perspective, my initial thought was that
> direct users of this _enable/disable_shared_dpll may have issues..
> But then realized, before we get to _dpll functions, we do a get on
> Port power domains, so, hopefully, that should keep us going.

Seems like that should work on BXT, but that is not how the power domain API is
supposed to be used, as far as I know. You should actually get/put the power
domain when you need it, so that the code is not dependent on side effects and
is tolerant to a new platform having that specific power domain in a different
power well.

With that being said, I'm not exactly sure what is POWER_DOMAIN_PLLS is supposed
to represent. It seems commit bd2bb1b9a1c8 ("drm/i915: add POWER_DOMAIN_PLLS")
added it to fix a shared dpll state mismatch in SandyBridge, but with the
current code the check for the power domain in ibx_pch_dpll_get_hw_state() will
always return true. Later patches show some connection of that power domain with
CDCLK, but I'm failing to grasp it.

Thanks,
Ander

> 
> So,
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
> 
> Thanks,
> Durga
> 
> > 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
> > 	pll->enable(dev_priv, pll);
> > 	pll->on = true;
> > @@ -1952,8 +1950,6 @@ static void intel_disable_shared_dpll(struct
> > intel_crtc *crtc)
> > 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> > 	pll->disable(dev_priv, pll);
> > 	pll->on = false;
> > -
> > -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> > }
> > 
> > static void ironlake_enable_pch_transcoder(struct drm_i915_private
> > *dev_priv,
> > @@ -5347,6 +5343,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 != DPLL_ID_PRIVATE)
> > +		mask |= BIT(POWER_DOMAIN_PLLS);
> > +
> > 	return mask;
> > }
> > 
> > @@ -15775,9 +15774,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);
> > 	}
> > 
> > 	for_each_intel_encoder(dev, encoder) {
> > --
> > 2.1.0
> > 
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Move pll power state to crtc power domains.

[Ander Conselvan De Oliveira]

On Mon, 2016-02-29 at 13:52 +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 6e3b8a1f7dd3..6f2dd3192bac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1912,8 +1912,6 @@ static 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->enable(dev_priv, pll);
>  	pll->on = true;
> @@ -1952,8 +1950,6 @@ static void intel_disable_shared_dpll(struct intel_crtc
> *crtc)
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
>  	pll->disable(dev_priv, pll);
>  	pll->on = false;
> -
> -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
>  
>  static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
> @@ -5347,6 +5343,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 != DPLL_ID_PRIVATE)
> +		mask |= BIT(POWER_DOMAIN_PLLS);
> +
>  	return mask;
>  }
>  
> @@ -15775,9 +15774,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);
>  	}
>  
>  	for_each_intel_encoder(dev, encoder) {

This is a nice little clean up. I'm afraid I might have to undo it so that a pll
can be enabled without a crtc, but for the time being:

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx