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

Daniel Vetter
2016-10-13T13:24:58Z

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