diff mbox

[4/9] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs

Message ID 1414575158-28148-5-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ander Conselvan de Oliveira Oct. 29, 2014, 9:32 a.m. UTC
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.

v2: Keep pll->new_config valid only during mode set (Ville)
    Use kmemdup() in i915_shared_dpll_start_config() (Ville)
    Restore old pll config if something fails before commit (Ville)
    Don't set compute_clock hooks since dev_priv is kzalloc()'d (Ville)

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 | 142 ++++++++++++++++++++++++++++-------
 2 files changed, 117 insertions(+), 28 deletions(-)

Comments

Daniel Vetter Nov. 3, 2014, 1:51 p.m. UTC | #1
On Wed, Oct 29, 2014 at 11:32:33AM +0200, 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.
> 
> v2: Keep pll->new_config valid only during mode set (Ville)
>     Use kmemdup() in i915_shared_dpll_start_config() (Ville)
>     Restore old pll config if something fails before commit (Ville)
>     Don't set compute_clock hooks since dev_priv is kzalloc()'d (Ville)
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Ran into a blocking question with this one, merged thus far.

> @@ -7395,6 +7456,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);

Don't we need the same fixup in intel_ddi_pll_select?
> +
>  		pll = intel_get_shared_dpll(crtc);
>  		if (pll == NULL) {
>  			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> @@ -10739,6 +10803,22 @@ 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) {
> +				intel_shared_dpll_abort_config(dev_priv);
> +				goto done;
> +			}
> +		}
> +	}

Might be useful to shuffle this and the vlv-specific code above into a new
intel_compute_global_config kind of helper function. But that can be done
later on I think.
-Daniel
Daniel Vetter Nov. 3, 2014, 1:56 p.m. UTC | #2
On Mon, Nov 03, 2014 at 02:51:27PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 11:32:33AM +0200, 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.
> > 
> > v2: Keep pll->new_config valid only during mode set (Ville)
> >     Use kmemdup() in i915_shared_dpll_start_config() (Ville)
> >     Restore old pll config if something fails before commit (Ville)
> >     Don't set compute_clock hooks since dev_priv is kzalloc()'d (Ville)
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> Ran into a blocking question with this one, merged thus far.
> 
> > @@ -7395,6 +7456,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);
> 
> Don't we need the same fixup in intel_ddi_pll_select?

Ok, I think I've figured it out - hsw does an unconditional put since a
ddi pll might not be needed (for e.g. DP).
-Daniel
Ander Conselvan de Oliveira Nov. 4, 2014, 6:57 a.m. UTC | #3
On 11/03/2014 03:56 PM, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 02:51:27PM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 11:32:33AM +0200, 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.
>>>
>>> v2: Keep pll->new_config valid only during mode set (Ville)
>>>      Use kmemdup() in i915_shared_dpll_start_config() (Ville)
>>>      Restore old pll config if something fails before commit (Ville)
>>>      Don't set compute_clock hooks since dev_priv is kzalloc()'d (Ville)
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> Ran into a blocking question with this one, merged thus far.
>>
>>> @@ -7395,6 +7456,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);
>>
>> Don't we need the same fixup in intel_ddi_pll_select?
>
> Ok, I think I've figured it out - hsw does an unconditional put since a
> ddi pll might not be needed (for e.g. DP).

Yep, intel_ddi_pll_select() already had an unconditional call to 
intel_put_shared_dpll(), while the ironlake code relied on them being 
released in intel_get_shared_dpll(). After this patch the shared DPLLs 
should be release by the caller before getting a new one, but a later 
patch removes all those intel_put_shared_dpll() calls anyway.

Ander
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4b3c00f..2d07b79 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;
@@ -481,6 +483,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_display.c b/drivers/gpu/drm/i915/intel_display.c
index aff9aa5..67f8828 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3859,15 +3859,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;
@@ -3876,7 +3870,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;
 	}
@@ -3885,17 +3879,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;
 		}
 	}
@@ -3903,7 +3896,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;
@@ -3913,18 +3906,85 @@  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 = kmemdup(&pll->config, sizeof pll->config,
+					  GFP_KERNEL);
+		if (!pll->new_config)
+			goto cleanup;
+
+		pll->new_config->crtc_mask &= ~clear_pipes;
+	}
+
+	return 0;
+
+cleanup:
+	while (--i >= 0) {
+		pll = &dev_priv->shared_dplls[i];
+		pll->new_config = NULL;
+	}
+
+	return -ENOMEM;
+}
+
+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];
+
+		WARN_ON(pll->new_config == &pll->config);
+
+		pll->config = *pll->new_config;
+		kfree(pll->new_config);
+		pll->new_config = NULL;
+	}
+}
+
+static void intel_shared_dpll_abort_config(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];
+
+		WARN_ON(pll->new_config == &pll->config);
+
+		kfree(pll->new_config);
+		pll->new_config = NULL;
+	}
+}
+
 static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -5409,11 +5469,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);
 
@@ -5463,10 +5523,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)
@@ -7395,6 +7456,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",
@@ -10739,6 +10803,22 @@  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) {
+				intel_shared_dpll_abort_config(dev_priv);
+				goto done;
+			}
+		}
+	}
+
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
 		intel_crtc_disable(&intel_crtc->base);
 
@@ -10766,6 +10846,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);
@@ -10800,9 +10883,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. */