Re: [PATCH 1/7] drm/i915: Introduce intel_release_shared_dpll()

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 04, 2016 at 03:32:11PM +0300, Ander Conselvan de Oliveira wrote:
> While the details of getting a shared dpll are wrapped by
> intel_get_shared_dpll(), the release was still hand rolled into the
> modeset code. Fix that by creating an entry point for releasing the
> pll and move that code there.
> 
> v2: Take old_dpll from crtc->state instead of crtc_state. (CI)
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  6 +----
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 41 +++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 11 +++-------
>  3 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a366656..28d9d3e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13768,7 +13768,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_shared_dpll_config *shared_dpll = NULL;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int i;
> @@ -13789,10 +13788,7 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  		if (!old_dpll)
>  			continue;
>  
> -		if (!shared_dpll)
> -			shared_dpll = intel_atomic_get_shared_dpll_state(state);
> -
> -		intel_shared_dpll_config_put(shared_dpll, old_dpll, intel_crtc);
> +		intel_release_shared_dpll(old_dpll, intel_crtc, state);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 1c59ca5..f1b3feb 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -79,28 +79,6 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
>  	return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
>  }
>  
> -void
> -intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
> -
> -	config[id].crtc_mask |= 1 << crtc->pipe;
> -}
> -
> -void
> -intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
> -
> -	config[id].crtc_mask &= ~(1 << crtc->pipe);
> -}
> -
>  /* For ILK+ */
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			struct intel_shared_dpll *pll,
> @@ -285,7 +263,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	intel_shared_dpll_config_get(shared_dpll, pll, crtc);
> +	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
>  }
>  
>  void intel_shared_dpll_commit(struct drm_atomic_state *state)
> @@ -1900,3 +1878,20 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
>  
>  	return dpll_mgr->get_dpll(crtc, crtc_state, encoder);
>  }
> +
> +/**
> + * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state
> + * @dpll: dpll in use by @crtc
> + * @crtc: crtc
> + * @state: atomic state
> + *
> + */
> +void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> +			       struct intel_crtc *crtc,
> +			       struct drm_atomic_state *state)
> +{
> +	struct intel_shared_dpll_config *shared_dpll_config;
> +
> +	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);

I wonder whether we shouldn't move intel_atomic_get_shared_dpll_state into
intel_dpll_mgr.c too. That would further reduce the interfaces exported
from intel_dpll_mgr.[hc].

On this patch: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index f438535..99a82c9 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -138,14 +138,6 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
>  enum intel_dpll_id
>  intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
>  			 struct intel_shared_dpll *pll);
> -void
> -intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc);
> -void
> -intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			struct intel_shared_dpll *pll,
>  			bool state);
> @@ -154,6 +146,9 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  						struct intel_crtc_state *state,
>  						struct intel_encoder *encoder);
> +void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> +			       struct intel_crtc *crtc,
> +			       struct drm_atomic_state *state);
>  void intel_prepare_shared_dpll(struct intel_crtc *crtc);
>  void intel_enable_shared_dpll(struct intel_crtc *crtc);
>  void intel_disable_shared_dpll(struct intel_crtc *crtc);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx