[PATCH 0/8] Stage shared dpll config

[PATCH 0/8] Stage shared dpll config

[Ander Conselvan de Oliveira]

This series changes the mode set sequence so that the clock and PLL
logic that was done in the *_crtc_mode_set() hooks is done before
disabling crtcs. This avoids having to restore the old configuration
in the case of failure, since the hardware was never touched.

Ander Conselvan de Oliveira (8):
  drm/i915: Convert shared dpll reference count to a crtc mask
  drm/i915: Move dpll crtc_mask and hw_state fields into separate struct
  drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs
  drm/i915: Covert HSW+ to choose DPLLS before disabling CRTCs
  drm/i915: Covert ILK-IVB to choose DPLLS before disabling CRTCs
  drm/i915: Covert remaining platforms to choose DPLLS before disabling
    CRTCs
  drm/i915: Remove crtc_mode_set() hook
  drm/i915: Don't store current shared DPLL in the new pipe_config

 drivers/gpu/drm/i915/i915_debugfs.c  |  15 +--
 drivers/gpu/drm/i915/i915_drv.h      |  16 +--
 drivers/gpu/drm/i915/intel_ddi.c     |   6 +-
 drivers/gpu/drm/i915/intel_display.c | 197 +++++++++++++++++++++++------------
 4 files changed, 150 insertions(+), 84 deletions(-)

-- 
1.9.1

[PATCH 1/8] drm/i915: Convert shared dpll reference count to a crtc mask

[Ander Conselvan de Oliveira]

This will be used in a follow up patch to properly release shared DPLLs
without relying on the shared_dpll field in pipe_config.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 +--
 drivers/gpu/drm/i915/i915_drv.h      |  4 +--
 drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++-------------
 3 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index da4036d..71885d8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2627,8 +2627,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, " refcount: %i, active: %i, on: %s\n", pll->refcount,
-			   pll->active, yesno(pll->on));
+		seq_printf(m, " crtc_mask: 0x%08x, active: %d, on: %s\n",
+			   pll->crtc_mask, pll->active, yesno(pll->on));
 		seq_printf(m, " tracked hardware state:\n");
 		seq_printf(m, " dpll:    0x%08x\n", pll->hw_state.dpll);
 		seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9f3d689..0db3e1c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -227,8 +227,8 @@ struct intel_dpll_hw_state {
 };
 
 struct intel_shared_dpll {
-	int refcount; /* count of number of CRTCs sharing this PLL */
-	int active; /* count of number of active CRTCs (i.e. DPMS on) */
+	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
+	int  active; /* count of number 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 7d9beaa..51c7cf8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1775,7 +1775,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	WARN_ON(!pll->refcount);
+	WARN_ON(!pll->crtc_mask);
 	if (pll->active == 0) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
@@ -1802,7 +1802,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	if (WARN_ON(pll->refcount == 0))
+	if (WARN_ON(pll->crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
@@ -1834,7 +1834,7 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 	       return;
 
-	if (WARN_ON(pll->refcount == 0))
+	if (WARN_ON(pll->crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
@@ -3834,12 +3834,13 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (pll->refcount == 0) {
-		WARN(1, "bad %s refcount\n", pll->name);
+	if (!(pll->crtc_mask & (1 << crtc->pipe))) {
+		WARN(1, "bad %s crtc mask\n", pll->name);
 		return;
 	}
 
-	if (--pll->refcount == 0) {
+	pll->crtc_mask &= ~(1 << crtc->pipe);
+	if (pll->crtc_mask == 0) {
 		WARN_ON(pll->on);
 		WARN_ON(pll->active);
 	}
@@ -3867,7 +3868,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->refcount);
+		WARN_ON(pll->crtc_mask);
 
 		goto found;
 	}
@@ -3876,14 +3877,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->refcount == 0)
+		if (pll->crtc_mask == 0)
 			continue;
 
 		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
 			   sizeof(pll->hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (refcount %d, ative %d)\n",
-				      crtc->base.base.id,
-				      pll->name, pll->refcount, pll->active);
+			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
+				      "(crtc_mask 0x%08x, active %d)\n",
+				      crtc->base.base.id, pll->name,
+				      pll->crtc_mask, pll->active);
 
 			goto found;
 		}
@@ -3892,7 +3894,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->refcount == 0) {
+		if (pll->crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -3902,14 +3904,14 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	return NULL;
 
 found:
-	if (pll->refcount == 0)
+	if (pll->crtc_mask == 0)
 		pll->hw_state = crtc->config.dpll_hw_state;
 
 	crtc->config.shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->refcount++;
+	pll->crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
@@ -10809,6 +10811,19 @@ check_crtc_state(struct drm_device *dev)
 	}
 }
 
+static int mask_to_refcount(unsigned mask)
+{
+	int refcount = 0;
+
+	while (mask) {
+		if (mask & 1)
+			refcount++;
+		mask >>= 1;
+	}
+
+	return refcount;
+}
+
 static void
 check_shared_dpll_state(struct drm_device *dev)
 {
@@ -10828,9 +10843,9 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
 
-		WARN(pll->active > pll->refcount,
+		WARN(pll->active > mask_to_refcount(pll->crtc_mask),
 		     "more active pll users than references: %i vs %i\n",
-		     pll->active, pll->refcount);
+		     pll->active, mask_to_refcount(pll->crtc_mask));
 		WARN(pll->active && !pll->on,
 		     "pll in active use but not on in sw tracking\n");
 		WARN(pll->on && !pll->active,
@@ -10848,9 +10863,9 @@ check_shared_dpll_state(struct drm_device *dev)
 		WARN(pll->active != active_crtcs,
 		     "pll active crtcs mismatch (expected %i, found %i)\n",
 		     pll->active, active_crtcs);
-		WARN(pll->refcount != enabled_crtcs,
+		WARN(mask_to_refcount(pll->crtc_mask) != enabled_crtcs,
 		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
-		     pll->refcount, enabled_crtcs);
+		     mask_to_refcount(pll->crtc_mask), enabled_crtcs);
 
 		WARN(pll->on && memcmp(&pll->hw_state, &dpll_hw_state,
 				       sizeof(dpll_hw_state)),
@@ -13275,16 +13290,18 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		pll->on = pll->get_hw_state(dev_priv, pll, &pll->hw_state);
 		pll->active = 0;
+		pll->crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
+			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
 				pll->active++;
+				pll->crtc_mask |= 1 << crtc->pipe;
+			}
 		}
-		pll->refcount = pll->active;
 
-		DRM_DEBUG_KMS("%s hw state readout: refcount %i, on %i\n",
-			      pll->name, pll->refcount, pll->on);
+		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
+			      pll->name, pll->crtc_mask, pll->on);
 
-		if (pll->refcount)
+		if (pll->crtc_mask)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-- 
1.9.1

Re: [PATCH 1/8] drm/i915: Convert shared dpll reference count to a crtc mask

[Ville Syrjälä]

On Tue, Oct 21, 2014 at 04:02:02PM +0300, Ander Conselvan de Oliveira wrote:
> This will be used in a follow up patch to properly release shared DPLLs
> without relying on the shared_dpll field in pipe_config.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  4 +--
>  drivers/gpu/drm/i915/i915_drv.h      |  4 +--
>  drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++-------------
>  3 files changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index da4036d..71885d8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2627,8 +2627,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, " refcount: %i, active: %i, on: %s\n", pll->refcount,
> -			   pll->active, yesno(pll->on));
> +		seq_printf(m, " crtc_mask: 0x%08x, active: %d, on: %s\n",
> +			   pll->crtc_mask, pll->active, yesno(pll->on));
>  		seq_printf(m, " tracked hardware state:\n");
>  		seq_printf(m, " dpll:    0x%08x\n", pll->hw_state.dpll);
>  		seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9f3d689..0db3e1c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -227,8 +227,8 @@ struct intel_dpll_hw_state {
>  };
>  
>  struct intel_shared_dpll {
> -	int refcount; /* count of number of CRTCs sharing this PLL */
> -	int active; /* count of number of active CRTCs (i.e. DPMS on) */
> +	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
> +	int  active; /* count of number of active CRTCs (i.e. DPMS on) */
           ^^
Spurious whitespace change.

>  	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 7d9beaa..51c7cf8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1775,7 +1775,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> -	WARN_ON(!pll->refcount);
> +	WARN_ON(!pll->crtc_mask);
>  	if (pll->active == 0) {
>  		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>  		WARN_ON(pll->on);
> @@ -1802,7 +1802,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> -	if (WARN_ON(pll->refcount == 0))
> +	if (WARN_ON(pll->crtc_mask == 0))
>  		return;
>  
>  	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
> @@ -1834,7 +1834,7 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  	       return;
>  
> -	if (WARN_ON(pll->refcount == 0))
> +	if (WARN_ON(pll->crtc_mask == 0))
>  		return;
>  
>  	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
> @@ -3834,12 +3834,13 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>  	if (pll == NULL)
>  		return;
>  
> -	if (pll->refcount == 0) {
> -		WARN(1, "bad %s refcount\n", pll->name);
> +	if (!(pll->crtc_mask & (1 << crtc->pipe))) {
> +		WARN(1, "bad %s crtc mask\n", pll->name);
>  		return;
>  	}
>  
> -	if (--pll->refcount == 0) {
> +	pll->crtc_mask &= ~(1 << crtc->pipe);
> +	if (pll->crtc_mask == 0) {
>  		WARN_ON(pll->on);
>  		WARN_ON(pll->active);
>  	}
> @@ -3867,7 +3868,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			      crtc->base.base.id, pll->name);
>  
> -		WARN_ON(pll->refcount);
> +		WARN_ON(pll->crtc_mask);
>  
>  		goto found;
>  	}
> @@ -3876,14 +3877,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> -		if (pll->refcount == 0)
> +		if (pll->crtc_mask == 0)
>  			continue;
>  
>  		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
>  			   sizeof(pll->hw_state)) == 0) {
> -			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (refcount %d, ative %d)\n",
> -				      crtc->base.base.id,
> -				      pll->name, pll->refcount, pll->active);
> +			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
> +				      "(crtc_mask 0x%08x, active %d)\n",
> +				      crtc->base.base.id, pll->name,
> +				      pll->crtc_mask, pll->active);
>  
>  			goto found;
>  		}
> @@ -3892,7 +3894,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	/* Ok no matching timings, maybe there's a free one? */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		if (pll->refcount == 0) {
> +		if (pll->crtc_mask == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>  				      crtc->base.base.id, pll->name);
>  			goto found;
> @@ -3902,14 +3904,14 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	return NULL;
>  
>  found:
> -	if (pll->refcount == 0)
> +	if (pll->crtc_mask == 0)
>  		pll->hw_state = crtc->config.dpll_hw_state;
>  
>  	crtc->config.shared_dpll = i;
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	pll->refcount++;
> +	pll->crtc_mask |= 1 << crtc->pipe;
>  
>  	return pll;
>  }
> @@ -10809,6 +10811,19 @@ check_crtc_state(struct drm_device *dev)
>  	}
>  }
>  
> +static int mask_to_refcount(unsigned mask)
> +{
> +	int refcount = 0;
> +
> +	while (mask) {
> +		if (mask & 1)
> +			refcount++;
> +		mask >>= 1;
> +	}
> +
> +	return refcount;
> +}

hweight32()

> +
>  static void
>  check_shared_dpll_state(struct drm_device *dev)
>  {
> @@ -10828,9 +10843,9 @@ check_shared_dpll_state(struct drm_device *dev)
>  
>  		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
>  
> -		WARN(pll->active > pll->refcount,
> +		WARN(pll->active > mask_to_refcount(pll->crtc_mask),
>  		     "more active pll users than references: %i vs %i\n",
> -		     pll->active, pll->refcount);
> +		     pll->active, mask_to_refcount(pll->crtc_mask));
>  		WARN(pll->active && !pll->on,
>  		     "pll in active use but not on in sw tracking\n");
>  		WARN(pll->on && !pll->active,
> @@ -10848,9 +10863,9 @@ check_shared_dpll_state(struct drm_device *dev)
>  		WARN(pll->active != active_crtcs,
>  		     "pll active crtcs mismatch (expected %i, found %i)\n",
>  		     pll->active, active_crtcs);
> -		WARN(pll->refcount != enabled_crtcs,
> +		WARN(mask_to_refcount(pll->crtc_mask) != enabled_crtcs,
>  		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
> -		     pll->refcount, enabled_crtcs);
> +		     mask_to_refcount(pll->crtc_mask), enabled_crtcs);
>  
>  		WARN(pll->on && memcmp(&pll->hw_state, &dpll_hw_state,
>  				       sizeof(dpll_hw_state)),
> @@ -13275,16 +13290,18 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		pll->on = pll->get_hw_state(dev_priv, pll, &pll->hw_state);
>  		pll->active = 0;
> +		pll->crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
> +			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
>  				pll->active++;
> +				pll->crtc_mask |= 1 << crtc->pipe;
> +			}
>  		}
> -		pll->refcount = pll->active;
>  
> -		DRM_DEBUG_KMS("%s hw state readout: refcount %i, on %i\n",
> -			      pll->name, pll->refcount, pll->on);
> +		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
> +			      pll->name, pll->crtc_mask, pll->on);
>  
> -		if (pll->refcount)
> +		if (pll->crtc_mask)
>  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	}
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/8] drm/i915: Move dpll crtc_mask and hw_state fields into separate struct

[Ander Conselvan de Oliveira]

The new struct will be used in a follow up patch to allow a current and
a staged config to exist for the same shared DPLL.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 13 ++++----
 drivers/gpu/drm/i915/i915_drv.h      |  8 +++--
 drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++++++++-----------------
 4 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 71885d8..f0548d6 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2628,13 +2628,14 @@ 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->crtc_mask, pll->active, yesno(pll->on));
+			   pll->config.crtc_mask, pll->active, yesno(pll->on));
 		seq_printf(m, " tracked hardware state:\n");
-		seq_printf(m, " dpll:    0x%08x\n", pll->hw_state.dpll);
-		seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
-		seq_printf(m, " fp0:     0x%08x\n", pll->hw_state.fp0);
-		seq_printf(m, " fp1:     0x%08x\n", pll->hw_state.fp1);
-		seq_printf(m, " wrpll:   0x%08x\n", pll->hw_state.wrpll);
+		seq_printf(m, " dpll:    0x%08x\n", pll->config.hw_state.dpll);
+		seq_printf(m, " dpll_md: 0x%08x\n",
+			   pll->config.hw_state.dpll_md);
+		seq_printf(m, " fp0:     0x%08x\n", pll->config.hw_state.fp0);
+		seq_printf(m, " fp1:     0x%08x\n", pll->config.hw_state.fp1);
+		seq_printf(m, " wrpll:   0x%08x\n", pll->config.hw_state.wrpll);
 	}
 	drm_modeset_unlock_all(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0db3e1c..d7412d9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -226,14 +226,18 @@ struct intel_dpll_hw_state {
 	uint32_t wrpll;
 };
 
-struct intel_shared_dpll {
+struct intel_shared_dpll_config {
 	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
+	struct intel_dpll_hw_state hw_state;
+};
+
+struct intel_shared_dpll {
+	struct intel_shared_dpll_config config;
 	int  active; /* count of number 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 */
 	enum intel_dpll_id id;
-	struct intel_dpll_hw_state hw_state;
 	/* The mode_set hook is optional and should be used together with the
 	 * intel_prepare_shared_dpll function. */
 	void (*mode_set)(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 14df1f8..5915a07 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1333,7 +1333,7 @@ int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
 static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
-	I915_WRITE(WRPLL_CTL(pll->id), pll->hw_state.wrpll);
+	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
 	POSTING_READ(WRPLL_CTL(pll->id));
 	udelay(20);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 51c7cf8..cdaf8ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1775,7 +1775,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	WARN_ON(!pll->crtc_mask);
+	WARN_ON(!pll->config.crtc_mask);
 	if (pll->active == 0) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
@@ -1802,7 +1802,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	if (WARN_ON(pll->crtc_mask == 0))
+	if (WARN_ON(pll->config.crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
@@ -1834,7 +1834,7 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 	       return;
 
-	if (WARN_ON(pll->crtc_mask == 0))
+	if (WARN_ON(pll->config.crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
@@ -3834,13 +3834,13 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (!(pll->crtc_mask & (1 << crtc->pipe))) {
+	if (!(pll->config.crtc_mask & (1 << crtc->pipe))) {
 		WARN(1, "bad %s crtc mask\n", pll->name);
 		return;
 	}
 
-	pll->crtc_mask &= ~(1 << crtc->pipe);
-	if (pll->crtc_mask == 0) {
+	pll->config.crtc_mask &= ~(1 << crtc->pipe);
+	if (pll->config.crtc_mask == 0) {
 		WARN_ON(pll->on);
 		WARN_ON(pll->active);
 	}
@@ -3868,7 +3868,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->crtc_mask);
+		WARN_ON(pll->config.crtc_mask);
 
 		goto found;
 	}
@@ -3877,15 +3877,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->crtc_mask == 0)
+		if (pll->config.crtc_mask == 0)
 			continue;
 
-		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
-			   sizeof(pll->hw_state)) == 0) {
+		if (memcmp(&crtc->config.dpll_hw_state,
+			   &pll->config.hw_state,
+			   sizeof(pll->config.hw_state)) == 0) {
 			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
 				      "(crtc_mask 0x%08x, active %d)\n",
 				      crtc->base.base.id, pll->name,
-				      pll->crtc_mask, pll->active);
+				      pll->config.crtc_mask, pll->active);
 
 			goto found;
 		}
@@ -3894,7 +3895,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->crtc_mask == 0) {
+		if (pll->config.crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -3904,14 +3905,14 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	return NULL;
 
 found:
-	if (pll->crtc_mask == 0)
-		pll->hw_state = crtc->config.dpll_hw_state;
+	if (pll->config.crtc_mask == 0)
+		pll->config.hw_state = crtc->config.dpll_hw_state;
 
 	crtc->config.shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->crtc_mask |= 1 << crtc->pipe;
+	pll->config.crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
@@ -10843,9 +10844,9 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
 
-		WARN(pll->active > mask_to_refcount(pll->crtc_mask),
+		WARN(pll->active > mask_to_refcount(pll->config.crtc_mask),
 		     "more active pll users than references: %i vs %i\n",
-		     pll->active, mask_to_refcount(pll->crtc_mask));
+		     pll->active, mask_to_refcount(pll->config.crtc_mask));
 		WARN(pll->active && !pll->on,
 		     "pll in active use but not on in sw tracking\n");
 		WARN(pll->on && !pll->active,
@@ -10863,11 +10864,11 @@ check_shared_dpll_state(struct drm_device *dev)
 		WARN(pll->active != active_crtcs,
 		     "pll active crtcs mismatch (expected %i, found %i)\n",
 		     pll->active, active_crtcs);
-		WARN(mask_to_refcount(pll->crtc_mask) != enabled_crtcs,
+		WARN(mask_to_refcount(pll->config.crtc_mask) != enabled_crtcs,
 		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
-		     mask_to_refcount(pll->crtc_mask), enabled_crtcs);
+		     mask_to_refcount(pll->config.crtc_mask), enabled_crtcs);
 
-		WARN(pll->on && memcmp(&pll->hw_state, &dpll_hw_state,
+		WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
 				       sizeof(dpll_hw_state)),
 		     "pll hw state mismatch\n");
 	}
@@ -11542,8 +11543,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
 				  struct intel_shared_dpll *pll)
 {
-	I915_WRITE(PCH_FP0(pll->id), pll->hw_state.fp0);
-	I915_WRITE(PCH_FP1(pll->id), pll->hw_state.fp1);
+	I915_WRITE(PCH_FP0(pll->id), pll->config.hw_state.fp0);
+	I915_WRITE(PCH_FP1(pll->id), pll->config.hw_state.fp1);
 }
 
 static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
@@ -11552,7 +11553,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 	/* PCH refclock must be enabled first */
 	ibx_assert_pch_refclk_enabled(dev_priv);
 
-	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
 
 	/* Wait for the clocks to stabilize. */
 	POSTING_READ(PCH_DPLL(pll->id));
@@ -11563,7 +11564,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 	 *
 	 * So write it again.
 	 */
-	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
 	POSTING_READ(PCH_DPLL(pll->id));
 	udelay(200);
 }
@@ -13288,20 +13289,21 @@ static void intel_modeset_readout_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];
 
-		pll->on = pll->get_hw_state(dev_priv, pll, &pll->hw_state);
+		pll->on = pll->get_hw_state(dev_priv, pll,
+					    &pll->config.hw_state);
 		pll->active = 0;
-		pll->crtc_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++;
-				pll->crtc_mask |= 1 << crtc->pipe;
+				pll->config.crtc_mask |= 1 << crtc->pipe;
 			}
 		}
 
 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
-			      pll->name, pll->crtc_mask, pll->on);
+			      pll->name, pll->config.crtc_mask, pll->on);
 
-		if (pll->crtc_mask)
+		if (pll->config.crtc_mask)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-- 
1.9.1

Re: [PATCH 2/8] drm/i915: Move dpll crtc_mask and hw_state fields into separate struct

[Damien Lespiau]

On Tue, Oct 21, 2014 at 04:02:03PM +0300, Ander Conselvan de Oliveira wrote:
> The new struct will be used in a follow up patch to allow a current and
> a staged config to exist for the same shared DPLL.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

