[PATCH v2 (rebased) 0/4] Prepare dpll for async.

[PATCH v2 (rebased) 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.

This is a resend because ander's patch series got a conflict which required
a rewrite of the patches.

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

 drivers/gpu/drm/i915/i915_debugfs.c   |  4 +--
 drivers/gpu/drm/i915/intel_display.c  | 54 ++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 58 +++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  4 ++-
 4 files changed, 66 insertions(+), 54 deletions(-)

-- 
2.1.0

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

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

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |  4 ++--
 drivers/gpu/drm/i915/intel_display.c  | 43 +++++++++++++++++------------------
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d67ab7130c0f..f8fd30bd75bf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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",
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5a95dcf68df1..f8f02ec201b7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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",
 			     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,
 				       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",
 			      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);
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 4b636c47e8e3..e5f67efec32d 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -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;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 1d341472f8b0..89c5ada1a315 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -115,7 +115,7 @@ struct intel_shared_dpll_funcs {
 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 */
-- 
2.1.0

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

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

[Ander Conselvan De Oliveira]

On Mon, 2016-03-14 at 09:27 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   |  4 ++--
>  drivers/gpu/drm/i915/intel_display.c  | 43 +++++++++++++++++-----------------
> -
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d67ab7130c0f..f8fd30bd75bf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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",
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5a95dcf68df1..f8f02ec201b7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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",
>  			     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,
>  				       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",
>  			      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);
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 4b636c47e8e3..e5f67efec32d 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -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;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 1d341472f8b0..89c5ada1a315 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -115,7 +115,7 @@ struct intel_shared_dpll_funcs {
>  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 */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 (rebased) 2/4] drm/i915: Perform dpll commit first, v2.

[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.

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 5 ++---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f8f02ec201b7..17d07a729cc0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 
 		if (dev_priv->display.modeset_commit_cdclk &&
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index e5f67efec32d..213862e8cd04 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -156,7 +156,7 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
+	if (WARN_ON(!(pll->active_mask & crtc_mask)))
 		return;
 
 	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
-- 
2.1.0

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

Re: [PATCH v2 (rebased) 2/4] drm/i915: Perform dpll commit first, v2.

[Ander Conselvan De Oliveira]

On Mon, 2016-03-14 at 09:27 +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.
> 
> Changes since v1:
> - Use crtc_mask (Durgadoss)
> - Rebase.

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  | 5 ++---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f8f02ec201b7..17d07a729cc0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
>  		if (dev_priv->display.modeset_commit_cdclk &&
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e5f67efec32d..213862e8cd04 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -156,7 +156,7 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	if (pll == NULL)
>  		return;
>  
> -	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
> +	if (WARN_ON(!(pll->active_mask & crtc_mask)))
>  		return;
>  
>  	DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 (rebased) 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  | 6 +++---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17d07a729cc0..dd76df70c62a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 	}
 
 	for_each_intel_encoder(dev, encoder) {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 213862e8cd04..a9084c7c3a36 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -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;
@@ -173,8 +171,6 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->funcs.disable(dev_priv, pll);
 	pll->on = false;
-
-	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
 
 static struct intel_shared_dpll *
-- 
2.1.0

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

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

[Ander Conselvan De Oliveira]

On Mon, 2016-03-14 at 09:27 +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_display.c  | 6 +++---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 4 ----
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 17d07a729cc0..dd76df70c62a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  	}
>  
>  	for_each_intel_encoder(dev, encoder) {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 213862e8cd04..a9084c7c3a36 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -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;
> @@ -173,8 +171,6 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
>  	pll->funcs.disable(dev_priv, pll);
>  	pll->on = false;
> -
> -	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
>  
>  static struct intel_shared_dpll *
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.

[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.

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

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index a9084c7c3a36..e730b2001c07 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -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;
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 89c5ada1a315..fba8cd36ce0a 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -113,6 +113,8 @@ struct intel_shared_dpll_funcs {
 };
 
 struct intel_shared_dpll {
+	struct mutex lock;
+
 	struct intel_shared_dpll_config config;
 
 	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
-- 
2.1.0

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

Re: [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.

[Ander Conselvan De Oliveira]

On Mon, 2016-03-14 at 09:27 +0100, Maarten Lankhorst wrote:
> 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.

Ander


> Changes since v1:
> - Rebased.
> - Fix locking to protect all accesses. (Durgadoss)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a9084c7c3a36..e730b2001c07 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -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;
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 89c5ada1a315..fba8cd36ce0a 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -113,6 +113,8 @@ struct intel_shared_dpll_funcs {
>  };
>  
>  struct intel_shared_dpll {
> +	struct mutex lock;
> +
>  	struct intel_shared_dpll_config config;
>  
>  	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.

[Maarten Lankhorst]

Op 16-03-16 om 17:19 schreef Ander Conselvan De Oliveira:
> On Mon, 2016-03-14 at 09:27 +0100, Maarten Lankhorst wrote:
>> 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.
>
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.

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

Re: [PATCH v2 (rebased) 4/4] drm/i915: Add locking to pll updates, v2.

[Ander Conselvan De Oliveira]

On Wed, 2016-03-16 at 18:48 +0100, Maarten Lankhorst wrote:
> Op 16-03-16 om 17:19 schreef Ander Conselvan De Oliveira:
> > On Mon, 2016-03-14 at 09:27 +0100, Maarten Lankhorst wrote:
> > > 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.
> > 
> 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.

Yeah, I think that should be enough.

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

[PATCH v3 (rebased) 4/4] drm/i915: Add locking to pll updates, v3.

[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.

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 <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 06e4773ae7f6..a66732744494 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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];
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 19bfe6743ef2..1175eebfe03b 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -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 *
@@ -1750,6 +1762,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
 
 	dev_priv->dpll_mgr = dpll_mgr;
 	dev_priv->num_shared_dpll = i;
+	mutex_init(&dev_priv->dpll_lock);
 
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 

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

Re: [PATCH v3 (rebased) 4/4] drm/i915: Add locking to pll updates, v3.

[Ander Conselvan De Oliveira]

On Wed, 2016-03-23 at 14:51 +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

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

> ---
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 06e4773ae7f6..a66732744494 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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];
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 19bfe6743ef2..1175eebfe03b 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -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 *
> @@ -1750,6 +1762,7 @@ void intel_shared_dpll_init(struct drm_device *dev)
>  
>  	dev_priv->dpll_mgr = dpll_mgr;
>  	dev_priv->num_shared_dpll = i;
> +	mutex_init(&dev_priv->dpll_lock);
>  
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  
> 
_______________________________________________
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. (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test core_auth:
        Subgroup basic-auth:
                incomplete -> PASS       (ilk-hp8440p)
Test gem_ctx_param_basic:
        Subgroup invalid-ctx-set:
                incomplete -> SKIP       (ilk-hp8440p)
Test gem_exec_basic:
        Subgroup basic-render:
                incomplete -> PASS       (ilk-hp8440p)
        Subgroup gtt-blt:
                incomplete -> SKIP       (ilk-hp8440p)
Test gem_exec_parse:
        Subgroup basic-rejected:
                pass       -> INCOMPLETE (byt-nuc)
Test gem_mmap_gtt:
        Subgroup basic-read-no-prefault:
                incomplete -> PASS       (ilk-hp8440p)
        Subgroup basic-small-copy:
                incomplete -> PASS       (ilk-hp8440p)
Test gem_ringfill:
        Subgroup basic-default-child:
                incomplete -> PASS       (ilk-hp8440p)
        Subgroup basic-default-s4:
                incomplete -> SKIP       (ilk-hp8440p)
Test kms_addfb_basic:
        Subgroup clobberred-modifier:
                incomplete -> PASS       (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> INCOMPLETE (bsw-nuc-2)
                incomplete -> PASS       (byt-nuc)
        Subgroup basic-flip-vs-wf_vblank:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> DMESG-WARN (hsw-brixbox)
        Subgroup basic-plain-flip:
                incomplete -> PASS       (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                incomplete -> PASS       (ilk-hp8440p)
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (hsw-gt2)
                pass       -> INCOMPLETE (byt-nuc)
        Subgroup nonblocking-crc-pipe-c:
                incomplete -> SKIP       (ilk-hp8440p)
        Subgroup read-crc-pipe-a:
                pass       -> INCOMPLETE (byt-nuc)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (hsw-gt2)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                incomplete -> SKIP       (ilk-hp8440p)
Test kms_sink_crc_basic:
                skip       -> INCOMPLETE (byt-nuc)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (hsw-brixbox)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:185  pass:148  dwarn:1   dfail:0   fail:0   skip:35 
byt-nuc          total:194  pass:151  dwarn:4   dfail:0   fail:1   skip:34 
hsw-brixbox      total:194  pass:171  dwarn:1   dfail:0   fail:0   skip:22 
hsw-gt2          total:88   pass:83   dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:194  pass:125  dwarn:5   dfail:0   fail:1   skip:63 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34 

Results at /archive/results/CI_IGT_test/Patchwork_1587/

10e4a34b0d08030f2287405c855f42afbc568f46 drm-intel-nightly: 2016y-03m-14d-07h-22m-53s UTC integration manifest
e6d2d7b22762ab67d6b7aeb306a404e241b94f65 drm/i915: Add locking to pll updates, v2.
5719b18d0fdf850dc972d46bc6d17b9a5f94a4e5 drm/i915: Move pll power state to crtc power domains.
b9c930663eb083354d7ce3b0e6379d38412bd006 drm/i915: Perform dpll commit first, v2.
1d8b01dcba8a50dfc6a5f25280e1251ca06e40d0 drm/i915: Use a crtc mask instead of a refcount for dpll functions, v2.

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