[Intel-gfx] [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()

Pandiyan, Dhinakaran dhinakaran.pandiyan at intel.com
Tue Jul 11 20:21:32 UTC 2017




On Mon, 2017-07-10 at 22:33 +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Currently the .modeset_calc_cdclk() hooks check the final cdclk value
> against the max allowed. That's not really sufficient since the low
> level calc_cdclk() functions effectively clamp the minimum required
> cdclk to the max supported by the platform. Hence if the minimum
> required exceeds the platforms capabilities we'd keep going anyway
> using the max cdclk frequency.
> 
> To fix that let's move the check earlier into
> intel_crtc_compute_min_cdclk() and we'll check the minimum required
> cdclk of the pipe against the maximum supported by the platform.
> 

I suppose this should save some power in the case where one of the
CRTC's pixel rate exceeds platform capabilities. And failing the
atomic_check instead of going with max_cdclk will help the userspace try
a lower mode. Is that the idea?

Moving the checks makes sense to me because that seems like the original
intention anyway, but I think it's a good idea to get someone else to
take a look too.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>

> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_cdclk.c   | 96 +++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_display.c |  5 +-
>  2 files changed, 48 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 50f153dbea14..603950f1b87f 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
>  		min_cdclk = max(2 * 96000, min_cdclk);
>  
> +	if (min_cdclk > dev_priv->max_cdclk_freq) {
> +		DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> +			      min_cdclk, dev_priv->max_cdclk_freq);
> +		return -EINVAL;
> +	}
> +
>  	return min_cdclk;
>  }
>  
> @@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc *crtc;
>  	struct intel_crtc_state *crtc_state;
> -	int min_cdclk = 0, i;
> +	int min_cdclk, i;
>  	enum pipe pipe;
>  
>  	memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
>  	       sizeof(intel_state->min_cdclk));
>  
> -	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> -		intel_state->min_cdclk[i] =
> -			intel_crtc_compute_min_cdclk(crtc_state);
> +	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> +		min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> +		if (min_cdclk < 0)
> +			return min_cdclk;
> +
> +		intel_state->min_cdclk[i] = min_cdclk;
> +	}
>  
> +	min_cdclk = 0;
>  	for_each_pipe(dev_priv, pipe)
>  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>  
> @@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int min_cdclk = intel_compute_min_cdclk(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int min_cdclk, cdclk;
>  
> -	cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> +	min_cdclk = intel_compute_min_cdclk(state);
> +	if (min_cdclk < 0)
> +		return min_cdclk;
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> +	cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
>  
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
> @@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>  
>  static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> -	int min_cdclk = intel_compute_min_cdclk(state);
> -	int cdclk;
> +	int min_cdclk, cdclk;
> +
> +	min_cdclk = intel_compute_min_cdclk(state);
> +	if (min_cdclk < 0)
> +		return min_cdclk;
>  
>  	/*
>  	 * FIXME should also account for plane ratio
> @@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = bdw_calc_cdclk(min_cdclk);
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
>  	if (!intel_state->active_crtcs) {
> @@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  
>  static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int min_cdclk = intel_compute_min_cdclk(state);
> -	int cdclk, vco;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int min_cdclk, cdclk, vco;
> +
> +	min_cdclk = intel_compute_min_cdclk(state);
> +	if (min_cdclk < 0)
> +		return min_cdclk;
>  
>  	vco = intel_state->cdclk.logical.vco;
>  	if (!vco)
> @@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	 */
>  	cdclk = skl_calc_cdclk(min_cdclk, vco);
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
>  	intel_state->cdclk.logical.vco = vco;
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
> @@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	int min_cdclk = intel_compute_min_cdclk(state);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int cdclk, vco;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int min_cdclk, cdclk, vco;
> +
> +	min_cdclk = intel_compute_min_cdclk(state);
> +	if (min_cdclk < 0)
> +		return min_cdclk;
>  
>  	if (IS_GEMINILAKE(dev_priv)) {
>  		cdclk = glk_calc_cdclk(min_cdclk);
> @@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  		vco = bxt_de_pll_vco(dev_priv, cdclk);
>  	}
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
>  	intel_state->cdclk.logical.vco = vco;
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
> @@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	struct intel_atomic_state *intel_state =
> -		to_intel_atomic_state(state);
> -	int min_cdclk = intel_compute_min_cdclk(state);
> -	int cdclk, vco;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	int min_cdclk, cdclk, vco;
> +
> +	min_cdclk = intel_compute_min_cdclk(state);
> +	if (min_cdclk < 0)
> +		return min_cdclk;
>  
>  	cdclk = cnl_calc_cdclk(min_cdclk);
>  	vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>  
> -	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			      cdclk, dev_priv->max_cdclk_freq);
> -		return -EINVAL;
> -	}
> -
>  	intel_state->cdclk.logical.vco = vco;
>  	intel_state->cdclk.logical.cdclk = cdclk;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b47535f5d95d..1caf0ef82e36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  			intel_crtc_compute_pixel_rate(crtc_state);
>  
> -			if (dev_priv->display.modeset_calc_cdclk)
> +			if (dev_priv->display.modeset_calc_cdclk) {
>  				min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> +				if (WARN_ON(min_cdclk < 0))
> +					min_cdclk = 0;
> +			}
>  
>  			drm_calc_timestamping_constants(&crtc->base,
>  							&crtc_state->base.adjusted_mode);


More information about the Intel-gfx mailing list