This needs to be rebased on top of Ville's comment to use hweight32() to
count the number of pipes, but otherwise:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 13 ++++----
>  drivers/gpu/drm/i915/i915_drv.h      |  8 +++--
>  drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 60 +++++++++++++++++++-----------------
>  4 files changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 71885d8..f0548d6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2628,13 +2628,14 @@ 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->crtc_mask, pll->active, yesno(pll->on));
> +			   pll->config.crtc_mask, pll->active, yesno(pll->on));
>  		seq_printf(m, " tracked hardware state:\n");
> -		seq_printf(m, " dpll:    0x%08x\n", pll->hw_state.dpll);
> -		seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
> -		seq_printf(m, " fp0:     0x%08x\n", pll->hw_state.fp0);
> -		seq_printf(m, " fp1:     0x%08x\n", pll->hw_state.fp1);
> -		seq_printf(m, " wrpll:   0x%08x\n", pll->hw_state.wrpll);
> +		seq_printf(m, " dpll:    0x%08x\n", pll->config.hw_state.dpll);
> +		seq_printf(m, " dpll_md: 0x%08x\n",
> +			   pll->config.hw_state.dpll_md);
> +		seq_printf(m, " fp0:     0x%08x\n", pll->config.hw_state.fp0);
> +		seq_printf(m, " fp1:     0x%08x\n", pll->config.hw_state.fp1);
> +		seq_printf(m, " wrpll:   0x%08x\n", pll->config.hw_state.wrpll);
>  	}
>  	drm_modeset_unlock_all(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0db3e1c..d7412d9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -226,14 +226,18 @@ struct intel_dpll_hw_state {
>  	uint32_t wrpll;
>  };
>  
> -struct intel_shared_dpll {
> +struct intel_shared_dpll_config {
>  	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
> +	struct intel_dpll_hw_state hw_state;
> +};
> +
> +struct intel_shared_dpll {
> +	struct intel_shared_dpll_config config;
>  	int  active; /* count of number 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 */
>  	enum intel_dpll_id id;
> -	struct intel_dpll_hw_state hw_state;
>  	/* The mode_set hook is optional and should be used together with the
>  	 * intel_prepare_shared_dpll function. */
>  	void (*mode_set)(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 14df1f8..5915a07 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1333,7 +1333,7 @@ int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
>  static void hsw_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(WRPLL_CTL(pll->id), pll->hw_state.wrpll);
> +	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
>  	POSTING_READ(WRPLL_CTL(pll->id));
>  	udelay(20);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 51c7cf8..cdaf8ef 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1775,7 +1775,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> -	WARN_ON(!pll->crtc_mask);
> +	WARN_ON(!pll->config.crtc_mask);
>  	if (pll->active == 0) {
>  		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>  		WARN_ON(pll->on);
> @@ -1802,7 +1802,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  		return;
>  
> -	if (WARN_ON(pll->crtc_mask == 0))
> +	if (WARN_ON(pll->config.crtc_mask == 0))
>  		return;
>  
>  	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
> @@ -1834,7 +1834,7 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	if (WARN_ON(pll == NULL))
>  	       return;
>  
> -	if (WARN_ON(pll->crtc_mask == 0))
> +	if (WARN_ON(pll->config.crtc_mask == 0))
>  		return;
>  
>  	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
> @@ -3834,13 +3834,13 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>  	if (pll == NULL)
>  		return;
>  
> -	if (!(pll->crtc_mask & (1 << crtc->pipe))) {
> +	if (!(pll->config.crtc_mask & (1 << crtc->pipe))) {
>  		WARN(1, "bad %s crtc mask\n", pll->name);
>  		return;
>  	}
>  
> -	pll->crtc_mask &= ~(1 << crtc->pipe);
> -	if (pll->crtc_mask == 0) {
> +	pll->config.crtc_mask &= ~(1 << crtc->pipe);
> +	if (pll->config.crtc_mask == 0) {
>  		WARN_ON(pll->on);
>  		WARN_ON(pll->active);
>  	}
> @@ -3868,7 +3868,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			      crtc->base.base.id, pll->name);
>  
> -		WARN_ON(pll->crtc_mask);
> +		WARN_ON(pll->config.crtc_mask);
>  
>  		goto found;
>  	}
> @@ -3877,15 +3877,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> -		if (pll->crtc_mask == 0)
> +		if (pll->config.crtc_mask == 0)
>  			continue;
>  
> -		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
> -			   sizeof(pll->hw_state)) == 0) {
> +		if (memcmp(&crtc->config.dpll_hw_state,
> +			   &pll->config.hw_state,
> +			   sizeof(pll->config.hw_state)) == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
>  				      "(crtc_mask 0x%08x, active %d)\n",
>  				      crtc->base.base.id, pll->name,
> -				      pll->crtc_mask, pll->active);
> +				      pll->config.crtc_mask, pll->active);
>  
>  			goto found;
>  		}
> @@ -3894,7 +3895,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	/* Ok no matching timings, maybe there's a free one? */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		if (pll->crtc_mask == 0) {
> +		if (pll->config.crtc_mask == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>  				      crtc->base.base.id, pll->name);
>  			goto found;
> @@ -3904,14 +3905,14 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	return NULL;
>  
>  found:
> -	if (pll->crtc_mask == 0)
> -		pll->hw_state = crtc->config.dpll_hw_state;
> +	if (pll->config.crtc_mask == 0)
> +		pll->config.hw_state = crtc->config.dpll_hw_state;
>  
>  	crtc->config.shared_dpll = i;
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	pll->crtc_mask |= 1 << crtc->pipe;
> +	pll->config.crtc_mask |= 1 << crtc->pipe;
>  
>  	return pll;
>  }
> @@ -10843,9 +10844,9 @@ check_shared_dpll_state(struct drm_device *dev)
>  
>  		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
>  
> -		WARN(pll->active > mask_to_refcount(pll->crtc_mask),
> +		WARN(pll->active > mask_to_refcount(pll->config.crtc_mask),
>  		     "more active pll users than references: %i vs %i\n",
> -		     pll->active, mask_to_refcount(pll->crtc_mask));
> +		     pll->active, mask_to_refcount(pll->config.crtc_mask));
>  		WARN(pll->active && !pll->on,
>  		     "pll in active use but not on in sw tracking\n");
>  		WARN(pll->on && !pll->active,
> @@ -10863,11 +10864,11 @@ check_shared_dpll_state(struct drm_device *dev)
>  		WARN(pll->active != active_crtcs,
>  		     "pll active crtcs mismatch (expected %i, found %i)\n",
>  		     pll->active, active_crtcs);
> -		WARN(mask_to_refcount(pll->crtc_mask) != enabled_crtcs,
> +		WARN(mask_to_refcount(pll->config.crtc_mask) != enabled_crtcs,
>  		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
> -		     mask_to_refcount(pll->crtc_mask), enabled_crtcs);
> +		     mask_to_refcount(pll->config.crtc_mask), enabled_crtcs);
>  
> -		WARN(pll->on && memcmp(&pll->hw_state, &dpll_hw_state,
> +		WARN(pll->on && memcmp(&pll->config.hw_state, &dpll_hw_state,
>  				       sizeof(dpll_hw_state)),
>  		     "pll hw state mismatch\n");
>  	}
> @@ -11542,8 +11543,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
>  				  struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(PCH_FP0(pll->id), pll->hw_state.fp0);
> -	I915_WRITE(PCH_FP1(pll->id), pll->hw_state.fp1);
> +	I915_WRITE(PCH_FP0(pll->id), pll->config.hw_state.fp0);
> +	I915_WRITE(PCH_FP1(pll->id), pll->config.hw_state.fp1);
>  }
>  
>  static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> @@ -11552,7 +11553,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	/* PCH refclock must be enabled first */
>  	ibx_assert_pch_refclk_enabled(dev_priv);
>  
> -	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
>  
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(PCH_DPLL(pll->id));
> @@ -11563,7 +11564,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	 *
>  	 * So write it again.
>  	 */
> -	I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
>  	POSTING_READ(PCH_DPLL(pll->id));
>  	udelay(200);
>  }
> @@ -13288,20 +13289,21 @@ static void intel_modeset_readout_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];
>  
> -		pll->on = pll->get_hw_state(dev_priv, pll, &pll->hw_state);
> +		pll->on = pll->get_hw_state(dev_priv, pll,
> +					    &pll->config.hw_state);
>  		pll->active = 0;
> -		pll->crtc_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++;
> -				pll->crtc_mask |= 1 << crtc->pipe;
> +				pll->config.crtc_mask |= 1 << crtc->pipe;
>  			}
>  		}
>  
>  		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
> -			      pll->name, pll->crtc_mask, pll->on);
> +			      pll->name, pll->config.crtc_mask, pll->on);
>  
> -		if (pll->crtc_mask)
> +		if (pll->config.crtc_mask)
>  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	}
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/8] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs

[Ander Conselvan de Oliveira]

It is possible for a mode set to fail if there aren't shared DPLLS that
match the new configuration requirement or other errors in clock
computation. If that step is executed after disabling crtcs, in the
failure case the hardware configuration is changed and needs to be
restored. Doing those things early will allow the mode set to fail
before actually touching the hardware.

Follow up patches will convert different platforms to use the new
infrastructure.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_ddi.c     |   2 +
 drivers/gpu/drm/i915/intel_display.c | 125 +++++++++++++++++++++++++++--------
 3 files changed, 102 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d7412d9..5b2464f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -233,6 +233,8 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
+	struct intel_shared_dpll_config *new_config;
+
 	int  active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */
 	const char *name;
@@ -480,6 +482,7 @@ struct drm_i915_display_funcs {
 				struct intel_crtc_config *);
 	void (*get_plane_config)(struct intel_crtc *,
 				 struct intel_plane_config *);
+	int (*crtc_compute_clock)(struct intel_crtc *crtc);
 	int (*crtc_mode_set)(struct intel_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 5915a07..7b8c4b8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1375,6 +1375,8 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
 	dev_priv->num_shared_dpll = 2;
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		dev_priv->shared_dplls[i].new_config =
+			&dev_priv->shared_dplls[i].config;
 		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_pll_disable;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cdaf8ef..f2f7e8f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3851,15 +3851,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
+	struct intel_shared_dpll *pll;
 	enum intel_dpll_id i;
 
-	if (pll) {
-		DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
-			      crtc->base.base.id, pll->name);
-		intel_put_shared_dpll(crtc);
-	}
-
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
 		i = (enum intel_dpll_id) crtc->pipe;
@@ -3868,7 +3862,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->config.crtc_mask);
+		WARN_ON(pll->new_config->crtc_mask);
 
 		goto found;
 	}
@@ -3877,17 +3871,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->config.crtc_mask == 0)
+		if (pll->new_config->crtc_mask == 0)
 			continue;
 
-		if (memcmp(&crtc->config.dpll_hw_state,
-			   &pll->config.hw_state,
-			   sizeof(pll->config.hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
-				      "(crtc_mask 0x%08x, active %d)\n",
+		if (memcmp(&crtc->new_config->dpll_hw_state,
+			   &pll->new_config->hw_state,
+			   sizeof(pll->new_config->hw_state)) == 0) {
+			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
 				      crtc->base.base.id, pll->name,
-				      pll->config.crtc_mask, pll->active);
-
+				      pll->new_config->crtc_mask,
+				      pll->active);
 			goto found;
 		}
 	}
@@ -3895,7 +3888,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->config.crtc_mask == 0) {
+		if (pll->new_config->crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -3905,18 +3898,64 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	return NULL;
 
 found:
-	if (pll->config.crtc_mask == 0)
-		pll->config.hw_state = crtc->config.dpll_hw_state;
+	if (pll->new_config->crtc_mask == 0)
+		pll->new_config->hw_state = crtc->new_config->dpll_hw_state;
 
-	crtc->config.shared_dpll = i;
+	crtc->new_config->shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->config.crtc_mask |= 1 << crtc->pipe;
+	pll->new_config->crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
 
+/**
+ * intel_shared_dpll_start_config - start a new PLL staged config
+ * @dev_priv: DRM device
+ * @clear_pipes: mask of pipes that will have their PLLs freed
+ *
+ * Starts a new PLL staged config, copying the current config but
+ * releasing the references of pipes specified in clear_pipes.
+ */
+static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
+					  unsigned clear_pipes)
+{
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		pll->new_config = kmalloc(sizeof(*pll->new_config),
+					  GFP_KERNEL);
+		if (!pll->new_config)
+			return -ENOMEM;
+
+		pll->new_config->crtc_mask =
+			pll->config.crtc_mask & ~clear_pipes;
+		pll->new_config->hw_state = pll->config.hw_state;
+	}
+
+	return 0;
+}
+
+static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
+{
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		BUG_ON(pll->new_config == &pll->config);
+
+		pll->config = *pll->new_config;
+		kfree(pll->new_config);
+		pll->new_config = &pll->config;
+	}
+}
+
 static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5401,11 +5440,11 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 				     struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
-		struct drm_i915_private *dev_priv = dev->dev_private;
 		int clock_limit =
 			dev_priv->display.get_display_clock_speed(dev);
 
@@ -5455,10 +5494,11 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 		hsw_compute_ips_config(crtc, pipe_config);
 
 	/*
-	 * XXX: PCH/WRPLL clock sharing is done in ->mode_set, so make sure the
-	 * old clock survives for now.
+	 * XXX: PCH/WRPLL clock sharing is done in ->mode_set if ->compute_clock is not
+	 * set, so make sure the old clock survives for now.
 	 */
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev) || HAS_DDI(dev))
+	if (dev_priv->display.crtc_compute_clock == NULL &&
+            (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev) || HAS_DDI(dev)))
 		pipe_config->shared_dpll = crtc->config.shared_dpll;
 
 	if (pipe_config->has_pch_encoder)
@@ -7328,6 +7368,9 @@ static int ironlake_crtc_mode_set(struct intel_crtc *crtc,
 		else
 			crtc->new_config->dpll_hw_state.fp1 = fp;
 
+		if (intel_crtc_to_shared_dpll(crtc))
+			intel_put_shared_dpll(crtc);
+
 		pll = intel_get_shared_dpll(crtc);
 		if (pll == NULL) {
 			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
@@ -10986,6 +11029,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		prepare_pipes &= ~disable_pipes;
 	}
 
+	if (dev_priv->display.crtc_compute_clock) {
+		unsigned clear_pipes = modeset_pipes | disable_pipes;
+
+		ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
+		if (ret)
+			goto done;
+
+		for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
+			ret = dev_priv->display.crtc_compute_clock(intel_crtc);
+			if (ret)
+				goto done;
+		}
+	}
+
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
 		intel_crtc_disable(&intel_crtc->base);
 
@@ -11013,6 +11070,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 						&pipe_config->adjusted_mode);
 	}
 
+	if (dev_priv->display.crtc_compute_clock)
+		intel_shared_dpll_commit(dev_priv);
+
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
 	intel_modeset_update_state(dev, prepare_pipes);
@@ -11047,9 +11107,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		crtc->x = x;
 		crtc->y = y;
 
-		ret = dev_priv->display.crtc_mode_set(intel_crtc, x, y, fb);
-		if (ret)
-			goto done;
+		if (dev_priv->display.crtc_mode_set) {
+			ret = dev_priv->display.crtc_mode_set(intel_crtc,
+							      x, y, fb);
+			if (ret)
+				goto done;
+		}
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -11599,6 +11662,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++) {
+		dev_priv->shared_dplls[i].new_config =
+			&dev_priv->shared_dplls[i].config;
 		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;
@@ -12586,6 +12651,7 @@ static void intel_init_display(struct drm_device *dev)
 	if (HAS_DDI(dev)) {
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
+		dev_priv->display.crtc_compute_clock = NULL;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
@@ -12599,6 +12665,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
+		dev_priv->display.crtc_compute_clock = NULL;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
@@ -12608,6 +12675,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
+		dev_priv->display.crtc_compute_clock = NULL;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -12617,6 +12685,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
+		dev_priv->display.crtc_compute_clock = NULL;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
-- 
1.9.1

Re: [PATCH 3/8] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs

[Ville Syrjälä]

On Tue, Oct 21, 2014 at 04:02:04PM +0300, Ander Conselvan de Oliveira wrote:
> It is possible for a mode set to fail if there aren't shared DPLLS that
> match the new configuration requirement or other errors in clock
> computation. If that step is executed after disabling crtcs, in the
> failure case the hardware configuration is changed and needs to be
> restored. Doing those things early will allow the mode set to fail
> before actually touching the hardware.
> 
> Follow up patches will convert different platforms to use the new
> infrastructure.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 125 +++++++++++++++++++++++++++--------
>  3 files changed, 102 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d7412d9..5b2464f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -233,6 +233,8 @@ struct intel_shared_dpll_config {
>  
>  struct intel_shared_dpll {
>  	struct intel_shared_dpll_config config;
> +	struct intel_shared_dpll_config *new_config;
> +
>  	int  active; /* count of number of active CRTCs (i.e. DPMS on) */
>  	bool on; /* is the PLL actually active? Disabled during modeset */
>  	const char *name;
> @@ -480,6 +482,7 @@ struct drm_i915_display_funcs {
>  				struct intel_crtc_config *);
>  	void (*get_plane_config)(struct intel_crtc *,
>  				 struct intel_plane_config *);
> +	int (*crtc_compute_clock)(struct intel_crtc *crtc);
>  	int (*crtc_mode_set)(struct intel_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5915a07..7b8c4b8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1375,6 +1375,8 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>  	dev_priv->num_shared_dpll = 2;
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +		dev_priv->shared_dplls[i].new_config =
> +			&dev_priv->shared_dplls[i].config;
>  		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_pll_disable;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cdaf8ef..f2f7e8f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3851,15 +3851,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
> +	struct intel_shared_dpll *pll;
>  	enum intel_dpll_id i;
>  
> -	if (pll) {
> -		DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
> -			      crtc->base.base.id, pll->name);
> -		intel_put_shared_dpll(crtc);
> -	}
> -
>  	if (HAS_PCH_IBX(dev_priv->dev)) {
>  		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>  		i = (enum intel_dpll_id) crtc->pipe;
> @@ -3868,7 +3862,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>  			      crtc->base.base.id, pll->name);
>  
> -		WARN_ON(pll->config.crtc_mask);
> +		WARN_ON(pll->new_config->crtc_mask);
>  
>  		goto found;
>  	}
> @@ -3877,17 +3871,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> -		if (pll->config.crtc_mask == 0)
> +		if (pll->new_config->crtc_mask == 0)
>  			continue;
>  
> -		if (memcmp(&crtc->config.dpll_hw_state,
> -			   &pll->config.hw_state,
> -			   sizeof(pll->config.hw_state)) == 0) {
> -			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
> -				      "(crtc_mask 0x%08x, active %d)\n",
> +		if (memcmp(&crtc->new_config->dpll_hw_state,
> +			   &pll->new_config->hw_state,
> +			   sizeof(pll->new_config->hw_state)) == 0) {
> +			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>  				      crtc->base.base.id, pll->name,
> -				      pll->config.crtc_mask, pll->active);
> -
> +				      pll->new_config->crtc_mask,
> +				      pll->active);
>  			goto found;
>  		}
>  	}
> @@ -3895,7 +3888,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	/* Ok no matching timings, maybe there's a free one? */
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		if (pll->config.crtc_mask == 0) {
> +		if (pll->new_config->crtc_mask == 0) {
>  			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>  				      crtc->base.base.id, pll->name);
>  			goto found;
> @@ -3905,18 +3898,64 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  	return NULL;
>  
>  found:
> -	if (pll->config.crtc_mask == 0)
> -		pll->config.hw_state = crtc->config.dpll_hw_state;
> +	if (pll->new_config->crtc_mask == 0)
> +		pll->new_config->hw_state = crtc->new_config->dpll_hw_state;
>  
> -	crtc->config.shared_dpll = i;
> +	crtc->new_config->shared_dpll = i;
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	pll->config.crtc_mask |= 1 << crtc->pipe;
> +	pll->new_config->crtc_mask |= 1 << crtc->pipe;
>  
>  	return pll;
>  }
>  
> +/**
> + * intel_shared_dpll_start_config - start a new PLL staged config
> + * @dev_priv: DRM device
> + * @clear_pipes: mask of pipes that will have their PLLs freed
> + *
> + * Starts a new PLL staged config, copying the current config but
> + * releasing the references of pipes specified in clear_pipes.
> + */
> +static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
> +					  unsigned clear_pipes)
> +{
> +	struct intel_shared_dpll *pll;
> +	enum intel_dpll_id i;
> +
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +		pll = &dev_priv->shared_dplls[i];
> +
> +		pll->new_config = kmalloc(sizeof(*pll->new_config),
> +					  GFP_KERNEL);
> +		if (!pll->new_config)
> +			return -ENOMEM;

Need to restore new_config = &config? For the crtc config we have
new_config==NULL outside modeset operations to catch abusers, but you
seem to have new_config==&config for the plls normally. I had the crtc
config that way too initially, but Daniel wanted to changed it. Not sure
how hard it would be to do the same thing for plls?

My original idea for the new_config==&config approach in crtcs was that
we might be able to share some functions that get called both during
and after modesets and they would automagically consult the correct
pipe config. But the NULL approach certainly seems to work so far and
it would result in a neat explosion if it didn't.

Also what happens with the already allocated pll configs if we fail
midway through the loop here? Or if we fail later in the
.compute_clock() or some other place before the pll config is commited?

Could also use kmemdup() here I suppose.

> +
> +		pll->new_config->crtc_mask =
> +			pll->config.crtc_mask & ~clear_pipes;
> +		pll->new_config->hw_state = pll->config.hw_state;
> +	}
> +
> +	return 0;
> +}
> +
> +static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_shared_dpll *pll;
> +	enum intel_dpll_id i;
> +
> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> +		pll = &dev_priv->shared_dplls[i];
> +
> +		BUG_ON(pll->new_config == &pll->config);
> +
> +		pll->config = *pll->new_config;
> +		kfree(pll->new_config);
> +		pll->new_config = &pll->config;
> +	}
> +}
> +
>  static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5401,11 +5440,11 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  				     struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>  
>  	/* FIXME should check pixel clock limits on all platforms */
>  	if (INTEL_INFO(dev)->gen < 4) {
> -		struct drm_i915_private *dev_priv = dev->dev_private;
>  		int clock_limit =
>  			dev_priv->display.get_display_clock_speed(dev);
>  
> @@ -5455,10 +5494,11 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		hsw_compute_ips_config(crtc, pipe_config);
>  
>  	/*
> -	 * XXX: PCH/WRPLL clock sharing is done in ->mode_set, so make sure the
> -	 * old clock survives for now.
> +	 * XXX: PCH/WRPLL clock sharing is done in ->mode_set if ->compute_clock is not
> +	 * set, so make sure the old clock survives for now.
>  	 */
> -	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev) || HAS_DDI(dev))
> +	if (dev_priv->display.crtc_compute_clock == NULL &&
> +            (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev) || HAS_DDI(dev)))
>  		pipe_config->shared_dpll = crtc->config.shared_dpll;
>  
>  	if (pipe_config->has_pch_encoder)
> @@ -7328,6 +7368,9 @@ static int ironlake_crtc_mode_set(struct intel_crtc *crtc,
>  		else
>  			crtc->new_config->dpll_hw_state.fp1 = fp;
>  
> +		if (intel_crtc_to_shared_dpll(crtc))
> +			intel_put_shared_dpll(crtc);
> +
>  		pll = intel_get_shared_dpll(crtc);
>  		if (pll == NULL) {
>  			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> @@ -10986,6 +11029,20 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		prepare_pipes &= ~disable_pipes;
>  	}
>  
> +	if (dev_priv->display.crtc_compute_clock) {
> +		unsigned clear_pipes = modeset_pipes | disable_pipes;
> +
> +		ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
> +		if (ret)
> +			goto done;
> +
> +		for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> +			ret = dev_priv->display.crtc_compute_clock(intel_crtc);
> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
>  	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>  		intel_crtc_disable(&intel_crtc->base);
>  
> @@ -11013,6 +11070,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  						&pipe_config->adjusted_mode);
>  	}
>  
> +	if (dev_priv->display.crtc_compute_clock)
> +		intel_shared_dpll_commit(dev_priv);
> +
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
>  	intel_modeset_update_state(dev, prepare_pipes);
> @@ -11047,9 +11107,12 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		crtc->x = x;
>  		crtc->y = y;
>  
> -		ret = dev_priv->display.crtc_mode_set(intel_crtc, x, y, fb);
> -		if (ret)
> -			goto done;
> +		if (dev_priv->display.crtc_mode_set) {
> +			ret = dev_priv->display.crtc_mode_set(intel_crtc,
> +							      x, y, fb);
> +			if (ret)
> +				goto done;
> +		}
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -11599,6 +11662,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++) {
> +		dev_priv->shared_dplls[i].new_config =
> +			&dev_priv->shared_dplls[i].config;
>  		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;
> @@ -12586,6 +12651,7 @@ static void intel_init_display(struct drm_device *dev)
>  	if (HAS_DDI(dev)) {
>  		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>  		dev_priv->display.get_plane_config = ironlake_get_plane_config;
> +		dev_priv->display.crtc_compute_clock = NULL;

dev_priv is kzalloc()ed so no need for NULL initialization.

>  		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>  		dev_priv->display.crtc_enable = haswell_crtc_enable;
>  		dev_priv->display.crtc_disable = haswell_crtc_disable;
> @@ -12599,6 +12665,7 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>  		dev_priv->display.get_plane_config = ironlake_get_plane_config;
> +		dev_priv->display.crtc_compute_clock = NULL;
>  		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>  		dev_priv->display.crtc_enable = ironlake_crtc_enable;
>  		dev_priv->display.crtc_disable = ironlake_crtc_disable;
> @@ -12608,6 +12675,7 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.get_plane_config = i9xx_get_plane_config;
> +		dev_priv->display.crtc_compute_clock = NULL;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -12617,6 +12685,7 @@ static void intel_init_display(struct drm_device *dev)
>  	} else {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.get_plane_config = i9xx_get_plane_config;
> +		dev_priv->display.crtc_compute_clock = NULL;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/8] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs

[Ander Conselvan de Oliveira]

On 10/22/2014 11:33 AM, Ville Syrjälä wrote:
> On Tue, Oct 21, 2014 at 04:02:04PM +0300, Ander Conselvan de Oliveira wrote:
>> It is possible for a mode set to fail if there aren't shared DPLLS that
>> match the new configuration requirement or other errors in clock
>> computation. If that step is executed after disabling crtcs, in the
>> failure case the hardware configuration is changed and needs to be
>> restored. Doing those things early will allow the mode set to fail
>> before actually touching the hardware.
>>
>> Follow up patches will convert different platforms to use the new
>> infrastructure.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |   3 +
>>   drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>>   drivers/gpu/drm/i915/intel_display.c | 125 +++++++++++++++++++++++++++--------
>>   3 files changed, 102 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index d7412d9..5b2464f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -233,6 +233,8 @@ struct intel_shared_dpll_config {
>>
>>   struct intel_shared_dpll {
>>   	struct intel_shared_dpll_config config;
>> +	struct intel_shared_dpll_config *new_config;
>> +
>>   	int  active; /* count of number of active CRTCs (i.e. DPMS on) */
>>   	bool on; /* is the PLL actually active? Disabled during modeset */
>>   	const char *name;
>> @@ -480,6 +482,7 @@ struct drm_i915_display_funcs {
>>   				struct intel_crtc_config *);
>>   	void (*get_plane_config)(struct intel_crtc *,
>>   				 struct intel_plane_config *);
>> +	int (*crtc_compute_clock)(struct intel_crtc *crtc);
>>   	int (*crtc_mode_set)(struct intel_crtc *crtc,
>>   			     int x, int y,
>>   			     struct drm_framebuffer *old_fb);
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 5915a07..7b8c4b8 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1375,6 +1375,8 @@ static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
>>   	dev_priv->num_shared_dpll = 2;
>>
>>   	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		dev_priv->shared_dplls[i].new_config =
>> +			&dev_priv->shared_dplls[i].config;
>>   		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_pll_disable;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index cdaf8ef..f2f7e8f 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3851,15 +3851,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>>   struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   {
>>   	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> +	struct intel_shared_dpll *pll;
>>   	enum intel_dpll_id i;
>>
>> -	if (pll) {
>> -		DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
>> -			      crtc->base.base.id, pll->name);
>> -		intel_put_shared_dpll(crtc);
>> -	}
>> -
>>   	if (HAS_PCH_IBX(dev_priv->dev)) {
>>   		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>   		i = (enum intel_dpll_id) crtc->pipe;
>> @@ -3868,7 +3862,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>   			      crtc->base.base.id, pll->name);
>>
>> -		WARN_ON(pll->config.crtc_mask);
>> +		WARN_ON(pll->new_config->crtc_mask);
>>
>>   		goto found;
>>   	}
>> @@ -3877,17 +3871,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   		pll = &dev_priv->shared_dplls[i];
>>
>>   		/* Only want to check enabled timings first */
>> -		if (pll->config.crtc_mask == 0)
>> +		if (pll->new_config->crtc_mask == 0)
>>   			continue;
>>
>> -		if (memcmp(&crtc->config.dpll_hw_state,
>> -			   &pll->config.hw_state,
>> -			   sizeof(pll->config.hw_state)) == 0) {
>> -			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
>> -				      "(crtc_mask 0x%08x, active %d)\n",
>> +		if (memcmp(&crtc->new_config->dpll_hw_state,
>> +			   &pll->new_config->hw_state,
>> +			   sizeof(pll->new_config->hw_state)) == 0) {
>> +			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>>   				      crtc->base.base.id, pll->name,
>> -				      pll->config.crtc_mask, pll->active);
>> -
>> +				      pll->new_config->crtc_mask,
>> +				      pll->active);
>>   			goto found;
>>   		}
>>   	}
>> @@ -3895,7 +3888,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   	/* Ok no matching timings, maybe there's a free one? */
>>   	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>   		pll = &dev_priv->shared_dplls[i];
>> -		if (pll->config.crtc_mask == 0) {
>> +		if (pll->new_config->crtc_mask == 0) {
>>   			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>>   				      crtc->base.base.id, pll->name);
>>   			goto found;
>> @@ -3905,18 +3898,64 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   	return NULL;
>>
>>   found:
>> -	if (pll->config.crtc_mask == 0)
>> -		pll->config.hw_state = crtc->config.dpll_hw_state;
>> +	if (pll->new_config->crtc_mask == 0)
>> +		pll->new_config->hw_state = crtc->new_config->dpll_hw_state;
>>
>> -	crtc->config.shared_dpll = i;
>> +	crtc->new_config->shared_dpll = i;
>>   	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>>   			 pipe_name(crtc->pipe));
>>
>> -	pll->config.crtc_mask |= 1 << crtc->pipe;
>> +	pll->new_config->crtc_mask |= 1 << crtc->pipe;
>>
>>   	return pll;
>>   }
>>
>> +/**
>> + * intel_shared_dpll_start_config - start a new PLL staged config
>> + * @dev_priv: DRM device
>> + * @clear_pipes: mask of pipes that will have their PLLs freed
>> + *
>> + * Starts a new PLL staged config, copying the current config but
>> + * releasing the references of pipes specified in clear_pipes.
>> + */
>> +static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
>> +					  unsigned clear_pipes)
>> +{
>> +	struct intel_shared_dpll *pll;
>> +	enum intel_dpll_id i;
>> +
>> +	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> +		pll = &dev_priv->shared_dplls[i];
>> +
>> +		pll->new_config = kmalloc(sizeof(*pll->new_config),
>> +					  GFP_KERNEL);
>> +		if (!pll->new_config)
>> +			return -ENOMEM;
>
> Need to restore new_config = &config? For the crtc config we have
> new_config==NULL outside modeset operations to catch abusers, but you
> seem to have new_config==&config for the plls normally. I had the crtc
> config that way too initially, but Daniel wanted to changed it. Not sure
> how hard it would be to do the same thing for plls?

That makes sense. It shouldn't be too difficult to do that, since the 
only function that needs to work on both old and new config is 
intel_get_shared_dpll(), and that is only called during mode set.

> My original idea for the new_config==&config approach in crtcs was that
> we might be able to share some functions that get called both during
> and after modesets and they would automagically consult the correct
> pipe config. But the NULL approach certainly seems to work so far and
> it would result in a neat explosion if it didn't.
>
> Also what happens with the already allocated pll configs if we fail
> midway through the loop here? Or if we fail later in the
> .compute_clock() or some other place before the pll config is commited?

Yeah, oops, we end up with corrupted state. Will fix.

> Could also use kmemdup() here I suppose.
>
>> +
>> +		pll->new_config->crtc_mask =
>> +			pll->config.crtc_mask & ~clear_pipes;
>> +		pll->new_config->hw_state = pll->config.hw_state;
>> +	}
>> +
>> +	return 0;
>> +}
>> +

[...]

>> @@ -12586,6 +12651,7 @@ static void intel_init_display(struct drm_device *dev)
>>   	if (HAS_DDI(dev)) {
>>   		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>>   		dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> +		dev_priv->display.crtc_compute_clock = NULL;
>
> dev_priv is kzalloc()ed so no need for NULL initialization.

Ok. I'll resend with the above fixed.


Thanks,
Ander

[PATCH 4/8] drm/i915: Covert HSW+ to choose DPLLS before disabling CRTCs

[Ander Conselvan de Oliveira]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     | 2 --
 drivers/gpu/drm/i915/intel_display.c | 9 ++++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 7b8c4b8..b42d76e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -843,8 +843,6 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
 		intel_ddi_get_crtc_new_encoder(intel_crtc);
 	int clock = intel_crtc->new_config->port_clock;
 
-	intel_put_shared_dpll(intel_crtc);
-
 	return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f2f7e8f..610de3f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7877,9 +7877,7 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	modeset_update_crtc_power_domains(dev);
 }
 
-static int haswell_crtc_mode_set(struct intel_crtc *crtc,
-				 int x, int y,
-				 struct drm_framebuffer *fb)
+static int haswell_crtc_compute_clock(struct intel_crtc *crtc)
 {
 	if (!intel_ddi_pll_select(crtc))
 		return -EINVAL;
@@ -12651,8 +12649,9 @@ static void intel_init_display(struct drm_device *dev)
 	if (HAS_DDI(dev)) {
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
-		dev_priv->display.crtc_compute_clock = NULL;
-		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
+		dev_priv->display.crtc_compute_clock =
+			haswell_crtc_compute_clock;
+		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
-- 
1.9.1

[PATCH 5/8] drm/i915: Covert ILK-IVB to choose DPLLS before disabling CRTCs

[Ander Conselvan de Oliveira]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 610de3f..9475271 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7320,9 +7320,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	return dpll | DPLL_VCO_ENABLE;
 }
 
-static int ironlake_crtc_mode_set(struct intel_crtc *crtc,
-				  int x, int y,
-				  struct drm_framebuffer *fb)
+static int ironlake_crtc_compute_clock(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	intel_clock_t clock, reduced_clock;
@@ -7368,17 +7366,13 @@ static int ironlake_crtc_mode_set(struct intel_crtc *crtc,
 		else
 			crtc->new_config->dpll_hw_state.fp1 = fp;
 
-		if (intel_crtc_to_shared_dpll(crtc))
-			intel_put_shared_dpll(crtc);
-
 		pll = intel_get_shared_dpll(crtc);
 		if (pll == NULL) {
 			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
 					 pipe_name(crtc->pipe));
 			return -EINVAL;
 		}
-	} else
-		intel_put_shared_dpll(crtc);
+	}
 
 	if (is_lvds && has_reduced_clock && i915.powersave)
 		crtc->lowfreq_avail = true;
@@ -12664,8 +12658,9 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
-		dev_priv->display.crtc_compute_clock = NULL;
-		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
+		dev_priv->display.crtc_compute_clock =
+			ironlake_crtc_compute_clock;
+		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
-- 
1.9.1

[PATCH 6/8] drm/i915: Covert remaining platforms to choose DPLLS before disabling CRTCs

[Ander Conselvan de Oliveira]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9475271..9e92ad6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6307,9 +6307,7 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
 	POSTING_READ(PIPECONF(intel_crtc->pipe));
 }
 
-static int i9xx_crtc_mode_set(struct intel_crtc *crtc,
-			      int x, int y,
-			      struct drm_framebuffer *fb)
+static int i9xx_crtc_compute_clock(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -12669,8 +12667,8 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
-		dev_priv->display.crtc_compute_clock = NULL;
-		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
+		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
+		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
@@ -12679,8 +12677,8 @@ static void intel_init_display(struct drm_device *dev)
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
-		dev_priv->display.crtc_compute_clock = NULL;
-		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
+		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
+		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-- 
1.9.1

[PATCH 7/8] drm/i915: Remove crtc_mode_set() hook

[Ander Conselvan de Oliveira]

There's no users left after the conversion to calculate clocks before
disabling crtcs during mode set.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 11 -----------
 2 files changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5b2464f..2b8a403 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -483,9 +483,6 @@ struct drm_i915_display_funcs {
 	void (*get_plane_config)(struct intel_crtc *,
 				 struct intel_plane_config *);
 	int (*crtc_compute_clock)(struct intel_crtc *crtc);
-	int (*crtc_mode_set)(struct intel_crtc *crtc,
-			     int x, int y,
-			     struct drm_framebuffer *old_fb);
 	void (*crtc_enable)(struct drm_crtc *crtc);
 	void (*crtc_disable)(struct drm_crtc *crtc);
 	void (*off)(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9e92ad6..65b040f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11096,13 +11096,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		crtc->primary->fb = fb;
 		crtc->x = x;
 		crtc->y = y;
-
-		if (dev_priv->display.crtc_mode_set) {
-			ret = dev_priv->display.crtc_mode_set(intel_crtc,
-							      x, y, fb);
-			if (ret)
-				goto done;
-		}
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -12643,7 +12636,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_compute_clock =
 			haswell_crtc_compute_clock;
-		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
@@ -12658,7 +12650,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_compute_clock =
 			ironlake_crtc_compute_clock;
-		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
@@ -12668,7 +12659,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
-		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
@@ -12678,7 +12668,6 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
-		dev_priv->display.crtc_mode_set = NULL;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-- 
1.9.1

[PATCH 8/8] drm/i915: Don't store current shared DPLL in the new pipe_config

[Ander Conselvan de Oliveira]

Now that shared DPLLs configuration is staged, there's no need to track
the current ones in the new pipe_config since those are released before
making the new pipe_config effective.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65b040f..2ad511e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5493,14 +5493,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	if (HAS_IPS(dev))
 		hsw_compute_ips_config(crtc, pipe_config);
 
-	/*
-	 * XXX: PCH/WRPLL clock sharing is done in ->mode_set if ->compute_clock is not
-	 * set, so make sure the old clock survives for now.
-	 */
-	if (dev_priv->display.crtc_compute_clock == NULL &&
-            (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev) || HAS_DDI(dev)))
-		pipe_config->shared_dpll = crtc->config.shared_dpll;
-
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 
-- 
1.9.1