Re: [PATCH v2 13/14] drm/i915: Replace the .modeset_commit_cdclk() hook with a more direct .set_cdclk() hook

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> With the cdclk state, all the .modeset_commit_cdclk() hooks are
> now pointless wrappers. Let's replace them with just a .set_cdclk()
> function pointer. However let's wrap that in a small helper that
> does the state comparison and prints a unified debug message across
> all platforms. We didn't even have the debug print on all platforms
> previously. This reduces the clutter in intel_atomic_commit_tail() a
> little bit.
> 
> v2: Wrap .set_cdclk() in intel_set_cdclk()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Nice cleanup!

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_cdclk.c   | 71 +++++++++++++--------------------
> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 +--
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  4 files changed, 30 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b5a8d0f4cfbd..faf753570a9a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -614,6 +614,8 @@ struct intel_cdclk_state;
>  struct drm_i915_display_funcs {
>  	void (*get_cdclk)(struct drm_i915_private *dev_priv,
>  			  struct intel_cdclk_state *cdclk_state);
> +	void (*set_cdclk)(struct drm_i915_private *dev_priv,
> +			  const struct intel_cdclk_state *cdclk_state);
>  	int (*get_fifo_size)(struct drm_i915_private *dev_priv, int plane);
>  	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
> @@ -628,7 +630,6 @@ struct drm_i915_display_funcs {
>  	int (*compute_global_watermarks)(struct drm_atomic_state *state);
>  	void (*update_wm)(struct intel_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> -	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
>  	/* Returns the active state of the crtc, and if the crtc is active,
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index bba077c4bd56..c2d995169528 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -855,9 +855,6 @@ static void skl_set_cdclk(struct drm_i915_private
> *dev_priv,
>  
>  	WARN_ON((cdclk == 24000) != (vco == 0));
>  
> -	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n",
> -			 cdclk, vco);
> -
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
>  				SKL_CDCLK_PREPARE_FOR_CHANGE,
> @@ -1158,9 +1155,6 @@ static void bxt_set_cdclk(struct drm_i915_private
> *dev_priv,
>  	u32 val, divider;
>  	int ret;
>  
> -	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d kHz)\n",
> -			 cdclk, vco);
> -
>  	/* cdclk = vco / 2 / div{1,1.5,2,4} */
>  	switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
>  	case 8:
> @@ -1323,6 +1317,22 @@ bool intel_cdclk_state_compare(const struct
> intel_cdclk_state *a,
>  	return memcmp(a, b, sizeof(*a)) == 0;
>  }
>  
> +void intel_set_cdclk(struct drm_i915_private *dev_priv,
> +		     const struct intel_cdclk_state *cdclk_state)
> +{
> +	if (intel_cdclk_state_compare(&dev_priv->cdclk.hw, cdclk_state))
> +		return;
> +
> +	if (WARN_ON_ONCE(!dev_priv->display.set_cdclk))
> +		return;
> +
> +	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d
> kHz\n",
> +			 cdclk_state->cdclk, cdclk_state->vco,
> +			 cdclk_state->ref);
> +
> +	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> +}
> +
>  static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> *crtc_state,
>  					  int pixel_rate)
>  {
> @@ -1417,16 +1427,6 @@ static int vlv_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void vlv_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> -
> -	if (IS_CHERRYVIEW(dev_priv))
> -		chv_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> -	else
> -		vlv_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> -}
> -
>  static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -1460,13 +1460,6 @@ static int bdw_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void bdw_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> -
> -	bdw_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> -}
> -
>  static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> @@ -1506,13 +1499,6 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> -
> -	skl_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> -}
> -
>  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -1557,13 +1543,6 @@ static int bxt_modeset_calc_cdclk(struct
> drm_atomic_state *state)
>  	return 0;
>  }
>  
> -static void bxt_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> -
> -	bxt_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> -}
> -
>  static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
>  {
>  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> @@ -1718,24 +1697,24 @@ void intel_update_rawclk(struct drm_i915_private
> *dev_priv)
>  
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  {
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		dev_priv->display.modeset_commit_cdclk =
> -			vlv_modeset_commit_cdclk;
> +	if (IS_CHERRYVIEW(dev_priv)) {
> +		dev_priv->display.set_cdclk = chv_set_cdclk;
> +		dev_priv->display.modeset_calc_cdclk =
> +			vlv_modeset_calc_cdclk;
> +	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		dev_priv->display.set_cdclk = vlv_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			vlv_modeset_calc_cdclk;
>  	} else if (IS_BROADWELL(dev_priv)) {
> -		dev_priv->display.modeset_commit_cdclk =
> -			bdw_modeset_commit_cdclk;
> +		dev_priv->display.set_cdclk = bdw_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			bdw_modeset_calc_cdclk;
>  	} else if (IS_GEN9_LP(dev_priv)) {
> -		dev_priv->display.modeset_commit_cdclk =
> -			bxt_modeset_commit_cdclk;
> +		dev_priv->display.set_cdclk = bxt_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			bxt_modeset_calc_cdclk;
>  	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> -		dev_priv->display.modeset_commit_cdclk =
> -			skl_modeset_commit_cdclk;
> +		dev_priv->display.set_cdclk = skl_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk =
>  			skl_modeset_calc_cdclk;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index fb3abb58b6ca..409537011b28 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12872,10 +12872,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  	if (intel_state->modeset) {
>  		drm_atomic_helper_update_legacy_modeset_state(state->dev,
> state);
>  
> -		if (dev_priv->display.modeset_commit_cdclk &&
> -		    !intel_cdclk_state_compare(&dev_priv->cdclk.hw,
> -					       &dev_priv->cdclk.actual))
> -			dev_priv->display.modeset_commit_cdclk(state);
> +		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
>  
>  		/*
>  		 * SKL workaround: bspec recommends we disable the SAGV when
> we
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d167ee458d63..40074827f01d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1231,6 +1231,8 @@ void intel_update_cdclk(struct drm_i915_private
> *dev_priv);
>  void intel_update_rawclk(struct drm_i915_private *dev_priv);
>  bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
>  			       const struct intel_cdclk_state *b);
> +void intel_set_cdclk(struct drm_i915_private *dev_priv,
> +		     const struct intel_cdclk_state *cdclk_state);
>  
>  /* intel_display.c */
>  enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx