Re: [PATCH 02/42] drm/i915: get rid of intel_crtc_disable and related code, v2

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 11, 2015 at 04:24:38PM +0200, Maarten Lankhorst wrote:
> Now that the dpll updates are (mostly) atomic, the .off() code is no longer used,
> and there are no more callers for intel_put_shared_dpll. Move all the updates
> done in intel_crtc_disable to intel_modeset_update_state, one less special case
> to worry about.

The commit message is a bit innaccurate: We do still have callers of
put_shared_dpll, but that work is superseeded by
intel_shared_dpll_start_config. Hopefully.

Because of that I think this should be two patches: First one to nuke
put_shared_dpll, second one to inline crtc_disable.

One comment below.

> 
> Changes since v1:
> - Move drm_atomic_helper_swap_state up.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   1 -
>  drivers/gpu/drm/i915/intel_display.c | 106 +++++++----------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 -
>  3 files changed, 19 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b3142a99171..a6d3ab94ec15 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -574,7 +574,6 @@ struct drm_i915_display_funcs {
>  				  struct intel_crtc_state *crtc_state);
>  	void (*crtc_enable)(struct drm_crtc *crtc);
>  	void (*crtc_disable)(struct drm_crtc *crtc);
> -	void (*off)(struct drm_crtc *crtc);
>  	void (*audio_codec_enable)(struct drm_connector *connector,
>  				   struct intel_encoder *encoder,
>  				   struct drm_display_mode *mode);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5543f0bcda9..af96d686aae2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4228,27 +4228,6 @@ static void lpt_pch_enable(struct drm_crtc *crtc)
>  	lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
>  }
>  
> -void intel_put_shared_dpll(struct intel_crtc *crtc)
> -{
> -	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
> -
> -	if (pll == NULL)
> -		return;
> -
> -	if (!(pll->config.crtc_mask & (1 << crtc->pipe))) {
> -		WARN(1, "bad %s crtc mask\n", pll->name);
> -		return;
> -	}
> -
> -	pll->config.crtc_mask &= ~(1 << crtc->pipe);
> -	if (pll->config.crtc_mask == 0) {
> -		WARN_ON(pll->on);
> -		WARN_ON(pll->active);
> -	}

We're dropping the edge-detection warnings here. But I think the code in
check_shared_dpll_state should be paranoid enough to keep us covered.
Worth mentioning in the commit message though.
-Daniel

> -
> -	crtc->config->shared_dpll = DPLL_ID_PRIVATE;
> -}
> -
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  						struct intel_crtc_state *crtc_state)
>  {
> @@ -5224,13 +5203,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  		intel_disable_shared_dpll(intel_crtc);
>  }
>  
> -static void ironlake_crtc_off(struct drm_crtc *crtc)
> -{
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	intel_put_shared_dpll(intel_crtc);
> -}
> -
> -
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -6062,10 +6034,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -static void i9xx_crtc_off(struct drm_crtc *crtc)
> -{
> -}
> -
>  /* Master function to enable/disable CRTC and corresponding power wells */
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  {
> @@ -6115,34 +6083,6 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
>  	crtc->state->active = enable;
>  }
>  
> -static void intel_crtc_disable(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_connector *connector;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	/* crtc should still be enabled when we disable it. */
> -	WARN_ON(!crtc->state->enable);
> -
> -	intel_crtc_disable_planes(crtc);
> -	dev_priv->display.crtc_disable(crtc);
> -	dev_priv->display.off(crtc);
> -
> -	drm_plane_helper_disable(crtc->primary);
> -
> -	/* Update computed state. */
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		if (!connector->encoder || !connector->encoder->crtc)
> -			continue;
> -
> -		if (connector->encoder->crtc != crtc)
> -			continue;
> -
> -		connector->dpms = DRM_MODE_DPMS_OFF;
> -		to_intel_encoder(connector->encoder)->connectors_active = false;
> -	}
> -}
> -
>  void intel_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> @@ -11614,26 +11554,22 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> -	int i;
>  
>  	intel_shared_dpll_commit(dev_priv);
> +	drm_atomic_helper_swap_state(state->dev, state);
>  
>  	for_each_intel_encoder(dev, intel_encoder) {
>  		if (!intel_encoder->base.crtc)
>  			continue;
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, i)
> -			if (crtc == intel_encoder->base.crtc)
> -				break;
> -
> -		if (crtc != intel_encoder->base.crtc)
> +		crtc = intel_encoder->base.crtc;
> +		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
> +		if (!crtc_state || !needs_modeset(crtc->state))
>  			continue;
>  
> -		if (crtc_state->enable && needs_modeset(crtc_state))
> -			intel_encoder->connectors_active = false;
> +		intel_encoder->connectors_active = false;
>  	}
>  
> -	drm_atomic_helper_swap_state(state->dev, state);
>  	intel_modeset_fixup_state(state);
>  
>  	/* Double check state. */
> @@ -11645,14 +11581,12 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		if (!connector->encoder || !connector->encoder->crtc)
>  			continue;
>  
> -		for_each_crtc_in_state(state, crtc, crtc_state, i)
> -			if (crtc == connector->encoder->crtc)
> -				break;
> -
> -		if (crtc != connector->encoder->crtc)
> +		crtc = connector->encoder->crtc;
> +		crtc_state = state->crtc_states[drm_crtc_index(crtc)];
> +		if (!crtc_state || !needs_modeset(crtc->state))
>  			continue;
>  
> -		if (crtc->state->enable && needs_modeset(crtc->state)) {
> +		if (crtc->state->enable) {
>  			struct drm_property *dpms_property =
>  				dev->mode_config.dpms_property;
>  
> @@ -11663,7 +11597,8 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  
>  			intel_encoder = to_intel_encoder(connector->encoder);
>  			intel_encoder->connectors_active = true;
> -		}
> +		} else
> +			connector->dpms = DRM_MODE_DPMS_OFF;
>  	}
>  
>  }
> @@ -12214,6 +12149,10 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	if (IS_ERR(pipe_config))
>  		return pipe_config;
>  
> +	if (!pipe_config->base.enable &&
> +	    WARN_ON(pipe_config->base.active))
> +		pipe_config->base.active = false;
> +
>  	if (!pipe_config->base.enable)
>  		return pipe_config;
>  
> @@ -12341,12 +12280,10 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> -		if (!crtc_state->enable) {
> -			intel_crtc_disable(crtc);
> -		} else if (crtc->state->enable) {
> -			intel_crtc_disable_planes(crtc);
> -			dev_priv->display.crtc_disable(crtc);
> -		}
> +		intel_crtc_disable_planes(crtc);
> +		dev_priv->display.crtc_disable(crtc);
> +		if (!crtc_state->enable)
> +			drm_plane_helper_disable(crtc->primary);
>  	}
>  
>  	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> @@ -14064,7 +14001,6 @@ static void intel_init_display(struct drm_device *dev)
>  			haswell_crtc_compute_clock;
>  		dev_priv->display.crtc_enable = haswell_crtc_enable;
>  		dev_priv->display.crtc_disable = haswell_crtc_disable;
> -		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			skylake_update_primary_plane;
>  	} else if (HAS_DDI(dev)) {
> @@ -14075,7 +14011,6 @@ static void intel_init_display(struct drm_device *dev)
>  			haswell_crtc_compute_clock;
>  		dev_priv->display.crtc_enable = haswell_crtc_enable;
>  		dev_priv->display.crtc_disable = haswell_crtc_disable;
> -		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			ironlake_update_primary_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> @@ -14086,7 +14021,6 @@ static void intel_init_display(struct drm_device *dev)
>  			ironlake_crtc_compute_clock;
>  		dev_priv->display.crtc_enable = ironlake_crtc_enable;
>  		dev_priv->display.crtc_disable = ironlake_crtc_disable;
> -		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			ironlake_update_primary_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
> @@ -14096,7 +14030,6 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> -		dev_priv->display.off = i9xx_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			i9xx_update_primary_plane;
>  	} else {
> @@ -14106,7 +14039,6 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> -		dev_priv->display.off = i9xx_crtc_off;
>  		dev_priv->display.update_primary_plane =
>  			i9xx_update_primary_plane;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 29262228f264..4c690403942c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,7 +1082,6 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  #define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false)
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  						struct intel_crtc_state *state);
> -void intel_put_shared_dpll(struct intel_crtc *crtc);
>  
>  void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
>  		      const struct dpll *dpll);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx