Re: [PATCH 08/13] drm/i915: Make shared dpll functions take crtc_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 03, 2018 at 03:37:10PM +0200, Maarten Lankhorst wrote:
> Do not rely on crtc->config any more. Remove the assertion from
> ibx_pch_dpll_disable, because we the dpll state tracking should
> already handle this case correctly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  | 14 ++++++------
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 31 +++++++++++----------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  6 +++---
>  3 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 57c81e8e44be..22f556b824f9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4749,7 +4749,7 @@ static void ironlake_pch_enable(const struct intel_atomic_state *state,
>  	 * Note that enable_shared_dpll tries to do the right thing, but
>  	 * get_shared_dpll unconditionally resets the pll - we need that to have
>  	 * the right LVDS enable sequence. */
> -	intel_enable_shared_dpll(crtc);
> +	intel_enable_shared_dpll(crtc_state);
>  
>  	/* set transcoder timing, panel must allow it */
>  	assert_panel_unlocked(dev_priv, pipe);
> @@ -5591,8 +5591,8 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
>  
> -	if (intel_crtc->config->has_pch_encoder)
> -		intel_prepare_shared_dpll(intel_crtc);
> +	if (pipe_config->has_pch_encoder)
> +		intel_prepare_shared_dpll(pipe_config);
>  
>  	if (intel_crtc_has_dp_encoder(intel_crtc->config))
>  		intel_dp_set_m_n(intel_crtc, M1_N1);
> @@ -5710,8 +5710,8 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>  
>  	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
>  
> -	if (intel_crtc->config->shared_dpll)
> -		intel_enable_shared_dpll(intel_crtc);
> +	if (pipe_config->shared_dpll)
> +		intel_enable_shared_dpll(pipe_config);
>  
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		icl_map_plls_to_ports(crtc, pipe_config, old_state);
> @@ -6286,7 +6286,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  
>  	intel_fbc_disable(intel_crtc);
>  	intel_update_watermarks(intel_crtc);
> -	intel_disable_shared_dpll(intel_crtc);
> +	intel_disable_shared_dpll(crtc_state);
>  
>  	domains = intel_crtc->enabled_power_domains;
>  	for_each_power_domain(domain, domains)
> @@ -12735,7 +12735,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  			dev_priv->display.crtc_disable(old_intel_crtc_state, state);
>  			intel_crtc->active = false;
>  			intel_fbc_disable(intel_crtc);
> -			intel_disable_shared_dpll(intel_crtc);
> +			intel_disable_shared_dpll(old_intel_crtc_state);
>  
>  			/*
>  			 * Underruns don't always raise
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e6cac9225536..54662c974c0b 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -131,13 +131,13 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>   * This calls the PLL's prepare hook if it has one and if the PLL is not
>   * already enabled. The prepare hook is platform specific.
>   */
> -void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> +void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>  
> -	if (WARN_ON(pll == NULL))
> +	if (!WARN_ON(!pll))
>  		return;
>  
>  	mutex_lock(&dev_priv->dpll_lock);
> @@ -158,11 +158,11 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>   *
>   * Enable the shared DPLL used by @crtc.
>   */
> -void intel_enable_shared_dpll(struct intel_crtc *crtc)
> +void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>  	unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
>  	unsigned int old_mask;
>  
> @@ -203,10 +203,11 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>   *
>   * Disable the shared DPLL used by @crtc.
>   */
> -void intel_disable_shared_dpll(struct intel_crtc *crtc)
> +void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_shared_dpll *pll = crtc->config->shared_dpll;
> +	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>  	unsigned int crtc_mask = drm_crtc_mask(&crtc->base);
>  
>  	/* PCH only available on ILK+ */
> @@ -409,14 +410,6 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv,
>  				 struct intel_shared_dpll *pll)
>  {
>  	const enum intel_dpll_id id = pll->info->id;
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_crtc *crtc;
> -
> -	/* Make sure no transcoder isn't still depending on us. */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->config->shared_dpll == pll)
> -			assert_pch_transcoder_disabled(dev_priv, crtc->pipe);
> -	}

Yeah I guess this can go. If we really wanted to keep it we should
probably make it not depend on any software state and just read out
everything from the hw.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	I915_WRITE(PCH_DPLL(id), 0);
>  	POSTING_READ(PCH_DPLL(id));
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index bf0de8a4dc63..9c033236f2ba 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -334,9 +334,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  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);
> +void intel_prepare_shared_dpll(const struct intel_crtc_state *crtc_state);
> +void intel_enable_shared_dpll(const struct intel_crtc_state *crtc_state);
> +void intel_disable_shared_dpll(const struct intel_crtc_state *crtc_state);
>  void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
>  void intel_shared_dpll_init(struct drm_device *dev);
>  
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx