diff mbox

[04/11] drm/i915: Factor out i9xx_compute_clocks() like ironlake_compute_clocks()

Message ID 1351698624-26626-5-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Oct. 31, 2012, 3:50 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split the i9xx clock stuff out from i9xx_compute_clocks().

Only compile tested!

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  128 ++++++++++++++++++++--------------
 1 files changed, 75 insertions(+), 53 deletions(-)

Comments

Daniel Vetter Oct. 31, 2012, 4:28 p.m. UTC | #1 
On Wed, Oct 31, 2012 at 05:50:17PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Split the i9xx clock stuff out from i9xx_compute_clocks().
> 
> Only compile tested!
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'm working on a massive overhaul of the clock computation madness, so
that we do all that stuff before we start to touch the hw (and so would
finally have a reasonable chance at getting global bw issues right).
Current wip pile is at:

http://cgit.freedesktop.org/~danvet/drm/log/?h=modeset-rework

Until we've figured out what's the best approach I prefere if we keep the
churn in this (rather convoluted code) low.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  128 ++++++++++++++++++++--------------
>  1 files changed, 75 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6d3feea..b42637b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4557,6 +4557,76 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
>  		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>  }
>  
> +static bool i9xx_compute_clocks(struct drm_crtc *crtc,
> +				struct drm_display_mode *adjusted_mode,
> +				intel_clock_t *clock,
> +				bool *has_reduced_clock,
> +				intel_clock_t *reduced_clock,
> +				int *refclk, int *num_connectors, bool *is_dp)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *encoder;
> +	const intel_limit_t *limit;
> +	bool ok, is_sdvo = false, is_tv = false, is_lvds = false;
> +
> +	*num_connectors = 0;
> +
> +	for_each_encoder_on_crtc(dev, crtc, encoder) {
> +		switch (encoder->type) {
> +		case INTEL_OUTPUT_LVDS:
> +			is_lvds = true;
> +			break;
> +		case INTEL_OUTPUT_SDVO:
> +		case INTEL_OUTPUT_HDMI:
> +			is_sdvo = true;
> +			if (encoder->needs_tv_clock)
> +				is_tv = true;
> +			break;
> +		case INTEL_OUTPUT_TVOUT:
> +			is_tv = true;
> +			break;
> +		case INTEL_OUTPUT_DISPLAYPORT:
> +			*is_dp = true;
> +			break;
> +		}
> +
> +		(*num_connectors)++;
> +	}
> +
> +	*refclk = i9xx_get_refclk(crtc, *num_connectors);
> +
> +	/*
> +	 * Returns a set of divisors for the desired target clock with the given
> +	 * refclk, or FALSE.  The returned values represent the clock equation:
> +	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> +	 */
> +	limit = intel_limit(crtc, *refclk);
> +	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, *refclk, NULL,
> +			      clock);
> +	if (!ok)
> +		return false;
> +
> +	if (is_lvds && dev_priv->lvds_downclock_avail) {
> +		/*
> +		 * Ensure we match the reduced clock's P to the target clock.
> +		 * If the clocks don't match, we can't switch the display clock
> +		 * by using the FP0/FP1. In such case we will disable the LVDS
> +		 * downclock feature.
> +		*/
> +		*has_reduced_clock = limit->find_pll(limit, crtc,
> +						     dev_priv->lvds_downclock,
> +						     *refclk,
> +						     clock,
> +						     reduced_clock);
> +	}
> +
> +	if (is_sdvo && is_tv)
> +		i9xx_adjust_sdvo_tv_clock(adjusted_mode, clock);
> +
> +	return true;
> +}
> +
>  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  			      struct drm_display_mode *mode,
>  			      struct drm_display_mode *adjusted_mode,
> @@ -4571,44 +4641,13 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	int refclk, num_connectors = 0;
>  	intel_clock_t clock, reduced_clock;
>  	u32 dspcntr, pipeconf;
> -	bool ok, has_reduced_clock = false, is_sdvo = false;
> -	bool is_lvds = false, is_tv = false, is_dp = false;
> -	struct intel_encoder *encoder;
> -	const intel_limit_t *limit;
> +	bool ok, has_reduced_clock = false;
> +	bool is_dp = false;
>  	int ret;
>  
> -	for_each_encoder_on_crtc(dev, crtc, encoder) {
> -		switch (encoder->type) {
> -		case INTEL_OUTPUT_LVDS:
> -			is_lvds = true;
> -			break;
> -		case INTEL_OUTPUT_SDVO:
> -		case INTEL_OUTPUT_HDMI:
> -			is_sdvo = true;
> -			if (encoder->needs_tv_clock)
> -				is_tv = true;
> -			break;
> -		case INTEL_OUTPUT_TVOUT:
> -			is_tv = true;
> -			break;
> -		case INTEL_OUTPUT_DISPLAYPORT:
> -			is_dp = true;
> -			break;
> -		}
> -
> -		num_connectors++;
> -	}
> -
> -	refclk = i9xx_get_refclk(crtc, num_connectors);
> -
> -	/*
> -	 * Returns a set of divisors for the desired target clock with the given
> -	 * refclk, or FALSE.  The returned values represent the clock equation:
> -	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> -	 */
> -	limit = intel_limit(crtc, refclk);
> -	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> -			     &clock);
> +	ok = i9xx_compute_clocks(crtc, adjusted_mode, &clock,
> +				 &has_reduced_clock, &reduced_clock,
> +				 &refclk, &num_connectors, &is_dp);
>  	if (!ok) {
>  		DRM_ERROR("Couldn't find PLL settings for mode!\n");
>  		return -EINVAL;
> @@ -4617,23 +4656,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	/* Ensure that the cursor is valid for the new mode before changing... */
>  	intel_crtc_update_cursor(crtc, true);
>  
> -	if (is_lvds && dev_priv->lvds_downclock_avail) {
> -		/*
> -		 * Ensure we match the reduced clock's P to the target clock.
> -		 * If the clocks don't match, we can't switch the display clock
> -		 * by using the FP0/FP1. In such case we will disable the LVDS
> -		 * downclock feature.
> -		*/
> -		has_reduced_clock = limit->find_pll(limit, crtc,
> -						    dev_priv->lvds_downclock,
> -						    refclk,
> -						    &clock,
> -						    &reduced_clock);
> -	}
> -
> -	if (is_sdvo && is_tv)
> -		i9xx_adjust_sdvo_tv_clock(adjusted_mode, &clock);
> -
>  	if (IS_GEN2(dev))
>  		i8xx_update_pll(crtc, adjusted_mode, &clock,
>  				has_reduced_clock ? &reduced_clock : NULL,
> -- 
> 1.7.8.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 31, 2012, 5:04 p.m. UTC | #2 
On Wed, Oct 31, 2012 at 05:28:40PM +0100, Daniel Vetter wrote:
> On Wed, Oct 31, 2012 at 05:50:17PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Split the i9xx clock stuff out from i9xx_compute_clocks().
> > 
> > Only compile tested!
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I'm working on a massive overhaul of the clock computation madness, so
> that we do all that stuff before we start to touch the hw (and so would
> finally have a reasonable chance at getting global bw issues right).

I was sure that the compute_clock() funcs already satisfied that.
Perhaps I didn't look hard enough.

The reason for this patch actually was that I'm already using that
approach in the atomic modeset code. Ie. I call compute_clock() for
all modified CRTCs before touching the hardware.

Or is the problem simply that multiple calls to compute_clock() would
depend on some bit of state that is changed somewhere later in
crtc_mode_set()? So that when doing a multi-CRTC modeset, the final
state is never seen by any of the compute_clock() funcs when used this
way?
Daniel Vetter Oct. 31, 2012, 6:29 p.m. UTC | #3 
On Wed, Oct 31, 2012 at 07:04:29PM +0200, Ville Syrjälä wrote:
> On Wed, Oct 31, 2012 at 05:28:40PM +0100, Daniel Vetter wrote:
> > On Wed, Oct 31, 2012 at 05:50:17PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Split the i9xx clock stuff out from i9xx_compute_clocks().
> > > 
> > > Only compile tested!
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I'm working on a massive overhaul of the clock computation madness, so
> > that we do all that stuff before we start to touch the hw (and so would
> > finally have a reasonable chance at getting global bw issues right).
> 
> I was sure that the compute_clock() funcs already satisfied that.
> Perhaps I didn't look hard enough.
> 
> The reason for this patch actually was that I'm already using that
> approach in the atomic modeset code. Ie. I call compute_clock() for
> all modified CRTCs before touching the hardware.
> 
> Or is the problem simply that multiple calls to compute_clock() would
> depend on some bit of state that is changed somewhere later in
> crtc_mode_set()? So that when doing a multi-CRTC modeset, the final
> state is never seen by any of the compute_clock() funcs when used this
> way?

Well the plan is to compute all this state into struct intel_crtc_config
and then switch the code that changes the hw state to only use that
precomputed configuration instead of recomputing it. The reason for that
intermediate state keeping is twofold:
- We need to have some much better notion of a pipe configuration for
  fastboot anyway (including things like pll sharing and exact dotclocks).
  Using the same data structure for both the hw readout code needed for
  fastboot and in our own modeset code should allow for some nice
  consistency checks.
- We have a bunch of rippling dependencies in our state computation, e.g.
  depending upon global configuration we have different amounts of
  bandwidth available, which (should) affect the pipe bpp and so the
  clocks we select, in turn having effects on how we can share plls. If we
  try to run these computations twice, once in the ->check callback and
  once when changing the hw state we'll never get a perfect match. Hence I
  want to precompute and store all values into that pipe_config.

Essentially that branch is trying to implement all the atomic modeset
stuff without actually having an atomic modeset ioctl ;-) My aim is that
in the end I can detect fdi b/c link sharing constraints, dither the other
pipe to a lower pipe_bpp, add it to the array of pipe that need a full
modeset and then enable everything. Also, while I'm at it a want to kill
all these messy layering inversions where the crtc code pokes around in
various encoders ... It's not quite there yet ;-)

Cheers, Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6d3feea..b42637b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4557,6 +4557,76 @@  static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
 		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
 }
 
