[Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update

[Ville Syrjälä]

On Tue, Mar 31, 2015 at 02:14:23PM +0300, Mika Kahola wrote:
> Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
> into one function 'intel_modeset_global_pipes()'
> 
> Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 89 +++++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5ed40df..7180d2b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
>  		return 200000;
>  }
>  
> -/* compute the max pixel clock for new configuration */
> -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = dev_priv->dev;
> -	struct intel_crtc *intel_crtc;
> -	int max_pixclk = 0;
> -
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		if (intel_crtc->new_enabled)
> -			max_pixclk = max(max_pixclk,
> -					 intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> -	}
> -
> -	return max_pixclk;
> -}
> -
> -static void valleyview_modeset_global_pipes(struct drm_device *dev,
> -					    unsigned *prepare_pipes)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc;
> -	int max_pixclk = intel_mode_max_pixclk(dev_priv);
> -
> -	if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
> -		return;
> -
> -	/* disable/enable all currently active pipes while we change cdclk */
> -	for_each_intel_crtc(dev, intel_crtc)
> -		if (intel_crtc->base.state->enable)
> -			*prepare_pipes |= (1 << intel_crtc->pipe);
> -}
> -
>  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
>  {
>  	unsigned int credits, default_credits;
> @@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
>  	WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
>  }
>  
> +/* compute the max pixel clock for new configuration */
> +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_crtc *intel_crtc;
> +	int max_pixclk = 0;
> +
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		if (intel_crtc->new_enabled)
> +			max_pixclk = max(max_pixclk,
> +					 intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> +	}
> +
> +	return max_pixclk;
> +}
> +
>  static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>  	     cdclk, dev_priv->cdclk_freq);
>  }
>  
> -static void haswell_modeset_global_pipes(struct drm_device *dev,
> -					 unsigned *prepare_pipes)
> +static void intel_modeset_global_pipes(struct drm_device *dev,
> +				       unsigned *prepare_pipes,
> +				       unsigned *disable_pipes)

You don't modify disable_pipes, so no need to pass as pointer.

I do think passing disable_pipes into intel_modeset_global_pipes() does
make sense however, as it makes it clearer why we need to clear out the
disable_pipes when the code is all in one place like this.

>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc;
> -	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> +	int max_pixclk;
>  
> -	if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
> -	    dev_priv->cdclk_freq)
> +	/* this modeset is valid only for VLV, HSW, and BDW */
> +	if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
>  		return;
>  
> +	if (IS_VALLEYVIEW(dev)) {
> +		max_pixclk = intel_mode_max_pixclk(dev_priv);
> +		if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
> +		    dev_priv->cdclk_freq)
> +			return;
> +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +		max_pixclk = ilk_max_pixel_rate(dev_priv);
> +		if (haswell_calc_cdclk(dev_priv, max_pixclk) ==
> +		    dev_priv->cdclk_freq)
> +			return;
> +
> +	}

Maybe move the current vs. newly computed pixclk comparison out of
the if ladder, so we don't have to duplicate it for each platform?
It should also get rid of the ugly line length issue we have here.

Eventually we should get rid of the platform specifics here, but
that requires that we sort out the pfit pixel rate mess for all
platforms in the same way. Currently we just ignore the pfit
scaling ratio for gmch (and gen9 IIRC) platforms.

> +
>  	/* disable/enable all currently active pipes while we change cdclk */
>  	for_each_intel_crtc(dev, crtc)
> -		if (crtc->base.enabled)
> -			*prepare_pipes |= 1 << crtc->pipe;
> +		if (crtc->base.state->enable)
> +			*prepare_pipes |= (1 << crtc->pipe);
> +
> +	/* may have added more to prepare_pipes than we should */
> +	*prepare_pipes &= ~*disable_pipes;
>  }
>  
>  static void haswell_modeset_global_resources(struct drm_atomic_state *state)
> @@ -12120,15 +12121,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (IS_VALLEYVIEW(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -		if (IS_VALLEYVIEW(dev))
> -			valleyview_modeset_global_pipes(dev, &prepare_pipes);
> -		else
> -			haswell_modeset_global_pipes(dev, &prepare_pipes);
> -
> -		/* may have added more to prepare_pipes than we should */
> -		prepare_pipes &= ~disable_pipes;
> -	}
> +	intel_modeset_global_pipes(dev, &prepare_pipes, &disable_pipes);
>  
>  	ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes);
>  	if (ret)
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC