Re: [PATCH 08/42] drm/i915: Implement intel_crtc_toggle using atomic state

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 11, 2015 at 04:24:44PM +0200, Maarten Lankhorst wrote:
> Assume the function is locked with drm_modeset_lock_all for now.

s/toggle/control/ in the commit message.

And a bit of blabla would be good to explain why we need to make a
mass-replacement of state->enable to state->active here now. And looking
at the enable/active replacement in detail I think we should do this in an
upfront patch first (the state should be consistent already). And have the
reworking of intel_crtc_control in a separate split-out patch.

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++++++++--------------
>  2 files changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9da955e4f355..a6816503a080 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  		return -EINVAL;
>  	}
>  
> -	if (!crtc->state->enable) {
> +	if (!crtc->state->active) {
>  		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
>  		return -EBUSY;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c2fb951029b..a21b2e51c054 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c

One s/state->enable/state->active/ in modeset_update_crtc_power_domains
seems to be missing.

> @@ -4656,7 +4656,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	bool reenable_ips = false;
>  
>  	/* The clocks have to be on to load the palette. */
> -	if (!crtc->state->enable || !intel_crtc->active)
> +	if (!crtc->state->active || !intel_crtc->active)

Replace the check for intel_crtc->active with a WARN_ON?

>  		return;
>  
>  	if (HAS_GMCH_DISPLAY(dev_priv->dev)) {
> @@ -5767,7 +5767,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
>  
>  	/* add all active pipes to the state */
>  	for_each_crtc(state->dev, crtc) {
> -		if (!crtc->state->enable)
> +		if (!crtc->state->active)
>  			continue;
>  
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);

The second state->enable looks a bit funky, imo

	if (crtc_state->active)
		crtc_state->active_changed

after this patch would make a lot of sense. That way we won't try to frob
pipes which are already off.

> @@ -5865,7 +5865,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	bool is_dsi;
>  
> -	WARN_ON(!crtc->state->enable);
> +	WARN_ON(!crtc->state->active);

You don't update haswell/ironlake_crtc_enable, which is inconsistent.

>  
>  	if (intel_crtc->active)
>  		return;
> @@ -5943,7 +5943,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	WARN_ON(!crtc->state->enable);
> +	WARN_ON(!crtc->state->active);
>  
>  	if (intel_crtc->active)
>  		return;

intel_connector_check_state should gain a

			I915_STATE_WARN(!crtc->state->active,
					"crtc not active\n");

imo.

Similar for check_crtc_state:

		I915_STATE_WARN(active != crtc->state->active,
		     "crtc's computed active state doesn't match sw active state "
		     "(expected %i, found %i)\n", active, crtc->active);


> @@ -6054,10 +6054,13 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum intel_display_power_domain domain;
> -	unsigned long domains;
> +	struct intel_crtc_state *pipe_config;
> +	struct drm_plane_state *plane_state;
> +	struct drm_atomic_state *state;
> +	int ret;
>  
>  	if (enable == intel_crtc->active)
>  		return;
> @@ -6065,24 +6068,40 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  	if (enable && !crtc->state->enable)
>  		return;
>  
> -	crtc->state->active = enable;
> -	if (enable) {
> -		domains = get_crtc_power_domains(crtc);
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_get(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = domains;
> +	/* this function should be called with drm_modeset_lock_all for now */
> +	if (WARN_ON(!ctx))
> +		return;
> +	lockdep_assert_held(&ctx->ww_ctx);
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> -	} else {
> -		intel_crtc_disable_planes(crtc);
> -		dev_priv->display.crtc_disable(crtc);
> +	state = drm_atomic_state_alloc(dev);
> +	if (WARN_ON(!state))
> +		return;
>  
> -		domains = intel_crtc->enabled_power_domains;
> -		for_each_power_domain(domain, domains)
> -			intel_display_power_put(dev_priv, domain);
> -		intel_crtc->enabled_power_domains = 0;
> +	state->acquire_ctx = ctx;
> +	state->allow_modeset = true;
> +
> +	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto err;
>  	}
> +	pipe_config->base.active = enable;
> +
> +	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
> +	if (IS_ERR(plane_state)) {
> +		ret = PTR_ERR(plane_state);
> +		goto err;
> +	}
> +
> +	ret = intel_set_mode(crtc, state);
> +	if (!ret)
> +		return;
> +
> +	DRM_ERROR("Failed to toggle crtc!\n");
> +
> +err:
> +	DRM_ERROR("Updating crtc active failed with %i\n", ret);
> +	drm_atomic_state_free(state);
>  }
>  
>  /**
> @@ -6158,7 +6177,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  
>  			crtc = encoder->base.crtc;
>  
> -			I915_STATE_WARN(!crtc->state->enable,
> +			I915_STATE_WARN(!crtc->state->active,
>  					"crtc not enabled\n");
>  			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
>  			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
> @@ -11554,7 +11573,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		if (!crtc_state || !needs_modeset(crtc->state))
>  			continue;
>  
> -		if (crtc->state->enable) {
> +		if (crtc->state->active) {
>  			struct drm_property *dpms_property =
>  				dev->mode_config.dpms_property;
>  
> @@ -12018,7 +12037,7 @@ check_shared_dpll_state(struct drm_device *dev)
>  		     pll->on, active);
>  
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
> +			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
>  				enabled_crtcs++;
>  			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
>  				active_crtcs++;
> @@ -12185,7 +12204,7 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  		goto done;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!needs_modeset(crtc_state) || !crtc_state->enable)
> +		if (!needs_modeset(crtc_state) || !crtc_state->active)
>  			continue;
>  
>  		intel_crtc = to_intel_crtc(crtc);
> @@ -12265,7 +12284,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	 * pipes; here we assume a single modeset_pipe and only track the
>  	 * single crtc and mode.
>  	 */
> -	if (pipe_config->base.enable && needs_modeset(&pipe_config->base)) {
> +	if (pipe_config->base.active && needs_modeset(&pipe_config->base)) {
>  		modeset_crtc->mode = pipe_config->base.mode;
>  
>  		/*
> @@ -12290,7 +12309,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (!needs_modeset(crtc->state) || !crtc->state->enable)

Why did you drop this needs_modeset check here? Won't this lead to an
enabled pipe getting enabled again, which is troublesome?

> +		if (!crtc->state->active)
>  			continue;
>  
>  		update_scanline_offset(to_intel_crtc(crtc));

__intel_mode_set also seems to have some troublesome use of state->enable.
Or missing checks for state->active maybe, dunno.

> @@ -14516,7 +14535,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * have active connectors/encoders. */
>  	intel_crtc_update_dpms(&crtc->base);
>  
> -	if (crtc->active != crtc->base.state->enable) {
> +	if (crtc->active != crtc->base.state->active) {
>  		struct intel_encoder *encoder;
>  
>  		/* This can happen either due to bugs in the get_hw_state
> @@ -14665,7 +14684,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev,
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			*crtc_mask |= drm_crtc_index(&crtc->base);
>  
>  		crtc->active = dev_priv->display.get_pipe_config(crtc,

These two hunks look like follow-up or independant work to switch from
intel_crtc->active to state->active. Imo better separate.

Cheers, Daniel
> -- 
> 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