+static bool i9xx_compute_clocks(struct drm_crtc *crtc,
+				struct drm_display_mode *adjusted_mode,
+				intel_clock_t *clock,
+				bool *has_reduced_clock,
+				intel_clock_t *reduced_clock,
+				int *refclk, int *num_connectors, bool *is_dp)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	const intel_limit_t *limit;
+	bool ok, is_sdvo = false, is_tv = false, is_lvds = false;
+
+	*num_connectors = 0;
+
+	for_each_encoder_on_crtc(dev, crtc, encoder) {
+		switch (encoder->type) {
+		case INTEL_OUTPUT_LVDS:
+			is_lvds = true;
+			break;
+		case INTEL_OUTPUT_SDVO:
+		case INTEL_OUTPUT_HDMI:
+			is_sdvo = true;
+			if (encoder->needs_tv_clock)
+				is_tv = true;
+			break;
+		case INTEL_OUTPUT_TVOUT:
+			is_tv = true;
+			break;
+		case INTEL_OUTPUT_DISPLAYPORT:
+			*is_dp = true;
+			break;
+		}
+
+		(*num_connectors)++;
+	}
+
+	*refclk = i9xx_get_refclk(crtc, *num_connectors);
+
+	/*
+	 * Returns a set of divisors for the desired target clock with the given
+	 * refclk, or FALSE.  The returned values represent the clock equation:
+	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
+	 */
+	limit = intel_limit(crtc, *refclk);
+	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, *refclk, NULL,
+			      clock);
+	if (!ok)
+		return false;
+
+	if (is_lvds && dev_priv->lvds_downclock_avail) {
+		/*
+		 * Ensure we match the reduced clock's P to the target clock.
+		 * If the clocks don't match, we can't switch the display clock
+		 * by using the FP0/FP1. In such case we will disable the LVDS
+		 * downclock feature.
+		*/
+		*has_reduced_clock = limit->find_pll(limit, crtc,
+						     dev_priv->lvds_downclock,
+						     *refclk,
+						     clock,
+						     reduced_clock);
+	}
+
+	if (is_sdvo && is_tv)
+		i9xx_adjust_sdvo_tv_clock(adjusted_mode, clock);
+
+	return true;
+}
+
 static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			      struct drm_display_mode *mode,
 			      struct drm_display_mode *adjusted_mode,
@@ -4571,44 +4641,13 @@  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	int refclk, num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
 	u32 dspcntr, pipeconf;
-	bool ok, has_reduced_clock = false, is_sdvo = false;
-	bool is_lvds = false, is_tv = false, is_dp = false;
-	struct intel_encoder *encoder;
-	const intel_limit_t *limit;
+	bool ok, has_reduced_clock = false;
+	bool is_dp = false;
 	int ret;
 
-	for_each_encoder_on_crtc(dev, crtc, encoder) {
-		switch (encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
-		case INTEL_OUTPUT_SDVO:
-		case INTEL_OUTPUT_HDMI:
-			is_sdvo = true;
-			if (encoder->needs_tv_clock)
-				is_tv = true;
-			break;
-		case INTEL_OUTPUT_TVOUT:
-			is_tv = true;
-			break;
-		case INTEL_OUTPUT_DISPLAYPORT:
-			is_dp = true;
-			break;
-		}
-
-		num_connectors++;
-	}
-
-	refclk = i9xx_get_refclk(crtc, num_connectors);
-
-	/*
-	 * Returns a set of divisors for the desired target clock with the given
-	 * refclk, or FALSE.  The returned values represent the clock equation:
-	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
-	 */
-	limit = intel_limit(crtc, refclk);
-	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
-			     &clock);
+	ok = i9xx_compute_clocks(crtc, adjusted_mode, &clock,
+				 &has_reduced_clock, &reduced_clock,
+				 &refclk, &num_connectors, &is_dp);
 	if (!ok) {
 		DRM_ERROR("Couldn't find PLL settings for mode!\n");
 		return -EINVAL;
@@ -4617,23 +4656,6 @@  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	if (is_lvds && dev_priv->lvds_downclock_avail) {
-		/*
-		 * Ensure we match the reduced clock's P to the target clock.
-		 * If the clocks don't match, we can't switch the display clock
-		 * by using the FP0/FP1. In such case we will disable the LVDS
-		 * downclock feature.
-		*/
-		has_reduced_clock = limit->find_pll(limit, crtc,
-						    dev_priv->lvds_downclock,
-						    refclk,
-						    &clock,
-						    &reduced_clock);
-	}
-
-	if (is_sdvo && is_tv)
-		i9xx_adjust_sdvo_tv_clock(adjusted_mode, &clock);
-
 	if (IS_GEN2(dev))
 		i8xx_update_pll(crtc, adjusted_mode, &clock,
 				has_reduced_clock ? &reduced_clock : NULL,