Re: [Intel-gfx] [PATCH v2 3/4] drm/i915: Introduce for_each_shared_dpll()

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

On Mon, 26 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> No one really cares how we store the shared_dplls. Currently
> it happens to be an array, but we could change that to a more
> flexible scheme at some point. Hide the implementation details
> behind an iterator macro.
>
> The slight downside is the pll variable moving out of the
> loop scope, but maybe someday soon we'll start to convert
> everything over to having declarations within for-statements...
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/intel_display_debugfs.c  |  5 +--
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 38 +++++++++----------
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  4 ++
>  .../gpu/drm/i915/display/intel_pch_refclk.c   |  4 +-
>  4 files changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index c5f47df0f362..c85f60f0ced8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -932,6 +932,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
>  	struct drm_device *dev = &dev_priv->drm;
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
>  	drm_modeset_lock_all(dev);
> @@ -940,9 +941,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  		   dev_priv->display.dpll.ref_clks.nssc,
>  		   dev_priv->display.dpll.ref_clks.ssc);
>  
> -	for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &dev_priv->display.dpll.shared_dplls[i];
> -
> +	for_each_shared_dpll(dev_priv, pll, i) {
>  		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->info->name,

Only noticed it here, but the loop variable i is no longer the DPLL
number, right? No place should actually use i for anything other than
the looping.

This should probably be fixed in an earlier patch.

BR,
Jani.

>  			   pll->info->id);
>  		seq_printf(m, " pipe_mask: 0x%x, active: 0x%x, on: %s\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 25e6f7a427b0..3dd414d663f0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -110,14 +110,12 @@ static void
>  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
>  				  struct intel_shared_dpll_state *shared_dpll)
>  {
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
>  	/* Copy shared dpll state */
> -	for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &dev_priv->display.dpll.shared_dplls[i];
> -
> +	for_each_shared_dpll(dev_priv, pll, i)
>  		shared_dpll[pll->index] = pll->state;
> -	}
>  }
>  
>  static struct intel_shared_dpll_state *
> @@ -149,11 +147,10 @@ struct intel_shared_dpll *
>  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
>  			    enum intel_dpll_id id)
>  {
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
> -	for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &dev_priv->display.dpll.shared_dplls[i];
> -
> +	for_each_shared_dpll(dev_priv, pll, i) {
>  		if (pll->info->id == id)
>  			return pll;
>  	}
> @@ -311,12 +308,11 @@ void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  static unsigned long
>  intel_dpll_mask_all(struct drm_i915_private *i915)
>  {
> +	struct intel_shared_dpll *pll;
>  	unsigned long dpll_mask = 0;
>  	int i;
>  
> -	for (i = 0; i < i915->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &i915->display.dpll.shared_dplls[i];
> -
> +	for_each_shared_dpll(i915, pll, i) {
>  		drm_WARN_ON(&i915->drm, dpll_mask & BIT(pll->info->id));
>  
>  		dpll_mask |= BIT(pll->info->id);
> @@ -449,16 +445,14 @@ void intel_shared_dpll_swap_state(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_shared_dpll_state *shared_dpll = state->shared_dpll;
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
>  	if (!state->dpll_set)
>  		return;
>  
> -	for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &dev_priv->display.dpll.shared_dplls[i];
> -
> +	for_each_shared_dpll(dev_priv, pll, i)
>  		swap(pll->state, shared_dpll[pll->index]);
> -	}
>  }
>  
>  static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -4431,10 +4425,11 @@ void intel_dpll_update_ref_clks(struct drm_i915_private *i915)
>  
>  void intel_dpll_readout_hw_state(struct drm_i915_private *i915)
>  {
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
> -	for (i = 0; i < i915->display.dpll.num_shared_dpll; i++)
> -		readout_dpll_hw_state(i915, &i915->display.dpll.shared_dplls[i]);
> +	for_each_shared_dpll(i915, pll, i)
> +		readout_dpll_hw_state(i915, pll);
>  }
>  
>  static void sanitize_dpll_state(struct drm_i915_private *i915,
> @@ -4458,10 +4453,11 @@ static void sanitize_dpll_state(struct drm_i915_private *i915,
>  
>  void intel_dpll_sanitize_state(struct drm_i915_private *i915)
>  {
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
> -	for (i = 0; i < i915->display.dpll.num_shared_dpll; i++)
> -		sanitize_dpll_state(i915, &i915->display.dpll.shared_dplls[i]);
> +	for_each_shared_dpll(i915, pll, i)
> +		sanitize_dpll_state(i915, pll);
>  }
>  
>  /**
> @@ -4571,9 +4567,9 @@ void intel_shared_dpll_state_verify(struct intel_crtc *crtc,
>  
>  void intel_shared_dpll_verify_disabled(struct drm_i915_private *i915)
>  {
> +	struct intel_shared_dpll *pll;
>  	int i;
>  
> -	for (i = 0; i < i915->display.dpll.num_shared_dpll; i++)
> -		verify_single_dpll_state(i915, &i915->display.dpll.shared_dplls[i],
> -					 NULL, NULL);
> +	for_each_shared_dpll(i915, pll, i)
> +		verify_single_dpll_state(i915, pll, NULL, NULL);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index b8dce4e9ac54..d419f4f98d28 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -36,6 +36,10 @@
>  	(void) (&__a == &__b);			\
>  	__a > __b ? (__a - __b) : (__b - __a); })
>  
> +#define for_each_shared_dpll(__i915, __pll, __i) \
> +	for ((__i) = 0; (__i) < (__i915)->display.dpll.num_shared_dpll && \
> +		     ((__pll) = &(__i915)->display.dpll.shared_dplls[(__i)]); (__i)++)
> +
>  enum tc_port;
>  struct drm_i915_private;
>  struct intel_atomic_state;
> diff --git a/drivers/gpu/drm/i915/display/intel_pch_refclk.c b/drivers/gpu/drm/i915/display/intel_pch_refclk.c
> index 4db4b8d57e21..bb4e70aa9f6e 100644
> --- a/drivers/gpu/drm/i915/display/intel_pch_refclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_pch_refclk.c
> @@ -497,6 +497,7 @@ static void lpt_init_pch_refclk(struct drm_i915_private *dev_priv)
>  static void ilk_init_pch_refclk(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_encoder *encoder;
> +	struct intel_shared_dpll *pll;
>  	int i;
>  	u32 val, final;
>  	bool has_lvds = false;
> @@ -532,8 +533,7 @@ static void ilk_init_pch_refclk(struct drm_i915_private *dev_priv)
>  	}
>  
>  	/* Check if any DPLLs are using the SSC source */
> -	for (i = 0; i < dev_priv->display.dpll.num_shared_dpll; i++) {
> -		struct intel_shared_dpll *pll = &dev_priv->display.dpll.shared_dplls[i];
> +	for_each_shared_dpll(dev_priv, pll, i) {
>  		u32 temp;
>  
>  		temp = intel_de_read(dev_priv, PCH_DPLL(pll->info->id));

-- 
Jani Nikula, Intel Open Source Graphics Center