Re: [PATCH 06/11] drm/i915/cdclk: Extract vlv_dsi_min_cdclk()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 29 Oct 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pull the DSI min cdclk calculation into a helper and hide
> it inside vlv_dsi.c in order to keep most DSI related
> details in one place.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 23 ++------------------
>  drivers/gpu/drm/i915/display/vlv_dsi.c     | 25 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/vlv_dsi.h     |  8 +++++++
>  3 files changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 89d12c521411..e10378744607 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -46,6 +46,7 @@
>  #include "intel_vdsc.h"
>  #include "skl_watermark.h"
>  #include "skl_watermark_regs.h"
> +#include "vlv_dsi.h"
>  #include "vlv_sideband.h"
>  
>  /**
> @@ -2849,8 +2850,6 @@ static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
>  
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>  {
> -	struct intel_display *display = to_intel_display(crtc_state);
> -	struct drm_i915_private *dev_priv = to_i915(display->drm);
>  	int min_cdclk;
>  
>  	if (!crtc_state->hw.enable)
> @@ -2859,25 +2858,7 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
>  	min_cdclk = intel_pixel_rate_to_cdclk(crtc_state);
>  	min_cdclk = max(hsw_ips_min_cdclk(crtc_state), min_cdclk);
>  	min_cdclk = max(intel_audio_min_cdclk(crtc_state), min_cdclk);
> -
> -	/*
> -	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
> -	 * than 320000KHz.
> -	 */
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
> -	    IS_VALLEYVIEW(dev_priv))
> -		min_cdclk = max(320000, min_cdclk);
> -
> -	/*
> -	 * On Geminilake once the CDCLK gets as low as 79200
> -	 * picture gets unstable, despite that values are
> -	 * correct for DSI PLL and DE PLL.
> -	 */
> -	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
> -	    IS_GEMINILAKE(dev_priv))
> -		min_cdclk = max(158400, min_cdclk);
> -
> -	/* Account for additional needs from the planes */
> +	min_cdclk = max(vlv_dsi_min_cdclk(crtc_state), min_cdclk);
>  	min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
>  
>  	if (crtc_state->dsc.compression_enable)
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 9383eedee2d4..49a895589150 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -1760,6 +1760,31 @@ static void vlv_dphy_param_init(struct intel_dsi *intel_dsi)
>  	intel_dsi_log_params(intel_dsi);
>  }
>  
> +int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> +	int min_cdclk = 0;
> +
> +	/*
> +	 * On Valleyview some DSI panels lose (v|h)sync when the clock is lower
> +	 * than 320000KHz.
> +	 */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
> +	    IS_VALLEYVIEW(dev_priv))
> +		min_cdclk = max(320000, min_cdclk);
> +
> +	/*
> +	 * On Geminilake once the CDCLK gets as low as 79200
> +	 * picture gets unstable, despite that values are
> +	 * correct for DSI PLL and DE PLL.
> +	 */
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
> +	    IS_GEMINILAKE(dev_priv))
> +		min_cdclk = max(158400, min_cdclk);
> +
> +	return min_cdclk;
> +}
> +
>  typedef void (*vlv_dsi_dmi_quirk_func)(struct intel_dsi *intel_dsi);
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.h b/drivers/gpu/drm/i915/display/vlv_dsi.h
> index cf9d7b82f288..5f99059b4c48 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.h
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.h
> @@ -8,13 +8,17 @@
>  
>  #include <linux/types.h>
>  
> +#include <drm/drm_mipi_dsi.h>
> +

Huh, why is this required? At least it's unrelated to the patch.

Other than that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  enum port;
>  struct drm_i915_private;
> +struct intel_crtc_state;
>  struct intel_dsi;
>  
>  #ifdef I915
>  void vlv_dsi_wait_for_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>  enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt);
> +int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state);
>  void vlv_dsi_init(struct drm_i915_private *dev_priv);
>  #else
>  static inline void vlv_dsi_wait_for_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
> @@ -24,6 +28,10 @@ static inline enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt
>  {
>  	return 0;
>  }
> +static inline int vlv_dsi_min_cdclk(const struct intel_crtc_state *crtc_state)
> +{
> +	return 0;
> +}
>  static inline void vlv_dsi_init(struct drm_i915_private *dev_priv)
>  {
>  }

-- 
Jani Nikula, Intel