[10/42] drm/i915: make plane helpers fully atomic
diff mbox

Message ID 1431354318-11995-11-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst May 11, 2015, 2:24 p.m. UTC
This kills off most of the transitional helpers and uses atomic plane updates
in the modeset path to update everything.

Getting rid of the transitional plane helpers meant that planes had to be added
in the crtc check function. On modeset a connector can be moved to a different
crtc, and this is not handled correctly otherwise.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  59 ++-
 drivers/gpu/drm/i915/intel_display.c      | 655 ++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h          |   2 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  80 +---
 4 files changed, 441 insertions(+), 355 deletions(-)

Comments

Daniel Vetter May 12, 2015, 8:18 a.m. UTC | #1
On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> This kills off most of the transitional helpers and uses atomic plane updates
> in the modeset path to update everything.
> 
> Getting rid of the transitional plane helpers meant that planes had to be added
> in the crtc check function. On modeset a connector can be moved to a different
> crtc, and this is not handled correctly otherwise.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Ok pile of comments on this one. I don't yet fully grasp it all, but at
the very bottom I've jotted down a list of ideas for how to move forward
with this one.

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  59 ++-
>  drivers/gpu/drm/i915/intel_display.c      | 655 ++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h          |   2 +-
>  drivers/gpu/drm/i915/intel_sprite.c       |  80 +---
>  4 files changed, 441 insertions(+), 355 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 86ba4b2c3a65..85b87e4d4b6e 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  				    struct drm_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>  
> -	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
> +	intel_state->visible = false;
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>  	 * property while the plane is disabled.  We don't actually have
>  	 * anything driver-specific we need to test in that case, so
>  	 * just return success.
>  	 */
> -	if (!crtc)
> +	if (!crtc) {
> +		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
>  		return 0;
> +	}
> +
> +	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];

Please reuse drm_atomic_get_crtc_state, this deref magic is hard to read.
Maybe we should have a nofail variant of those to encode the below WARN_ON
even ...

> +	if (WARN_ON(!crtc_state))
> +		return 0;
> +
> +	if (!crtc_state->enable) {
> +		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
>  
> -	/* FIXME: temporary hack necessary while we still use the plane update
> -	 * helper. */
> -	if (state->state) {
> -		crtc_state =
> -			intel_atomic_get_crtc_state(state->state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> -	} else {
> -		crtc_state = intel_crtc->config;
> +		/*
> +		 * Probably allowed after converting to atomic. Right
> +		 * now it probably means we have the state confused.
> +		 */
> +		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);

This is already possible with the primary plane support - you can disable
it (though not change the mode at the same time).

> +		return 0;
> +	}
> +
> +	if (!crtc_state->active) {
> +		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
> +		return 0;
>  	}
>  
>  	/*
> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
> -	intel_state->clip.x2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> -	intel_state->clip.y2 =
> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> -
> -	/*
> -	 * Disabling a plane is always okay; we just need to update
> -	 * fb tracking in a special way since cleanup_fb() won't
> -	 * get called by the plane helpers.
> -	 */
> -	if (state->fb == NULL && plane->state->fb != NULL) {
> -		/*
> -		 * 'prepare' is never called when plane is being disabled, so
> -		 * we need to handle frontbuffer tracking as a special case
> -		 */
> -		intel_crtc->atomic.disabled_planes |=
> -			(1 << drm_plane_index(plane));
> -	}
> +	drm_crtc_get_hv_timing(&crtc_state->mode,
> +			       &intel_state->clip.x2,
> +			       &intel_state->clip.y2);

Imo this is obfuscating things a bit, why not just unconditionally copy
pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
most platforms must match the primary plane window except for gen2 and
gen9+.

>  
>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 956c9964275d..9610f76a2489 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -100,14 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
>  static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config);
> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *crtc_state);

I consider forward declarations evil, but given how much of a mess
intel_display.c is it doesn't really matter much ;-)

>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
>  static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>  			   int num_connectors);
> -static void intel_crtc_enable_planes(struct drm_crtc *crtc);
> -static void intel_crtc_disable_planes(struct drm_crtc *crtc);
> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
> +static void intel_post_enable_primary(struct drm_crtc *crtc);
>  
>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>  {
> @@ -2220,28 +2222,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
>  	POSTING_READ(reg);
>  }
>  
> -/**
> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
> - * @plane:  plane to be enabled
> - * @crtc: crtc for the plane
> - *
> - * Enable @plane on @crtc, making sure that the pipe is running first.
> - */
> -static void intel_enable_primary_hw_plane(struct drm_plane *plane,
> -					  struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> -	/* If the pipe isn't enabled, we can't pump pixels and may hang */
> -	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> -	to_intel_plane_state(plane->state)->visible = true;
> -
> -	dev_priv->display.update_primary_plane(crtc, plane->fb,
> -					       crtc->x, crtc->y);
> -}
> -
>  static bool need_vtd_wa(struct drm_device *dev)
>  {
>  #ifdef CONFIG_INTEL_IOMMU
> @@ -3161,11 +3141,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane_state *plane_state =
> +		to_intel_plane_state(crtc->primary->state);
> +	bool was_visible = plane_state->visible;
>  
> -	if (dev_priv->display.disable_fbc)
> +	/* Not supported right now by the helper, but lets be thorough. */
> +	if (was_visible && !fb)
> +		intel_pre_disable_primary(crtc);
> +	else if (was_visible && dev_priv->display.disable_fbc)
>  		dev_priv->display.disable_fbc(dev);
>  
> +	plane_state->visible = !!fb;
>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> +	if (!was_visible && fb)
> +		intel_post_enable_primary(crtc);

This contains a vblank_wait (at least for broadwell) which is a no-go in
set_base_atomic. Given that this is only used by kgdb and I'm not aware of
anyone using that and we also don't have any testcase for this code I
think the best would be to just not touch this and let it keep on
bitrotting ...

>  
>  	return 0;
>  }
> @@ -3192,16 +3181,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  		drm_modeset_lock(&crtc->mutex, NULL);
> -		/*
> -		 * FIXME: Once we have proper support for primary planes (and
> -		 * disabling them without disabling the entire crtc) allow again
> -		 * a NULL crtc->primary->fb.
> -		 */
> -		if (intel_crtc->active && crtc->primary->fb)
> +
> +		if (intel_crtc->active) {
> +			const struct intel_plane_state *state =
> +				to_intel_plane_state(crtc->primary->state);
> +
>  			dev_priv->display.update_primary_plane(crtc,
> -							       crtc->primary->fb,
> -							       crtc->x,
> -							       crtc->y);
> +							state->base.fb,
> +							state->src.x1 >> 16,
> +							state->src.y1 >> 16);
> +		}

I think the above hunk could be split out. And I'm not sure it's required
really, this is just to catch up on CS flips. Which won't be used any more
once we have atomic, but until then the legacy state stuff used here
should be good enough.

> +
>  		drm_modeset_unlock(&crtc->mutex);
>  	}
>  }
> @@ -4572,20 +4562,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>  	}
>  }
>  
> -static void intel_enable_sprite_planes(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> -	struct drm_plane *plane;
> -	struct intel_plane *intel_plane;
> -
> -	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> -		intel_plane = to_intel_plane(plane);
> -		if (intel_plane->pipe == pipe)
> -			intel_plane_restore(&intel_plane->base);
> -	}
> -}
> -
>  void hsw_enable_ips(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -4815,44 +4791,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  	hsw_disable_ips(intel_crtc);
>  }
>  
> -static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> -{
> -	intel_enable_primary_hw_plane(crtc->primary, crtc);
> -	intel_enable_sprite_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> -
> -	intel_post_enable_primary(crtc);
> -}
> -
> -static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	int pipe = intel_crtc->pipe;
> -
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	intel_pre_disable_primary(crtc);
> -
> -	intel_crtc_dpms_overlay_disable(intel_crtc);
> -	for_each_intel_plane(dev, intel_plane) {
> -		if (intel_plane->pipe == pipe) {
> -			struct drm_crtc *from = intel_plane->base.crtc;
> -
> -			intel_plane->disable_plane(&intel_plane->base,
> -						   from ?: crtc, true);
> -		}
> -	}
> -
> -	/*
> -	 * FIXME: Once we grow proper nuclear flip support out of this we need
> -	 * to compute the mask of flip planes precisely. For the time being
> -	 * consider this a flip to a NULL plane.
> -	 */
> -	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
> -}
> -
>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -11061,6 +10999,7 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.load_lut = intel_crtc_load_lut,
>  	.atomic_begin = intel_begin_crtc_commit,
>  	.atomic_flush = intel_finish_crtc_commit,
> +	.atomic_check = intel_atomic_check_crtc,
>  };
>  
>  /* Transitional helper to copy current connector/encoder state to
> @@ -11426,16 +11365,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>  	int i;
>  	bool retry = true;
>  
> -	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
> -		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!check_digital_port_conflicts(state)) {
> -		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> -		return -EINVAL;
> -	}
> -

I think it would help if you'd split out the addition of the crtc_check
function into a separate prep patch. At least I don't see a depency here
...

>  	clear_intel_crtc_state(pipe_config);
>  
>  	pipe_config->cpu_transcoder =
> @@ -11553,9 +11482,27 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_connector *connector;
> +	struct drm_connector_state *connector_state;
> +	int i;
>  
>  	intel_shared_dpll_commit(dev_priv);
> -	drm_atomic_helper_swap_state(state->dev, state);
> +
> +	/*
> +	 * swap crtc and connector state, plane state is already swapped in
> +	 * __intel_set_mode_update_planes. Once .crtc_disable is fixed
> +	 * all state should be swapped before disabling crtc's.
> +	 */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		crtc->state->state = state;
> +		swap(state->crtc_states[i], crtc->state);
> +		crtc->state->state = NULL;
> +	}
> +
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		connector->state->state = state;
> +		swap(state->connector_states[i], connector->state);
> +		connector->state->state = NULL;
> +	}
>  
>  	for_each_intel_encoder(dev, intel_encoder) {
>  		if (!intel_encoder->base.crtc)
> @@ -12163,8 +12110,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	    WARN_ON(pipe_config->base.active))
>  		pipe_config->base.active = false;
>  
> -	if (!pipe_config->base.enable)
> -		return pipe_config;
> +	if (!pipe_config->base.active)
> +		goto done;
>  
>  	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
>  	if (ret)
> @@ -12182,8 +12129,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  	 * required changes and forcing a mode set.
>  	 */
>  
> -	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
> -
> +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
> +done:
>  	ret = drm_atomic_helper_check_planes(state->dev, state);
>  	if (ret)
>  		return ERR_PTR(ret);
> @@ -12247,6 +12194,11 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	int ret;
>  
> +	if (!check_digital_port_conflicts(state)) {
> +		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * See if the config requires any additional preparation, e.g.
>  	 * to adjust global state with pipes off.  We need to do this
> @@ -12267,6 +12219,112 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void __intel_set_mode_update_planes(struct drm_device *dev,
> +					   struct drm_atomic_state *state)
> +{
> +	int i;
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +
> +	/*
> +	 * For now only swap plane state, will be replaced with a
> +	 * call to drm_atomic_helper_swap_state
> +	 */
> +	for_each_plane_in_state(state, plane, old_plane_state, i) {
> +		struct drm_plane *plane = state->planes[i];
> +
> +		if (!plane)
> +			continue;
> +
> +		plane->state->state = state;
> +		swap(state->plane_states[i], plane->state);
> +		plane->state->state = NULL;
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		const struct drm_crtc_helper_funcs *funcs;
> +
> +		funcs = crtc->helper_private;
> +
> +		if (!funcs || !funcs->atomic_begin)
> +			continue;
> +
> +		/* XXX: Hack because crtc state is not swapped */
> +		crtc->state->mode_changed = crtc_state->mode_changed;
> +		crtc->state->active_changed = crtc_state->active_changed;
> +
> +		DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i);
> +		funcs->atomic_begin(crtc);
> +	}
> +
> +	for_each_plane_in_state(state, plane, old_plane_state, i) {
> +		bool visible = to_intel_plane_state(plane->state)->visible;
> +		struct intel_plane *intel_plane = to_intel_plane(plane);
> +		const struct drm_plane_helper_funcs *funcs =
> +			plane->helper_private;
> +
> +		crtc = plane->state->crtc;
> +
> +		/* no point in disabling if already disabled */
> +		if (!to_intel_plane_state(old_plane_state)->visible &&
> +		    !to_intel_plane_state(old_plane_state)->hw_enabled)
> +			continue;
> +
> +		to_intel_plane_state(plane->state)->hw_enabled = false;
> +		DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
> +
> +		if (!visible)
> +			funcs->atomic_update(plane, old_plane_state);
> +		else if (needs_modeset(crtc->state))
> +			intel_plane->disable_plane(plane, crtc, true);
> +	}
> +}
> +
> +static void __intel_set_mode_cleanup_planes(struct drm_device *dev,
> +					    struct drm_atomic_state *old_state)
> +{
> +	int nplanes = dev->mode_config.num_total_plane;
> +	int ncrtcs = dev->mode_config.num_crtc;
> +	int i;
> +
> +	for (i = 0; i < nplanes; i++) {
> +		const struct drm_plane_helper_funcs *funcs;
> +		struct drm_plane *plane = old_state->planes[i];
> +		struct drm_plane_state *old_plane_state;
> +
> +		if (!plane)
> +			continue;
> +
> +		funcs = plane->helper_private;
> +		old_plane_state = old_state->plane_states[i];
> +
> +		if (to_intel_plane_state(plane->state)->visible) {
> +			DRM_DEBUG_ATOMIC("Plane %i is updated\n", i);
> +			funcs->atomic_update(plane, old_plane_state);
> +		} else
> +			DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i);
> +	}
> +
> +	for (i = 0; i < ncrtcs; i++) {
> +		const struct drm_crtc_helper_funcs *funcs;
> +		struct drm_crtc *crtc = old_state->crtcs[i];
> +
> +		if (!crtc)
> +			continue;
> +
> +		funcs = crtc->helper_private;
> +
> +		if (!funcs || !funcs->atomic_flush)
> +			continue;
> +
> +		DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i);
> +		funcs->atomic_flush(crtc);
> +	}
> +	drm_atomic_helper_cleanup_planes(dev, old_state);
> +}
> +
>  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			    struct intel_crtc_state *pipe_config)
>  {
> @@ -12275,7 +12333,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	struct drm_atomic_state *state = pipe_config->base.state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
> +	int ret;
>  	int i;
>  
>  	ret = __intel_set_mode_checks(state);
> @@ -12286,14 +12344,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  	if (ret)
>  		return ret;
>  
> +	__intel_set_mode_update_planes(dev, state);

This calls crtc->atomic_begin, which does an irq_disable ...

> +
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> -		intel_crtc_disable_planes(crtc);
> +		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
>  		dev_priv->display.crtc_disable(crtc);
> -		if (!crtc_state->enable)
> -			drm_plane_helper_disable(crtc->primary);
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> @@ -12305,8 +12363,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  	modeset_update_crtc_power_domains(state);
>  
> -	drm_atomic_helper_commit_planes(dev, state);
> -
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (!crtc->state->active)
> @@ -12314,13 +12370,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  
>  		update_scanline_offset(to_intel_crtc(crtc));
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> +		if (needs_modeset(crtc->state))
> +			dev_priv->display.crtc_enable(crtc);
>  	}
>  
> -	/* FIXME: add subpixel order */
> +	__intel_set_mode_cleanup_planes(dev, state);

which is only cleared in atomic_flush here. We need both of these at the
bottom here. In atomic-helpers-speak the sequence should be

drm_atomic_helper_commit_modeset_disables();
drm_atomic_helper_commit_modeset_enables();
drm_atomic_helper_commit_planes();

with the note that our crtc_disable must do the unconditional plane
disabling itself. I.e. you can't just remove disable_planes without
replacement, that one needs to be open-coded (using plane->atomic_disable
if possible).

Doing things this way would also alleviate any need to split up swap_state
as you do in this patch here.

>  
> -	drm_atomic_helper_cleanup_planes(dev, state);

Please keep this separate and don't hide it in another function, it's a
distinct step from the hw update (which is now done in the misnamed
intel_set_mode_cleanup_planes).

> +	/* FIXME: add subpixel order */
>  
>  	drm_atomic_state_free(state);
>  
> @@ -12568,20 +12624,11 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static bool primary_plane_visible(struct drm_crtc *crtc)
> -{
> -	struct intel_plane_state *plane_state =
> -		to_intel_plane_state(crtc->primary->state);
> -
> -	return plane_state->visible;
> -}
> -
>  static int intel_crtc_set_config(struct drm_mode_set *set)
>  {
>  	struct drm_device *dev;
>  	struct drm_atomic_state *state = NULL;
>  	struct intel_crtc_state *pipe_config;
> -	bool primary_plane_was_visible;
>  	int ret;
>  
>  	BUG_ON(!set);
> @@ -12620,38 +12667,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
>  
> -	primary_plane_was_visible = primary_plane_visible(set->crtc);
> -
>  	ret = intel_set_mode_with_config(set->crtc, pipe_config);
> -
> -	if (ret == 0 &&
> -	    pipe_config->base.enable &&
> -	    pipe_config->base.planes_changed &&
> -	    !needs_modeset(&pipe_config->base)) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> -
> -		/*
> -		 * We need to make sure the primary plane is re-enabled if it
> -		 * has previously been turned off.
> -		 */
> -		if (ret == 0 && !primary_plane_was_visible &&
> -		    primary_plane_visible(set->crtc)) {
> -			WARN_ON(!intel_crtc->active);
> -			intel_post_enable_primary(set->crtc);
> -		}
> -
> -		/*
> -		 * In the fastboot case this may be our only check of the
> -		 * state after boot.  It would be better to only do it on
> -		 * the first update, but we don't have a nice way of doing that
> -		 * (and really, set_config isn't used much for high freq page
> -		 * flipping, so increasing its cost here shouldn't be a big
> -		 * deal).
> -		 */
> -		if (i915.fastboot && ret == 0)
> -			intel_modeset_check_state(set->crtc->dev);
> -	}
> -
>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>  			      set->crtc->base.id, ret);
> @@ -12791,6 +12807,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
>  	    plane->state->rotation != state->rotation)
>  		return true;
>  
> +	if (plane->state->crtc_w != state->crtc_w)
> +		return true;
> +
>  	return false;
>  }
>  
> @@ -12819,6 +12838,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	unsigned frontbuffer_bits = 0;
>  	int ret = 0;
>  
> +	if (!to_intel_plane_state(plane->state)->visible)
> +		old_obj = NULL;
> +
>  	if (!obj)
>  		return 0;
>  
> @@ -12915,10 +12937,8 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc = state->base.crtc;
>  	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	struct drm_rect *dest = &state->dst;
>  	struct drm_rect *src = &state->src;
> @@ -12926,90 +12946,46 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	bool can_position = false;
>  	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
>  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
> -	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
> -	crtc_state = state->base.state ?
> -		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> +		struct intel_crtc_state *crtc_state =
> +			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
> +
>  		min_scale = 1;
>  		max_scale = skl_max_scale(intel_crtc, crtc_state);
>  		can_position = true;
>  	}
>  
> -	ret = drm_plane_helper_check_update(plane, crtc, fb,
> -					    src, dest, clip,
> -					    min_scale,
> -					    max_scale,
> -					    can_position, true,
> -					    &state->visible);
> -	if (ret)
> -		return ret;
> -
> -	if (intel_crtc->active) {
> -		struct intel_plane_state *old_state =
> -			to_intel_plane_state(plane->state);
> -
> -		intel_crtc->atomic.wait_for_flips = true;
> -
> -		/*
> -		 * FBC does not work on some platforms for rotated
> -		 * planes, so disable it when rotation is not 0 and
> -		 * update it when rotation is set back to 0.
> -		 *
> -		 * FIXME: This is redundant with the fbc update done in
> -		 * the primary plane enable function except that that
> -		 * one is done too late. We eventually need to unify
> -		 * this.
> -		 */
> -		if (state->visible &&
> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> -		    dev_priv->fbc.crtc == intel_crtc &&
> -		    state->base.rotation != BIT(DRM_ROTATE_0)) {
> -			intel_crtc->atomic.disable_fbc = true;
> -		}
> -
> -		if (state->visible && !old_state->visible) {
> -			/*
> -			 * BDW signals flip done immediately if the plane
> -			 * is disabled, even if the plane enable is already
> -			 * armed to occur at the next vblank :(
> -			 */
> -			if (IS_BROADWELL(dev))
> -				intel_crtc->atomic.wait_vblank = true;
> -		}
> -
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> -
> -		intel_crtc->atomic.update_fbc = true;
> -
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> -	}
> +	return drm_plane_helper_check_update(plane, crtc, fb,
> +					     src, dest, clip,
> +					     min_scale, max_scale,
> +					     can_position, true,
> +					     &state->visible);
> +}
>  
> -	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state,
> -			to_intel_plane(plane), state, 0);
> -		if (ret)
> -			return ret;
> -	}
> +static void
> +intel_disable_primary_plane(struct drm_plane *plane,
> +			    struct drm_crtc *crtc,
> +			    bool force)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	return 0;
> +	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
>  }
>  
>  static void
>  intel_commit_primary_plane(struct drm_plane *plane,
> -			   struct intel_plane_state *state)
> +			   struct intel_plane_state *new_state)
>  {
> -	struct drm_crtc *crtc = state->base.crtc;
> -	struct drm_framebuffer *fb = state->base.fb;
> +	struct drm_crtc *crtc = new_state->base.crtc;
> +	struct drm_framebuffer *fb = new_state->base.fb;
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	struct drm_rect *src = &state->src;
> +	struct drm_rect *src = &new_state->src;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
> @@ -13018,25 +12994,178 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	crtc->x = src->x1 >> 16;
>  	crtc->y = src->y1 >> 16;
>  
> -	if (intel_crtc->active) {
> -		if (state->visible)
> -			/* FIXME: kill this fastboot hack */
> -			intel_update_pipe_size(intel_crtc);
> +	if (!new_state->visible ||
> +	    WARN_ON(new_state->visible && !crtc->state->active)) {
> +		intel_disable_primary_plane(plane, crtc, false);
> +	} else {
> +		/* FIXME: kill this fastboot hack */
> +		intel_update_pipe_size(intel_crtc);
>  
> -		dev_priv->display.update_primary_plane(crtc, plane->fb,
> -						       crtc->x, crtc->y);
> +		dev_priv->display.update_primary_plane(crtc, fb,
> +						       src->x1 >> 16,
> +						       src->y1 >> 16);
>  	}
>  }
>  
> -static void
> -intel_disable_primary_plane(struct drm_plane *plane,
> -			    struct drm_crtc *crtc,
> -			    bool force)
> +/* Transitional checking here, mostly for plane updates */
> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> +				   struct drm_crtc_state *crtc_state)
>  {
> -	struct drm_device *dev = plane->dev;
> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_plane *plane;
> +	unsigned plane_mask;
> +	int idx, ret;
> +	bool mode_changed = needs_modeset(crtc_state);
> +	bool is_crtc_enabled = crtc_state->active;
> +	bool was_crtc_enabled = crtc->state->active;
>  
> -	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
> +	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
> +		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> +		return -EINVAL;
> +	}

I think we need two have 2 blocks in the crtc_check function: One part
that just updates watermarks (when state->plane_changed is set) and one
part that deals with the modeset checks (adding planes, checking cloning
and all that) when needs_modeset(state) is true.

> +
> +	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> +	intel_crtc->atomic.update_wm = mode_changed;
> +
> +	idx = crtc->base.id;
> +	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> +		"Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
> +		idx, crtc->state->active, intel_crtc->active);

State computation is async, you cant check this in the atomic_check
functions. If you think this is useful then we'd need to check this every
time before we update intel_crtc->active. But we have the various WARN_ON
all over the place (converted over to crtc->state->active) so I think
we're more than covered and can just rip out our own intel_crtc->active
eventually. At least that's kinda been my idea with state->active too.

> +
> +	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> +			 idx, was_crtc_enabled, is_crtc_enabled);
> +
> +	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> +		int i = drm_plane_index(plane);
> +		struct drm_plane_state *plane_state = state->plane_states[i];
> +		struct intel_plane_state *old_plane_state;
> +		bool turn_off, turn_on, visible, was_visible;
> +		struct drm_framebuffer *fb;
> +
> +		if (!plane_state) {
> +			const struct drm_plane_helper_funcs *funcs;
> +			int ret;
> +
> +			if (!mode_changed || !plane->state->fb)
> +				continue;
> +
> +			plane_state = drm_atomic_get_plane_state(state, plane);

We need to check for the crtc constrains before acquiring the plane state,
otherwise all the locking goes single-threaded. On intel hw we can do that
because the plane->crtc links are static, you can look at
intel_plane->pipe.

> +			if (IS_ERR(plane_state))
> +				return PTR_ERR(plane_state);
> +
> +			funcs = plane->helper_private;
> +			ret = funcs->atomic_check(plane, plane_state);
> +			if (ret)
> +				return ret;
> +		}
> +		old_plane_state = to_intel_plane_state(plane->state);
> +
> +		was_visible = was_crtc_enabled && (old_plane_state->visible ||
> +			      old_plane_state->hw_enabled);
> +		visible = to_intel_plane_state(plane_state)->visible &&
> +			      is_crtc_enabled;
> +
> +		if (plane->state->crtc != crtc)
> +			was_visible = false;
> +		if (plane_state->crtc != crtc)
> +			visible = false;
> +
> +		if (!was_visible && !visible)
> +			continue;
> +
> +		turn_off = was_visible && (!visible || mode_changed);
> +		turn_on = visible && (!was_visible || mode_changed);
> +		fb = plane_state->fb;
> +
> +		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
> +			plane->base.id, fb ? fb->base.id : -1);
> +		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
> +			was_visible, visible, turn_off, turn_on, mode_changed);
> +
> +		/* plane being turned off as part of modeset or changes? */
> +		if (intel_wm_need_update(plane, plane_state))
> +			intel_crtc->atomic.update_wm = true;
> +
> +		if (INTEL_INFO(dev)->gen >= 9 &&
> +		    plane->base.type != DRM_PLANE_TYPE_CURSOR) {
> +			ret = skl_update_scaler_users(intel_crtc, pipe_config,
> +					to_intel_plane(plane),
> +					to_intel_plane_state(plane_state), 0);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking as a special case
> +		 */
> +		if (old_plane_state->base.fb && !visible)
> +			intel_crtc->atomic.disabled_planes |= 1 << i;
> +
> +		switch (plane->base.type) {

We should probably store the frontbuffer tracking mask in intel_plane
somewhere.

> +		case DRM_PLANE_TYPE_PRIMARY:
> +			if (visible)
> +				intel_crtc->atomic.fb_bits |=
> +				    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> +
> +			intel_crtc->atomic.wait_for_flips = true;
> +			intel_crtc->atomic.pre_disable_primary = turn_off;
> +			intel_crtc->atomic.post_enable_primary = turn_on;
> +
> +			if (turn_off)
> +				intel_crtc->atomic.disable_fbc = true;
> +
> +			/*
> +			 * FBC does not work on some platforms for rotated
> +			 * planes, so disable it when rotation is not 0 and
> +			 * update it when rotation is set back to 0.
> +			 *
> +			 * FIXME: This is redundant with the fbc update done in
> +			 * the primary plane enable function except that that
> +			 * one is done too late. We eventually need to unify
> +			 * this.
> +			 */
> +
> +			if (visible &&
> +			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +			    dev_priv->fbc.crtc == intel_crtc &&
> +			    plane_state->rotation != BIT(DRM_ROTATE_0))
> +				intel_crtc->atomic.disable_fbc = true;
> +
> +			/*
> +			 * BDW signals flip done immediately if the plane
> +			 * is disabled, even if the plane enable is already
> +			 * armed to occur at the next vblank :(
> +			 */
> +			if (turn_on && IS_BROADWELL(dev))
> +				intel_crtc->atomic.wait_vblank = true;
> +
> +			intel_crtc->atomic.update_fbc = true;
> +			break;
> +		case DRM_PLANE_TYPE_CURSOR:
> +			if (visible)
> +				intel_crtc->atomic.fb_bits |=
> +				    INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> +			break;
> +		case DRM_PLANE_TYPE_OVERLAY:
> +			if (visible)
> +				intel_crtc->atomic.fb_bits |=
> +				    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> +
> +			if (turn_off) {
> +				intel_crtc->atomic.wait_vblank = true;
> +				intel_crtc->atomic.update_sprite_watermarks |=
> +					1 << i;
> +			}
> +			break;
> +		}

I'm completely lost as to why we need to move all the plane->atomic_check
code into a loop in the crtc_check function?

> +	}
> +	return 0;
>  }
>  
>  static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> @@ -13087,10 +13216,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (intel_crtc->active)
> +	if (intel_crtc->active && !needs_modeset(crtc->state) &&
> +	    !dev_priv->power_domains.init_power_on)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> +	else
> +		intel_crtc->atomic.evade = false;
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> @@ -13099,6 +13231,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *p;
> +	unsigned plane_mask;
>  
>  	if (intel_crtc->atomic.evade)
>  		intel_pipe_update_end(intel_crtc,
> @@ -13120,12 +13253,13 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>  	if (intel_crtc->atomic.post_enable_primary)
>  		intel_post_enable_primary(crtc);
>  
> -	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
> -		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
> -			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
> -						       false, false);
> +	plane_mask = intel_crtc->atomic.update_sprite_watermarks;
> +	drm_for_each_plane_mask(p, dev, plane_mask)
> +		intel_update_sprite_watermarks(p, crtc, 0, 0, 0, false, false);
>  
>  	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> +	crtc->state->mode_changed = false;
> +	crtc->state->active_changed = false;
>  }
>  
>  /**
> @@ -13238,13 +13372,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  	const struct drm_rect *clip = &state->clip;
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct intel_crtc *intel_crtc;
>  	unsigned stride;
>  	int ret;
>  
> -	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
>  	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    src, dest, clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> @@ -13256,7 +13386,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  
>  	/* if we want to turn off the cursor ignore width and height */
>  	if (!obj)
> -		goto finish;
> +		return 0;
>  
>  	/* Check for which cursor types we support */
>  	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
> @@ -13273,19 +13403,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  
>  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> -		ret = -EINVAL;
> -	}
> -
> -finish:
> -	if (intel_crtc->active) {
> -		if (plane->state->crtc_w != state->base.crtc_w)
> -			intel_crtc->atomic.update_wm = true;
> -
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
> +		return -EINVAL;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static void
> @@ -13306,20 +13427,26 @@ intel_disable_cursor_plane(struct drm_plane *plane,
>  
>  static void
>  intel_commit_cursor_plane(struct drm_plane *plane,
> -			  struct intel_plane_state *state)
> +			  struct intel_plane_state *new_state)
>  {
> -	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_crtc *crtc = new_state->base.crtc;
>  	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(new_state->base.fb);
>  	uint32_t addr;
>  
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> -	plane->fb = state->base.fb;
> -	crtc->cursor_x = state->base.crtc_x;
> -	crtc->cursor_y = state->base.crtc_y;
> +	plane->fb = new_state->base.fb;
> +	crtc->cursor_x = new_state->base.crtc_x;
> +	crtc->cursor_y = new_state->base.crtc_y;
> +
> +	if (!new_state->visible ||
> +	    WARN_ON(new_state->visible && !crtc->state->active)) {

As an aside: We must move over to compute ->visible irrespective of
->active. I haven't check whether you fix that later on already, but this
is important to be able to check wm constraints correctly. The
crtc_state->active handling should imo be done in the common code by
simply not calling any of the plane or crtc functions hw update if the
crtc is off.

Of course that means we need to add all planes both for ->mode_changed and
for ->active_changed, but needs_modeset(crtc_state) takes care of that
already.

Aside: The idea behind the split between drm_atomic_helper_check_planes
and drm_atomic_helper_check_modeset is that the former handles planes-only
updates while the latter handles modeset. We can't fully use
check_modesets because we don't use crtc helpers, but I think reusing the
same idea still has merit:

- We create an intel_check_modeset which calls the helper's check_modeset
  plus afterwards all the modeset checks (dpll, cloning checks, adding
  plane states and global resources) that we do in crtc_compute_config and
  friends right now. That one would also add plane states as needed (but
  not call plane->atomic_check). All of this would be skipped if
  needs_modeset isn't true.

- After that we'd call the helper's plane_check functions which does
  exclusively plane-update related checks. crtc/plane->atomic_check
  wouldn't contain any modeset related checks (like the encoder cloning or
  plane adding you're doing right now).

> +		intel_disable_cursor_plane(plane, crtc, false);
> +		return;
> +	}
>  
>  	if (intel_crtc->cursor_bo == obj)
>  		goto update;
> @@ -13333,10 +13460,9 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  
>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = obj;
> -update:
>  
> -	if (intel_crtc->active)
> -		intel_crtc_update_cursor(crtc, state->visible);
> +update:
> +	intel_crtc_update_cursor(crtc, new_state->visible);
>  }
>  
>  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> @@ -14695,7 +14821,14 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev,
>  		crtc->base.enabled = crtc->active;
>  
>  		plane_state = to_intel_plane_state(primary->state);
> -		plane_state->visible = primary_get_hw_state(crtc);
> +		plane_state->hw_enabled = plane_state->visible =
> +			primary_get_hw_state(crtc);
> +		if (plane_state->visible)
> +			crtc->base.state->plane_mask |=
> +				1 << drm_plane_index(primary);
> +		else
> +			crtc->base.state->plane_mask &=
> +				~(1 << drm_plane_index(primary));
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  			      crtc->base.base.id,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1e892098eea2..9ef89c91aa5c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -235,7 +235,7 @@ struct intel_plane_state {
>  	struct drm_rect src;
>  	struct drm_rect dst;
>  	struct drm_rect clip;
> -	bool visible;
> +	bool visible, hw_enabled;
>  
>  	/*
>  	 * scaler_id
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 497e7953ad4d..28291ab0993f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -759,7 +759,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  {
>  	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> -	struct intel_crtc_state *crtc_state;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -771,16 +770,9 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size;
> -	int ret;
> -
> -	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> -	crtc_state = state->base.state ?
> -		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
>  
> -	if (!fb) {
> -		state->visible = false;
> -		goto finish;
> -	}
> +	if (!fb)
> +		return 0;
>  
>  	/* Don't modify another pipe's plane */
>  	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -803,6 +795,9 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> +		struct intel_crtc_state *crtc_state =
> +			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
> +
>  		min_scale = 1;
>  		max_scale = skl_max_scale(intel_crtc, crtc_state);
>  	}
> @@ -816,7 +811,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(vscale < 0);
>  
> -	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> +	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>  
>  	crtc_x = dst->x1;
>  	crtc_y = dst->y1;
> @@ -921,36 +916,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	dst->y1 = crtc_y;
>  	dst->y2 = crtc_y + crtc_h;
>  
> -finish:
> -	/*
> -	 * If the sprite is completely covering the primary plane,
> -	 * we can disable the primary and save power.
> -	 */
> -	if (intel_crtc->active) {
> -		intel_crtc->atomic.fb_bits |=
> -			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
> -
> -		if (intel_wm_need_update(plane, &state->base))
> -			intel_crtc->atomic.update_wm = true;
> -
> -		if (!state->visible) {
> -			/*
> -			 * Avoid underruns when disabling the sprite.
> -			 * FIXME remove once watermark updates are done properly.
> -			 */
> -			intel_crtc->atomic.wait_vblank = true;
> -			intel_crtc->atomic.update_sprite_watermarks |=
> -				(1 << drm_plane_index(plane));
> -		}
> -	}
> -
> -	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
> -			state, 0);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -959,7 +924,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  			  struct intel_plane_state *state)
>  {
>  	struct drm_crtc *crtc = state->base.crtc;
> -	struct intel_crtc *intel_crtc;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	struct drm_framebuffer *fb = state->base.fb;
>  	int crtc_x, crtc_y;
> @@ -967,26 +931,22 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	uint32_t src_x, src_y, src_w, src_h;
>  
>  	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
> -
>  	plane->fb = fb;
>  
> -	if (intel_crtc->active) {
> -		if (state->visible) {
> -			crtc_x = state->dst.x1;
> -			crtc_y = state->dst.y1;
> -			crtc_w = drm_rect_width(&state->dst);
> -			crtc_h = drm_rect_height(&state->dst);
> -			src_x = state->src.x1 >> 16;
> -			src_y = state->src.y1 >> 16;
> -			src_w = drm_rect_width(&state->src) >> 16;
> -			src_h = drm_rect_height(&state->src) >> 16;
> -			intel_plane->update_plane(plane, crtc, fb,
> -						  crtc_x, crtc_y, crtc_w, crtc_h,
> -						  src_x, src_y, src_w, src_h);
> -		} else {
> -			intel_plane->disable_plane(plane, crtc, false);
> -		}
> +	if (state->visible && crtc->state->active) {
> +		crtc_x = state->dst.x1;
> +		crtc_y = state->dst.y1;
> +		crtc_w = drm_rect_width(&state->dst);
> +		crtc_h = drm_rect_height(&state->dst);
> +		src_x = state->src.x1 >> 16;
> +		src_y = state->src.y1 >> 16;
> +		src_w = drm_rect_width(&state->src) >> 16;
> +		src_h = drm_rect_height(&state->src) >> 16;
> +		intel_plane->update_plane(plane, crtc, fb,
> +					  crtc_x, crtc_y, crtc_w, crtc_h,
> +					  src_x, src_y, src_w, src_h);
> +	} else {
> +		intel_plane->disable_plane(plane, crtc, false);
>  	}
>  }

Ok I think overall this patch is too big and needs to be split a bit. The
following should be doable as prep-patches:
- set_base_atomic (if we dare to touch it at all)
- shuffling the crtc modeset check logic around to add plane states as
  needed, making sure we only run that if needs_modeset indicates so.
- Pushing the checks for intel_crtc->active or crtc->state->active out of
  the low-level plane code up into higher level atomic plane code (and the
  few legacy entry points for plane updates that we still have maybe, but
  those should be covered I think with the universal plane + transitional
  helpers).

Then we can put in the meat of this patch, i.e. replacing the helpers with
atomic updates in the modeset code.

Cheers, Daniel
Maarten Lankhorst May 12, 2015, 1:33 p.m. UTC | #2
Op 12-05-15 om 10:18 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
>> This kills off most of the transitional helpers and uses atomic plane updates
>> in the modeset path to update everything.
>>
>> Getting rid of the transitional plane helpers meant that planes had to be added
>> in the crtc check function. On modeset a connector can be moved to a different
>> crtc, and this is not handled correctly otherwise.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Ok pile of comments on this one. I don't yet fully grasp it all, but at
> the very bottom I've jotted down a list of ideas for how to move forward
> with this one.
>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  59 ++-
>>  drivers/gpu/drm/i915/intel_display.c      | 655 ++++++++++++++++++------------
>>  drivers/gpu/drm/i915/intel_drv.h          |   2 +-
>>  drivers/gpu/drm/i915/intel_sprite.c       |  80 +---
>>  4 files changed, 441 insertions(+), 355 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index 86ba4b2c3a65..85b87e4d4b6e 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  				    struct drm_plane_state *state)
>>  {
>>  	struct drm_crtc *crtc = state->crtc;
>> -	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>> +	struct drm_crtc_state *crtc_state;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
>>  
>> -	crtc = crtc ? crtc : plane->crtc;
>> -	intel_crtc = to_intel_crtc(crtc);
>> -
>> +	intel_state->visible = false;
>>  	/*
>>  	 * Both crtc and plane->crtc could be NULL if we're updating a
>>  	 * property while the plane is disabled.  We don't actually have
>>  	 * anything driver-specific we need to test in that case, so
>>  	 * just return success.
>>  	 */
>> -	if (!crtc)
>> +	if (!crtc) {
>> +		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
>>  		return 0;
>> +	}
>> +
>> +	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
> Please reuse drm_atomic_get_crtc_state, this deref magic is hard to read.
> Maybe we should have a nofail variant of those to encode the below WARN_ON
> even ...
Yeah, there are a few places where I use it like this, because in those cases the relevant state should already exist.
>> +	if (WARN_ON(!crtc_state))
>> +		return 0;
>> +
>> +	if (!crtc_state->enable) {
>> +		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
>>  
>> -	/* FIXME: temporary hack necessary while we still use the plane update
>> -	 * helper. */
>> -	if (state->state) {
>> -		crtc_state =
>> -			intel_atomic_get_crtc_state(state->state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> -	} else {
>> -		crtc_state = intel_crtc->config;
>> +		/*
>> +		 * Probably allowed after converting to atomic. Right
>> +		 * now it probably means we have the state confused.
>> +		 */
>> +		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);
> This is already possible with the primary plane support - you can disable
> it (though not change the mode at the same time).
That explains a few warnings, thanks. :-)
>> +		return 0;
>> +	}
>> +
>> +	if (!crtc_state->active) {
>> +		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
>> +		return 0;
>>  	}
>>  
>>  	/*
>> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
>>  	intel_state->clip.x1 = 0;
>>  	intel_state->clip.y1 = 0;
>> -	intel_state->clip.x2 =
>> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
>> -	intel_state->clip.y2 =
>> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
>> -
>> -	/*
>> -	 * Disabling a plane is always okay; we just need to update
>> -	 * fb tracking in a special way since cleanup_fb() won't
>> -	 * get called by the plane helpers.
>> -	 */
>> -	if (state->fb == NULL && plane->state->fb != NULL) {
>> -		/*
>> -		 * 'prepare' is never called when plane is being disabled, so
>> -		 * we need to handle frontbuffer tracking as a special case
>> -		 */
>> -		intel_crtc->atomic.disabled_planes |=
>> -			(1 << drm_plane_index(plane));
>> -	}
>> +	drm_crtc_get_hv_timing(&crtc_state->mode,
>> +			       &intel_state->clip.x2,
>> +			       &intel_state->clip.y2);
> Imo this is obfuscating things a bit, why not just unconditionally copy
> pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> most platforms must match the primary plane window except for gen2 and
> gen9+.
pipe_src_* is calculated in the same way, and at the time of plane validation the crtc validation hasn't run yet,
so pipe_src_* contains outdated values which break in interesting ways.

>>  
>>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 956c9964275d..9610f76a2489 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -100,14 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>>  			    const struct intel_crtc_state *pipe_config);
>>  static void chv_prepare_pll(struct intel_crtc *crtc,
>>  			    const struct intel_crtc_state *pipe_config);
>> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>> +				   struct drm_crtc_state *crtc_state);
> I consider forward declarations evil, but given how much of a mess
> intel_display.c is it doesn't really matter much ;-)
>
>>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
>>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
>>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
>>  	struct intel_crtc_state *crtc_state);
>>  static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
>>  			   int num_connectors);
>> -static void intel_crtc_enable_planes(struct drm_crtc *crtc);
>> -static void intel_crtc_disable_planes(struct drm_crtc *crtc);
>> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
>> +static void intel_post_enable_primary(struct drm_crtc *crtc);
>>  
>>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
>>  {
>> @@ -2220,28 +2222,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
>>  	POSTING_READ(reg);
>>  }
>>  
>> -/**
>> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
>> - * @plane:  plane to be enabled
>> - * @crtc: crtc for the plane
>> - *
>> - * Enable @plane on @crtc, making sure that the pipe is running first.
>> - */
>> -static void intel_enable_primary_hw_plane(struct drm_plane *plane,
>> -					  struct drm_crtc *crtc)
>> -{
>> -	struct drm_device *dev = plane->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -
>> -	/* If the pipe isn't enabled, we can't pump pixels and may hang */
>> -	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
>> -	to_intel_plane_state(plane->state)->visible = true;
>> -
>> -	dev_priv->display.update_primary_plane(crtc, plane->fb,
>> -					       crtc->x, crtc->y);
>> -}
>> -
>>  static bool need_vtd_wa(struct drm_device *dev)
>>  {
>>  #ifdef CONFIG_INTEL_IOMMU
>> @@ -3161,11 +3141,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_plane_state *plane_state =
>> +		to_intel_plane_state(crtc->primary->state);
>> +	bool was_visible = plane_state->visible;
>>  
>> -	if (dev_priv->display.disable_fbc)
>> +	/* Not supported right now by the helper, but lets be thorough. */
>> +	if (was_visible && !fb)
>> +		intel_pre_disable_primary(crtc);
>> +	else if (was_visible && dev_priv->display.disable_fbc)
>>  		dev_priv->display.disable_fbc(dev);
>>  
>> +	plane_state->visible = !!fb;
>>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
>> +	if (!was_visible && fb)
>> +		intel_post_enable_primary(crtc);
> This contains a vblank_wait (at least for broadwell) which is a no-go in
> set_base_atomic. Given that this is only used by kgdb and I'm not aware of
> anyone using that and we also don't have any testcase for this code I
> think the best would be to just not touch this and let it keep on
> bitrotting ...
Ok, I'll drop this hunk.

>>  
>>  	return 0;
>>  }
>> @@ -3192,16 +3181,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>>  		drm_modeset_lock(&crtc->mutex, NULL);
>> -		/*
>> -		 * FIXME: Once we have proper support for primary planes (and
>> -		 * disabling them without disabling the entire crtc) allow again
>> -		 * a NULL crtc->primary->fb.
>> -		 */
>> -		if (intel_crtc->active && crtc->primary->fb)
>> +
>> +		if (intel_crtc->active) {
>> +			const struct intel_plane_state *state =
>> +				to_intel_plane_state(crtc->primary->state);
>> +
>>  			dev_priv->display.update_primary_plane(crtc,
>> -							       crtc->primary->fb,
>> -							       crtc->x,
>> -							       crtc->y);
>> +							state->base.fb,
>> +							state->src.x1 >> 16,
>> +							state->src.y1 >> 16);
>> +		}
> I think the above hunk could be split out. And I'm not sure it's required
> really, this is just to catch up on CS flips. Which won't be used any more
> once we have atomic, but until then the legacy state stuff used here
> should be good enough.
?
>> +
>>  		drm_modeset_unlock(&crtc->mutex);
>>  	}
>>  }
>> @@ -4572,20 +4562,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
>>  	}
>>  }
>>  
>> -static void intel_enable_sprite_planes(struct drm_crtc *crtc)
>> -{
>> -	struct drm_device *dev = crtc->dev;
>> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>> -	struct drm_plane *plane;
>> -	struct intel_plane *intel_plane;
>> -
>> -	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>> -		intel_plane = to_intel_plane(plane);
>> -		if (intel_plane->pipe == pipe)
>> -			intel_plane_restore(&intel_plane->base);
>> -	}
>> -}
>> -
>>  void hsw_enable_ips(struct intel_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->base.dev;
>> @@ -4815,44 +4791,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>  	hsw_disable_ips(intel_crtc);
>>  }
>>  
>> -static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>> -{
>> -	intel_enable_primary_hw_plane(crtc->primary, crtc);
>> -	intel_enable_sprite_planes(crtc);
>> -	intel_crtc_update_cursor(crtc, true);
>> -
>> -	intel_post_enable_primary(crtc);
>> -}
>> -
>> -static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>> -{
>> -	struct drm_device *dev = crtc->dev;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_plane *intel_plane;
>> -	int pipe = intel_crtc->pipe;
>> -
>> -	intel_crtc_wait_for_pending_flips(crtc);
>> -
>> -	intel_pre_disable_primary(crtc);
>> -
>> -	intel_crtc_dpms_overlay_disable(intel_crtc);
>> -	for_each_intel_plane(dev, intel_plane) {
>> -		if (intel_plane->pipe == pipe) {
>> -			struct drm_crtc *from = intel_plane->base.crtc;
>> -
>> -			intel_plane->disable_plane(&intel_plane->base,
>> -						   from ?: crtc, true);
>> -		}
>> -	}
>> -
>> -	/*
>> -	 * FIXME: Once we grow proper nuclear flip support out of this we need
>> -	 * to compute the mask of flip planes precisely. For the time being
>> -	 * consider this a flip to a NULL plane.
>> -	 */
>> -	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>> -}
>> -
>>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> @@ -11061,6 +10999,7 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
>>  	.load_lut = intel_crtc_load_lut,
>>  	.atomic_begin = intel_begin_crtc_commit,
>>  	.atomic_flush = intel_finish_crtc_commit,
>> +	.atomic_check = intel_atomic_check_crtc,
>>  };
>>  
>>  /* Transitional helper to copy current connector/encoder state to
>> @@ -11426,16 +11365,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>>  	int i;
>>  	bool retry = true;
>>  
>> -	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
>> -		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (!check_digital_port_conflicts(state)) {
>> -		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>> -		return -EINVAL;
>> -	}
>> -
> I think it would help if you'd split out the addition of the crtc_check
> function into a separate prep patch. At least I don't see a depency here
> ...
Yeah but that would be the only thing put in there, oh well got to start somewhere...
>
>> @@ -12275,7 +12333,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>>  	struct drm_atomic_state *state = pipe_config->base.state;
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>> -	int ret = 0;
>> +	int ret;
>>  	int i;
>>  
>>  	ret = __intel_set_mode_checks(state);
>> @@ -12286,14 +12344,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>>  	if (ret)
>>  		return ret;
>>  
>> +	__intel_set_mode_update_planes(dev, state);
> This calls crtc->atomic_begin, which does an irq_disable ...
Yeah I tried to handle it by fixing all previous places to prevent atomic evading on modeset,
which means it is mostly safe to do it like this.

>
>> +
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		if (!needs_modeset(crtc_state))
>>  			continue;
>>  
>> -		intel_crtc_disable_planes(crtc);
>> +		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
>>  		dev_priv->display.crtc_disable(crtc);
>> -		if (!crtc_state->enable)
>> -			drm_plane_helper_disable(crtc->primary);
>>  	}
>>  
>>  	/* Only after disabling all output pipelines that will be changed can we
>> @@ -12305,8 +12363,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>>  
>>  	modeset_update_crtc_power_domains(state);
>>  
>> -	drm_atomic_helper_commit_planes(dev, state);
>> -
>>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		if (!crtc->state->active)
>> @@ -12314,13 +12370,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>>  
>>  		update_scanline_offset(to_intel_crtc(crtc));
>>  
>> -		dev_priv->display.crtc_enable(crtc);
>> -		intel_crtc_enable_planes(crtc);
>> +		if (needs_modeset(crtc->state))
>> +			dev_priv->display.crtc_enable(crtc);
>>  	}
>>  
>> -	/* FIXME: add subpixel order */
>> +	__intel_set_mode_cleanup_planes(dev, state);
> which is only cleared in atomic_flush here. We need both of these at the
> bottom here. In atomic-helpers-speak the sequence should be
>
> drm_atomic_helper_commit_modeset_disables();
> drm_atomic_helper_commit_modeset_enables();
> drm_atomic_helper_commit_planes();
>
> with the note that our crtc_disable must do the unconditional plane
> disabling itself. I.e. you can't just remove disable_planes without
> replacement, that one needs to be open-coded (using plane->atomic_disable
> if possible).
>
> Doing things this way would also alleviate any need to split up swap_state
> as you do in this patch here.
You're right it could be done like this, but doing commit_planes around the modesets allows
us to test the same code for calculating disable planes during modeset as when togging planes
separately. For example this is useful when a connector moves to a different crtc.

In any case this is cosmetic, and done for bisecting. I can move the swap_states in this commit,
but chose to do it 2 commits later for easier bisection.

I can't move it to before this commit because calling the plane helpers messes up the state.

>>  
>> -	drm_atomic_helper_cleanup_planes(dev, state);
> Please keep this separate and don't hide it in another function, it's a
> distinct step from the hw update (which is now done in the misnamed
> intel_set_mode_cleanup_planes).
Ok.
>
>> +	/* FIXME: add subpixel order */
>>  
>>  	drm_atomic_state_free(state);
>>  
>> @@ -12568,20 +12624,11 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>>  	return 0;
>>  }
>>  
>> -static bool primary_plane_visible(struct drm_crtc *crtc)
>> -{
>> -	struct intel_plane_state *plane_state =
>> -		to_intel_plane_state(crtc->primary->state);
>> -
>> -	return plane_state->visible;
>> -}
>> -
>>  static int intel_crtc_set_config(struct drm_mode_set *set)
>>  {
>>  	struct drm_device *dev;
>>  	struct drm_atomic_state *state = NULL;
>>  	struct intel_crtc_state *pipe_config;
>> -	bool primary_plane_was_visible;
>>  	int ret;
>>  
>>  	BUG_ON(!set);
>> @@ -12620,38 +12667,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>>  
>>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
>>  
>> -	primary_plane_was_visible = primary_plane_visible(set->crtc);
>> -
>>  	ret = intel_set_mode_with_config(set->crtc, pipe_config);
>> -
>> -	if (ret == 0 &&
>> -	    pipe_config->base.enable &&
>> -	    pipe_config->base.planes_changed &&
>> -	    !needs_modeset(&pipe_config->base)) {
>> -		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
>> -
>> -		/*
>> -		 * We need to make sure the primary plane is re-enabled if it
>> -		 * has previously been turned off.
>> -		 */
>> -		if (ret == 0 && !primary_plane_was_visible &&
>> -		    primary_plane_visible(set->crtc)) {
>> -			WARN_ON(!intel_crtc->active);
>> -			intel_post_enable_primary(set->crtc);
>> -		}
>> -
>> -		/*
>> -		 * In the fastboot case this may be our only check of the
>> -		 * state after boot.  It would be better to only do it on
>> -		 * the first update, but we don't have a nice way of doing that
>> -		 * (and really, set_config isn't used much for high freq page
>> -		 * flipping, so increasing its cost here shouldn't be a big
>> -		 * deal).
>> -		 */
>> -		if (i915.fastboot && ret == 0)
>> -			intel_modeset_check_state(set->crtc->dev);
>> -	}
>> -
>>  	if (ret) {
>>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>>  			      set->crtc->base.id, ret);
>> @@ -12791,6 +12807,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
>>  	    plane->state->rotation != state->rotation)
>>  		return true;
>>  
>> +	if (plane->state->crtc_w != state->crtc_w)
>> +		return true;
>> +
>>  	return false;
>>  }
>>  
>> @@ -12819,6 +12838,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>>  	unsigned frontbuffer_bits = 0;
>>  	int ret = 0;
>>  
>> +	if (!to_intel_plane_state(plane->state)->visible)
>> +		old_obj = NULL;
>> +
>>  	if (!obj)
>>  		return 0;
>>  
>> @@ -12915,10 +12937,8 @@ intel_check_primary_plane(struct drm_plane *plane,
>>  			  struct intel_plane_state *state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_crtc *crtc = state->base.crtc;
>>  	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>>  	struct drm_framebuffer *fb = state->base.fb;
>>  	struct drm_rect *dest = &state->dst;
>>  	struct drm_rect *src = &state->src;
>> @@ -12926,90 +12946,46 @@ intel_check_primary_plane(struct drm_plane *plane,
>>  	bool can_position = false;
>>  	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
>>  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
>> -	int ret;
>>  
>> -	crtc = crtc ? crtc : plane->crtc;
>>  	intel_crtc = to_intel_crtc(crtc);
>> -	crtc_state = state->base.state ?
>> -		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
>>  
>>  	if (INTEL_INFO(dev)->gen >= 9) {
>> +		struct intel_crtc_state *crtc_state =
>> +			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
>> +
>>  		min_scale = 1;
>>  		max_scale = skl_max_scale(intel_crtc, crtc_state);
>>  		can_position = true;
>>  	}
>>  
>> -	ret = drm_plane_helper_check_update(plane, crtc, fb,
>> -					    src, dest, clip,
>> -					    min_scale,
>> -					    max_scale,
>> -					    can_position, true,
>> -					    &state->visible);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (intel_crtc->active) {
>> -		struct intel_plane_state *old_state =
>> -			to_intel_plane_state(plane->state);
>> -
>> -		intel_crtc->atomic.wait_for_flips = true;
>> -
>> -		/*
>> -		 * FBC does not work on some platforms for rotated
>> -		 * planes, so disable it when rotation is not 0 and
>> -		 * update it when rotation is set back to 0.
>> -		 *
>> -		 * FIXME: This is redundant with the fbc update done in
>> -		 * the primary plane enable function except that that
>> -		 * one is done too late. We eventually need to unify
>> -		 * this.
>> -		 */
>> -		if (state->visible &&
>> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
>> -		    dev_priv->fbc.crtc == intel_crtc &&
>> -		    state->base.rotation != BIT(DRM_ROTATE_0)) {
>> -			intel_crtc->atomic.disable_fbc = true;
>> -		}
>> -
>> -		if (state->visible && !old_state->visible) {
>> -			/*
>> -			 * BDW signals flip done immediately if the plane
>> -			 * is disabled, even if the plane enable is already
>> -			 * armed to occur at the next vblank :(
>> -			 */
>> -			if (IS_BROADWELL(dev))
>> -				intel_crtc->atomic.wait_vblank = true;
>> -		}
>> -
>> -		intel_crtc->atomic.fb_bits |=
>> -			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>> -
>> -		intel_crtc->atomic.update_fbc = true;
>> -
>> -		if (intel_wm_need_update(plane, &state->base))
>> -			intel_crtc->atomic.update_wm = true;
>> -	}
>> +	return drm_plane_helper_check_update(plane, crtc, fb,
>> +					     src, dest, clip,
>> +					     min_scale, max_scale,
>> +					     can_position, true,
>> +					     &state->visible);
>> +}
>>  
>> -	if (INTEL_INFO(dev)->gen >= 9) {
>> -		ret = skl_update_scaler_users(intel_crtc, crtc_state,
>> -			to_intel_plane(plane), state, 0);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +static void
>> +intel_disable_primary_plane(struct drm_plane *plane,
>> +			    struct drm_crtc *crtc,
>> +			    bool force)
>> +{
>> +	struct drm_device *dev = plane->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	return 0;
>> +	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
>>  }
>>  
>>  static void
>>  intel_commit_primary_plane(struct drm_plane *plane,
>> -			   struct intel_plane_state *state)
>> +			   struct intel_plane_state *new_state)
>>  {
>> -	struct drm_crtc *crtc = state->base.crtc;
>> -	struct drm_framebuffer *fb = state->base.fb;
>> +	struct drm_crtc *crtc = new_state->base.crtc;
>> +	struct drm_framebuffer *fb = new_state->base.fb;
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc;
>> -	struct drm_rect *src = &state->src;
>> +	struct drm_rect *src = &new_state->src;
>>  
>>  	crtc = crtc ? crtc : plane->crtc;
>>  	intel_crtc = to_intel_crtc(crtc);
>> @@ -13018,25 +12994,178 @@ intel_commit_primary_plane(struct drm_plane *plane,
>>  	crtc->x = src->x1 >> 16;
>>  	crtc->y = src->y1 >> 16;
>>  
>> -	if (intel_crtc->active) {
>> -		if (state->visible)
>> -			/* FIXME: kill this fastboot hack */
>> -			intel_update_pipe_size(intel_crtc);
>> +	if (!new_state->visible ||
>> +	    WARN_ON(new_state->visible && !crtc->state->active)) {
>> +		intel_disable_primary_plane(plane, crtc, false);
>> +	} else {
>> +		/* FIXME: kill this fastboot hack */
>> +		intel_update_pipe_size(intel_crtc);
>>  
>> -		dev_priv->display.update_primary_plane(crtc, plane->fb,
>> -						       crtc->x, crtc->y);
>> +		dev_priv->display.update_primary_plane(crtc, fb,
>> +						       src->x1 >> 16,
>> +						       src->y1 >> 16);
>>  	}
>>  }
>>  
>> -static void
>> -intel_disable_primary_plane(struct drm_plane *plane,
>> -			    struct drm_crtc *crtc,
>> -			    bool force)
>> +/* Transitional checking here, mostly for plane updates */
>> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
>> +				   struct drm_crtc_state *crtc_state)
>>  {
>> -	struct drm_device *dev = plane->dev;
>> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct drm_atomic_state *state = crtc_state->state;
>> +	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_plane *plane;
>> +	unsigned plane_mask;
>> +	int idx, ret;
>> +	bool mode_changed = needs_modeset(crtc_state);
>> +	bool is_crtc_enabled = crtc_state->active;
>> +	bool was_crtc_enabled = crtc->state->active;
>>  
>> -	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
>> +	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
>> +		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
>> +		return -EINVAL;
>> +	}
> I think we need two have 2 blocks in the crtc_check function: One part
> that just updates watermarks (when state->plane_changed is set) and one
> part that deals with the modeset checks (adding planes, checking cloning
> and all that) when needs_modeset(state) is true.
Makes sense.

>> +
>> +	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
>> +	intel_crtc->atomic.update_wm = mode_changed;
>> +
>> +	idx = crtc->base.id;
>> +	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
>> +		"Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
>> +		idx, crtc->state->active, intel_crtc->active);
> State computation is async, you cant check this in the atomic_check
> functions. If you think this is useful then we'd need to check this every
> time before we update intel_crtc->active. But we have the various WARN_ON
> all over the place (converted over to crtc->state->active) so I think
> we're more than covered and can just rip out our own intel_crtc->active
> eventually. At least that's kinda been my idea with state->active too.
Yeah, it's a remainder when I had the commits in a different order.
>> +
>> +	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
>> +			 idx, was_crtc_enabled, is_crtc_enabled);
>> +
>> +	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
>> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
>> +		int i = drm_plane_index(plane);
>> +		struct drm_plane_state *plane_state = state->plane_states[i];
>> +		struct intel_plane_state *old_plane_state;
>> +		bool turn_off, turn_on, visible, was_visible;
>> +		struct drm_framebuffer *fb;
>> +
>> +		if (!plane_state) {
>> +			const struct drm_plane_helper_funcs *funcs;
>> +			int ret;
>> +
>> +			if (!mode_changed || !plane->state->fb)
>> +				continue;
>> +
>> +			plane_state = drm_atomic_get_plane_state(state, plane);
> We need to check for the crtc constrains before acquiring the plane state,
> otherwise all the locking goes single-threaded. On intel hw we can do that
> because the plane->crtc links are static, you can look at
> intel_plane->pipe.
This code is to make sure all planes are checked when mode_changed && plane->state->fb,
plane_state->visible needs to be updated in that case.

>> +			if (IS_ERR(plane_state))
>> +				return PTR_ERR(plane_state);
>> +
>> +			funcs = plane->helper_private;
>> +			ret = funcs->atomic_check(plane, plane_state);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		old_plane_state = to_intel_plane_state(plane->state);
>> +
>> +		was_visible = was_crtc_enabled && (old_plane_state->visible ||
>> +			      old_plane_state->hw_enabled);
>> +		visible = to_intel_plane_state(plane_state)->visible &&
>> +			      is_crtc_enabled;
>> +
>> +		if (plane->state->crtc != crtc)
>> +			was_visible = false;
>> +		if (plane_state->crtc != crtc)
>> +			visible = false;
>> +
>> +		if (!was_visible && !visible)
>> +			continue;
>> +
>> +		turn_off = was_visible && (!visible || mode_changed);
>> +		turn_on = visible && (!was_visible || mode_changed);
>> +		fb = plane_state->fb;
>> +
>> +		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
>> +			plane->base.id, fb ? fb->base.id : -1);
>> +		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
>> +			was_visible, visible, turn_off, turn_on, mode_changed);
>> +
>> +		/* plane being turned off as part of modeset or changes? */
>> +		if (intel_wm_need_update(plane, plane_state))
>> +			intel_crtc->atomic.update_wm = true;
>> +
>> +		if (INTEL_INFO(dev)->gen >= 9 &&
>> +		    plane->base.type != DRM_PLANE_TYPE_CURSOR) {
>> +			ret = skl_update_scaler_users(intel_crtc, pipe_config,
>> +					to_intel_plane(plane),
>> +					to_intel_plane_state(plane_state), 0);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		/*
>> +		 * 'prepare' is never called when plane is being disabled, so
>> +		 * we need to handle frontbuffer tracking as a special case
>> +		 */
>> +		if (old_plane_state->base.fb && !visible)
>> +			intel_crtc->atomic.disabled_planes |= 1 << i;
>> +
>> +		switch (plane->base.type) {
> We should probably store the frontbuffer tracking mask in intel_plane
> somewhere.

Indeed, but this code's already complicated enough so I felt that initially doing
a 1:1 moving from plane_check to crtc_check would be easier to debug.


>> <snip>
> I'm completely lost as to why we need to move all the plane->atomic_check
> code into a loop in the crtc_check function?
To calculate what needs to happen during atomic_begin and atomic_flush() on each crtc.

>> <snip>
>> @@ -13306,20 +13427,26 @@ intel_disable_cursor_plane(struct drm_plane *plane,
>>  
>>  static void
>>  intel_commit_cursor_plane(struct drm_plane *plane,
>> -			  struct intel_plane_state *state)
>> +			  struct intel_plane_state *new_state)
>>  {
>> -	struct drm_crtc *crtc = state->base.crtc;
>> +	struct drm_crtc *crtc = new_state->base.crtc;
>>  	struct drm_device *dev = plane->dev;
>>  	struct intel_crtc *intel_crtc;
>> -	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
>> +	struct drm_i915_gem_object *obj = intel_fb_obj(new_state->base.fb);
>>  	uint32_t addr;
>>  
>>  	crtc = crtc ? crtc : plane->crtc;
>>  	intel_crtc = to_intel_crtc(crtc);
>>  
>> -	plane->fb = state->base.fb;
>> -	crtc->cursor_x = state->base.crtc_x;
>> -	crtc->cursor_y = state->base.crtc_y;
>> +	plane->fb = new_state->base.fb;
>> +	crtc->cursor_x = new_state->base.crtc_x;
>> +	crtc->cursor_y = new_state->base.crtc_y;
>> +
>> +	if (!new_state->visible ||
>> +	    WARN_ON(new_state->visible && !crtc->state->active)) {
> As an aside: We must move over to compute ->visible irrespective of
> ->active. I haven't check whether you fix that later on already, but this
> is important to be able to check wm constraints correctly. The
> crtc_state->active handling should imo be done in the common code by
> simply not calling any of the plane or crtc functions hw update if the
> crtc is off.
In the crtc_check function I'm setting:
intel_crtc->atomic.update_wm = mode_changed; ?

It means recalculating wm on dpms off->on too but is that really such a burden?
Treating crtc->state->active as the only knob for crtc on/off simplifies things.
> Of course that means we need to add all planes both for ->mode_changed and
> for ->active_changed, but needs_modeset(crtc_state) takes care of that
> already.
This is exactly why I add all planes in the check function.

> Aside: The idea behind the split between drm_atomic_helper_check_planes
> and drm_atomic_helper_check_modeset is that the former handles planes-only
> updates while the latter handles modeset. We can't fully use
> check_modesets because we don't use crtc helpers, but I think reusing the
> same idea still has merit:
>
> - We create an intel_check_modeset which calls the helper's check_modeset
>   plus afterwards all the modeset checks (dpll, cloning checks, adding
>   plane states and global resources) that we do in crtc_compute_config and
>   friends right now. That one would also add plane states as needed (but
>   not call plane->atomic_check). All of this would be skipped if
>   needs_modeset isn't true.
Might be doable if we do add planes between check_modeset and check_planes.

Originally I tried it in intel_modeset_compute_config, but that didn't work when
the crtc was different from the one that had the argument passed.

> - After that we'd call the helper's plane_check functions which does
>   exclusively plane-update related checks. crtc/plane->atomic_check
>   wouldn't contain any modeset related checks (like the encoder cloning or
>   plane adding you're doing right now).
I think encoder cloning ought to be part of the crtc check since its done on each crtc.
>> <snip>
> Ok I think overall this patch is too big and needs to be split a bit. The
> following should be doable as prep-patches:
> - set_base_atomic (if we dare to touch it at all)
I think I'll drop it.
> - shuffling the crtc modeset check logic around to add plane states as
>   needed, making sure we only run that if needs_modeset indicates so.
I think splitting this out would break the transitional helpers, they rely on the plane state not being there so the state is correctly updated
when the transitional plane helpers run, else we would cause stale state to be swapped. :-(
> - Pushing the checks for intel_crtc->active or crtc->state->active out of
>   the low-level plane code up into higher level atomic plane code (and the
>   few legacy entry points for plane updates that we still have maybe, but
>   those should be covered I think with the universal plane + transitional
>   helpers).
I fear this might break the transitional helper calls too, the transitional helpers require the transitional state to be mostly in sync with the atomic state.

> Then we can put in the meat of this patch, i.e. replacing the helpers with
> atomic updates in the modeset code.
I wish that would work. ;-)

~Maarten
Ville Syrjala May 12, 2015, 1:43 p.m. UTC | #3
On Tue, May 12, 2015 at 03:33:12PM +0200, Maarten Lankhorst wrote:
> Op 12-05-15 om 10:18 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> >> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> >>  	intel_state->clip.x1 = 0;
> >>  	intel_state->clip.y1 = 0;
> >> -	intel_state->clip.x2 =
> >> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> >> -	intel_state->clip.y2 =
> >> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> >> -
> >> -	/*
> >> -	 * Disabling a plane is always okay; we just need to update
> >> -	 * fb tracking in a special way since cleanup_fb() won't
> >> -	 * get called by the plane helpers.
> >> -	 */
> >> -	if (state->fb == NULL && plane->state->fb != NULL) {
> >> -		/*
> >> -		 * 'prepare' is never called when plane is being disabled, so
> >> -		 * we need to handle frontbuffer tracking as a special case
> >> -		 */
> >> -		intel_crtc->atomic.disabled_planes |=
> >> -			(1 << drm_plane_index(plane));
> >> -	}
> >> +	drm_crtc_get_hv_timing(&crtc_state->mode,
> >> +			       &intel_state->clip.x2,
> >> +			       &intel_state->clip.y2);
> > Imo this is obfuscating things a bit, why not just unconditionally copy
> > pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> > most platforms must match the primary plane window except for gen2 and
> > gen9+.
> pipe_src_* is calculated in the same way,

Except we may end up rounding pipe_src_w down if we have
double-wide/dual link lvds etc., and we definitely need to clip the
planes against the real pipe src size.

> and at the time of plane validation the crtc validation hasn't run yet,
> so pipe_src_* contains outdated values which break in interesting ways.

Just check crtc first. Or actually we probably need pre + post crtc
checks since we want to do wm compute and such after the planes have
been handled.
Ville Syrjala May 12, 2015, 1:46 p.m. UTC | #4
On Tue, May 12, 2015 at 04:43:09PM +0300, Ville Syrjälä wrote:
> On Tue, May 12, 2015 at 03:33:12PM +0200, Maarten Lankhorst wrote:
> > Op 12-05-15 om 10:18 schreef Daniel Vetter:
> > > On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> > >> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > >>  	intel_state->clip.x1 = 0;
> > >>  	intel_state->clip.y1 = 0;
> > >> -	intel_state->clip.x2 =
> > >> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> > >> -	intel_state->clip.y2 =
> > >> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> > >> -
> > >> -	/*
> > >> -	 * Disabling a plane is always okay; we just need to update
> > >> -	 * fb tracking in a special way since cleanup_fb() won't
> > >> -	 * get called by the plane helpers.
> > >> -	 */
> > >> -	if (state->fb == NULL && plane->state->fb != NULL) {
> > >> -		/*
> > >> -		 * 'prepare' is never called when plane is being disabled, so
> > >> -		 * we need to handle frontbuffer tracking as a special case
> > >> -		 */
> > >> -		intel_crtc->atomic.disabled_planes |=
> > >> -			(1 << drm_plane_index(plane));
> > >> -	}
> > >> +	drm_crtc_get_hv_timing(&crtc_state->mode,
> > >> +			       &intel_state->clip.x2,
> > >> +			       &intel_state->clip.y2);
> > > Imo this is obfuscating things a bit, why not just unconditionally copy
> > > pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> > > most platforms must match the primary plane window except for gen2 and
> > > gen9+.
> > pipe_src_* is calculated in the same way,
> 
> Except we may end up rounding pipe_src_w down if we have
> double-wide/dual link lvds etc., and we definitely need to clip the
> planes against the real pipe src size.

Oh and we definitely want to keep the 'user mode == pipe src'
assumptions to a minimum so that we might some day finish the "expose
panel fitter to userland" task.
Daniel Vetter May 12, 2015, 3:31 p.m. UTC | #5
On Tue, May 12, 2015 at 04:43:09PM +0300, Ville Syrjälä wrote:
> On Tue, May 12, 2015 at 03:33:12PM +0200, Maarten Lankhorst wrote:
> > Op 12-05-15 om 10:18 schreef Daniel Vetter:
> > > On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> > >> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> > >>  	intel_state->clip.x1 = 0;
> > >>  	intel_state->clip.y1 = 0;
> > >> -	intel_state->clip.x2 =
> > >> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> > >> -	intel_state->clip.y2 =
> > >> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> > >> -
> > >> -	/*
> > >> -	 * Disabling a plane is always okay; we just need to update
> > >> -	 * fb tracking in a special way since cleanup_fb() won't
> > >> -	 * get called by the plane helpers.
> > >> -	 */
> > >> -	if (state->fb == NULL && plane->state->fb != NULL) {
> > >> -		/*
> > >> -		 * 'prepare' is never called when plane is being disabled, so
> > >> -		 * we need to handle frontbuffer tracking as a special case
> > >> -		 */
> > >> -		intel_crtc->atomic.disabled_planes |=
> > >> -			(1 << drm_plane_index(plane));
> > >> -	}
> > >> +	drm_crtc_get_hv_timing(&crtc_state->mode,
> > >> +			       &intel_state->clip.x2,
> > >> +			       &intel_state->clip.y2);
> > > Imo this is obfuscating things a bit, why not just unconditionally copy
> > > pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> > > most platforms must match the primary plane window except for gen2 and
> > > gen9+.
> > pipe_src_* is calculated in the same way,
> 
> Except we may end up rounding pipe_src_w down if we have
> double-wide/dual link lvds etc., and we definitely need to clip the
> planes against the real pipe src size.
> 
> > and at the time of plane validation the crtc validation hasn't run yet,
> > so pipe_src_* contains outdated values which break in interesting ways.
> 
> Just check crtc first. Or actually we probably need pre + post crtc
> checks since we want to do wm compute and such after the planes have
> been handled.

Yeah in the end the sequence should be:
1. compute pipe_config for modeset changes on any crtcs where mode_changed
   is set.
2. call helper_check_planes which should use the values computed in step
   1. to compute the nuclear plane flip states. It's important that the
   depencies flow one-way since only then can we skip all the modeset
   state recomputation (and possible serialization because we need more
   crtc states) for pure plane flips.

Where exactly do the pipe_src_w/h values get out of sync here? Imo better
to patch them up someplace in the legacy modeset code than add hacks to
plane atomic functions.

Cheers, Daniel
Daniel Vetter May 12, 2015, 4 p.m. UTC | #6
On Tue, May 12, 2015 at 03:33:12PM +0200, Maarten Lankhorst wrote:
> Op 12-05-15 om 10:18 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:24:46PM +0200, Maarten Lankhorst wrote:
> >> This kills off most of the transitional helpers and uses atomic plane updates
> >> in the modeset path to update everything.
> >>
> >> Getting rid of the transitional plane helpers meant that planes had to be added
> >> in the crtc check function. On modeset a connector can be moved to a different
> >> crtc, and this is not handled correctly otherwise.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Ok pile of comments on this one. I don't yet fully grasp it all, but at
> > the very bottom I've jotted down a list of ideas for how to move forward
> > with this one.
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic_plane.c |  59 ++-
> >>  drivers/gpu/drm/i915/intel_display.c      | 655 ++++++++++++++++++------------
> >>  drivers/gpu/drm/i915/intel_drv.h          |   2 +-
> >>  drivers/gpu/drm/i915/intel_sprite.c       |  80 +---
> >>  4 files changed, 441 insertions(+), 355 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index 86ba4b2c3a65..85b87e4d4b6e 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>  				    struct drm_plane_state *state)
> >>  {
> >>  	struct drm_crtc *crtc = state->crtc;
> >> -	struct intel_crtc *intel_crtc;
> >> -	struct intel_crtc_state *crtc_state;
> >> +	struct drm_crtc_state *crtc_state;
> >>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >>  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> >>  
> >> -	crtc = crtc ? crtc : plane->crtc;
> >> -	intel_crtc = to_intel_crtc(crtc);
> >> -
> >> +	intel_state->visible = false;
> >>  	/*
> >>  	 * Both crtc and plane->crtc could be NULL if we're updating a
> >>  	 * property while the plane is disabled.  We don't actually have
> >>  	 * anything driver-specific we need to test in that case, so
> >>  	 * just return success.
> >>  	 */
> >> -	if (!crtc)
> >> +	if (!crtc) {
> >> +		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
> >>  		return 0;
> >> +	}
> >> +
> >> +	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
> > Please reuse drm_atomic_get_crtc_state, this deref magic is hard to read.
> > Maybe we should have a nofail variant of those to encode the below WARN_ON
> > even ...
> Yeah, there are a few places where I use it like this, because in those cases the relevant state should already exist.

Imo get_foo_state + WARN_ON(IS_ERR) is better than open-coding it. It's a
tricky deref chain and I've gotten it wrong multiple times when writing
atomic helpers.

> >> +	if (WARN_ON(!crtc_state))
> >> +		return 0;
> >> +
> >> +	if (!crtc_state->enable) {
> >> +		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
> >>  
> >> -	/* FIXME: temporary hack necessary while we still use the plane update
> >> -	 * helper. */
> >> -	if (state->state) {
> >> -		crtc_state =
> >> -			intel_atomic_get_crtc_state(state->state, intel_crtc);
> >> -		if (IS_ERR(crtc_state))
> >> -			return PTR_ERR(crtc_state);
> >> -	} else {
> >> -		crtc_state = intel_crtc->config;
> >> +		/*
> >> +		 * Probably allowed after converting to atomic. Right
> >> +		 * now it probably means we have the state confused.
> >> +		 */
> >> +		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);
> > This is already possible with the primary plane support - you can disable
> > it (though not change the mode at the same time).
> That explains a few warnings, thanks. :-)
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (!crtc_state->active) {
> >> +		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
> >> +		return 0;
> >>  	}
> >>  
> >>  	/*
> >> @@ -155,24 +163,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> >>  	intel_state->clip.x1 = 0;
> >>  	intel_state->clip.y1 = 0;
> >> -	intel_state->clip.x2 =
> >> -		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
> >> -	intel_state->clip.y2 =
> >> -		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
> >> -
> >> -	/*
> >> -	 * Disabling a plane is always okay; we just need to update
> >> -	 * fb tracking in a special way since cleanup_fb() won't
> >> -	 * get called by the plane helpers.
> >> -	 */
> >> -	if (state->fb == NULL && plane->state->fb != NULL) {
> >> -		/*
> >> -		 * 'prepare' is never called when plane is being disabled, so
> >> -		 * we need to handle frontbuffer tracking as a special case
> >> -		 */
> >> -		intel_crtc->atomic.disabled_planes |=
> >> -			(1 << drm_plane_index(plane));
> >> -	}
> >> +	drm_crtc_get_hv_timing(&crtc_state->mode,
> >> +			       &intel_state->clip.x2,
> >> +			       &intel_state->clip.y2);
> > Imo this is obfuscating things a bit, why not just unconditionally copy
> > pipe_src_w/h to clip.x/y2? get_hv_timing is for the pipe size, which on
> > most platforms must match the primary plane window except for gen2 and
> > gen9+.
> pipe_src_* is calculated in the same way, and at the time of plane validation the crtc validation hasn't run yet,
> so pipe_src_* contains outdated values which break in interesting ways.

See my comment in reply to Ville's mail - we need to track this down and
paper over (if needed) somewhere else than the atomic plane code.

> 
> >>  
> >>  	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> >>  		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 956c9964275d..9610f76a2489 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -100,14 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
> >>  			    const struct intel_crtc_state *pipe_config);
> >>  static void chv_prepare_pll(struct intel_crtc *crtc,
> >>  			    const struct intel_crtc_state *pipe_config);
> >> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> >> +				   struct drm_crtc_state *crtc_state);
> > I consider forward declarations evil, but given how much of a mess
> > intel_display.c is it doesn't really matter much ;-)
> >
> >>  static void intel_begin_crtc_commit(struct drm_crtc *crtc);
> >>  static void intel_finish_crtc_commit(struct drm_crtc *crtc);
> >>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
> >>  	struct intel_crtc_state *crtc_state);
> >>  static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
> >>  			   int num_connectors);
> >> -static void intel_crtc_enable_planes(struct drm_crtc *crtc);
> >> -static void intel_crtc_disable_planes(struct drm_crtc *crtc);
> >> +static void intel_pre_disable_primary(struct drm_crtc *crtc);
> >> +static void intel_post_enable_primary(struct drm_crtc *crtc);
> >>  
> >>  static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
> >>  {
> >> @@ -2220,28 +2222,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
> >>  	POSTING_READ(reg);
> >>  }
> >>  
> >> -/**
> >> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
> >> - * @plane:  plane to be enabled
> >> - * @crtc: crtc for the plane
> >> - *
> >> - * Enable @plane on @crtc, making sure that the pipe is running first.
> >> - */
> >> -static void intel_enable_primary_hw_plane(struct drm_plane *plane,
> >> -					  struct drm_crtc *crtc)
> >> -{
> >> -	struct drm_device *dev = plane->dev;
> >> -	struct drm_i915_private *dev_priv = dev->dev_private;
> >> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> -
> >> -	/* If the pipe isn't enabled, we can't pump pixels and may hang */
> >> -	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
> >> -	to_intel_plane_state(plane->state)->visible = true;
> >> -
> >> -	dev_priv->display.update_primary_plane(crtc, plane->fb,
> >> -					       crtc->x, crtc->y);
> >> -}
> >> -
> >>  static bool need_vtd_wa(struct drm_device *dev)
> >>  {
> >>  #ifdef CONFIG_INTEL_IOMMU
> >> @@ -3161,11 +3141,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct intel_plane_state *plane_state =
> >> +		to_intel_plane_state(crtc->primary->state);
> >> +	bool was_visible = plane_state->visible;
> >>  
> >> -	if (dev_priv->display.disable_fbc)
> >> +	/* Not supported right now by the helper, but lets be thorough. */
> >> +	if (was_visible && !fb)
> >> +		intel_pre_disable_primary(crtc);
> >> +	else if (was_visible && dev_priv->display.disable_fbc)
> >>  		dev_priv->display.disable_fbc(dev);
> >>  
> >> +	plane_state->visible = !!fb;
> >>  	dev_priv->display.update_primary_plane(crtc, fb, x, y);
> >> +	if (!was_visible && fb)
> >> +		intel_post_enable_primary(crtc);
> > This contains a vblank_wait (at least for broadwell) which is a no-go in
> > set_base_atomic. Given that this is only used by kgdb and I'm not aware of
> > anyone using that and we also don't have any testcase for this code I
> > think the best would be to just not touch this and let it keep on
> > bitrotting ...
> Ok, I'll drop this hunk.
> 
> >>  
> >>  	return 0;
> >>  }
> >> @@ -3192,16 +3181,17 @@ static void intel_update_primary_planes(struct drm_device *dev)
> >>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>  
> >>  		drm_modeset_lock(&crtc->mutex, NULL);
> >> -		/*
> >> -		 * FIXME: Once we have proper support for primary planes (and
> >> -		 * disabling them without disabling the entire crtc) allow again
> >> -		 * a NULL crtc->primary->fb.
> >> -		 */
> >> -		if (intel_crtc->active && crtc->primary->fb)
> >> +
> >> +		if (intel_crtc->active) {
> >> +			const struct intel_plane_state *state =
> >> +				to_intel_plane_state(crtc->primary->state);
> >> +
> >>  			dev_priv->display.update_primary_plane(crtc,
> >> -							       crtc->primary->fb,
> >> -							       crtc->x,
> >> -							       crtc->y);
> >> +							state->base.fb,
> >> +							state->src.x1 >> 16,
> >> +							state->src.y1 >> 16);
> >> +		}
> > I think the above hunk could be split out. And I'm not sure it's required
> > really, this is just to catch up on CS flips. Which won't be used any more
> > once we have atomic, but until then the legacy state stuff used here
> > should be good enough.
> ?

This is the gpu reset code - CS based flips can get lost if the gpu gets
reset, which means we need to do them manually. By re-running
update_primary_plane with the current state.

But CS based flips don't work for atomic, which means as soon as we've
hooked up the pageflip atomic helper we won't ever see an a CS based flip
when using atomic. Which means using the legacy state objects meanwhile
(until that transition is done and we can just outright nuke this code)
should be perfectly safe.

> >> +
> >>  		drm_modeset_unlock(&crtc->mutex);
> >>  	}
> >>  }
> >> @@ -4572,20 +4562,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
> >>  	}
> >>  }
> >>  
> >> -static void intel_enable_sprite_planes(struct drm_crtc *crtc)
> >> -{
> >> -	struct drm_device *dev = crtc->dev;
> >> -	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >> -	struct drm_plane *plane;
> >> -	struct intel_plane *intel_plane;
> >> -
> >> -	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> >> -		intel_plane = to_intel_plane(plane);
> >> -		if (intel_plane->pipe == pipe)
> >> -			intel_plane_restore(&intel_plane->base);
> >> -	}
> >> -}
> >> -
> >>  void hsw_enable_ips(struct intel_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->base.dev;
> >> @@ -4815,44 +4791,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>  	hsw_disable_ips(intel_crtc);
> >>  }
> >>  
> >> -static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> >> -{
> >> -	intel_enable_primary_hw_plane(crtc->primary, crtc);
> >> -	intel_enable_sprite_planes(crtc);
> >> -	intel_crtc_update_cursor(crtc, true);
> >> -
> >> -	intel_post_enable_primary(crtc);
> >> -}
> >> -
> >> -static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> >> -{
> >> -	struct drm_device *dev = crtc->dev;
> >> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> -	struct intel_plane *intel_plane;
> >> -	int pipe = intel_crtc->pipe;
> >> -
> >> -	intel_crtc_wait_for_pending_flips(crtc);
> >> -
> >> -	intel_pre_disable_primary(crtc);
> >> -
> >> -	intel_crtc_dpms_overlay_disable(intel_crtc);
> >> -	for_each_intel_plane(dev, intel_plane) {
> >> -		if (intel_plane->pipe == pipe) {
> >> -			struct drm_crtc *from = intel_plane->base.crtc;
> >> -
> >> -			intel_plane->disable_plane(&intel_plane->base,
> >> -						   from ?: crtc, true);
> >> -		}
> >> -	}
> >> -
> >> -	/*
> >> -	 * FIXME: Once we grow proper nuclear flip support out of this we need
> >> -	 * to compute the mask of flip planes precisely. For the time being
> >> -	 * consider this a flip to a NULL plane.
> >> -	 */
> >> -	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
> >> -}
> >> -
> >>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >>  {
> >>  	struct drm_device *dev = crtc->dev;
> >> @@ -11061,6 +10999,7 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> >>  	.load_lut = intel_crtc_load_lut,
> >>  	.atomic_begin = intel_begin_crtc_commit,
> >>  	.atomic_flush = intel_finish_crtc_commit,
> >> +	.atomic_check = intel_atomic_check_crtc,
> >>  };
> >>  
> >>  /* Transitional helper to copy current connector/encoder state to
> >> @@ -11426,16 +11365,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> >>  	int i;
> >>  	bool retry = true;
> >>  
> >> -	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
> >> -		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> >> -		return -EINVAL;
> >> -	}
> >> -
> >> -	if (!check_digital_port_conflicts(state)) {
> >> -		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
> >> -		return -EINVAL;
> >> -	}
> >> -
> > I think it would help if you'd split out the addition of the crtc_check
> > function into a separate prep patch. At least I don't see a depency here
> > ...
> Yeah but that would be the only thing put in there, oh well got to start somewhere...

Yeah that's kinda what I mean. This patch's subject talks about reworking
the plane update handling, this clearly isn't part of that. So imo a good
(small) prep patch to split out.

> >
> >> @@ -12275,7 +12333,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> >>  	struct drm_atomic_state *state = pipe_config->base.state;
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_crtc_state *crtc_state;
> >> -	int ret = 0;
> >> +	int ret;
> >>  	int i;
> >>  
> >>  	ret = __intel_set_mode_checks(state);
> >> @@ -12286,14 +12344,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	__intel_set_mode_update_planes(dev, state);
> > This calls crtc->atomic_begin, which does an irq_disable ...
> Yeah I tried to handle it by fixing all previous places to prevent atomic evading on modeset,
> which means it is mostly safe to do it like this.

Hm, we want to do the flip to black before disables and the flip form
black on enabling as nuclear flips too. Dropping the vblank evade isn't a
good idea imo since we'll need to resurrect it. And failing to bring up
all planes on the very first frame torpedoes the "all frames are perfect"
goal.

And there's a lot more special cases than just the evade I think, so I'm
not sure whether the reuse is worth it.

> 
> >
> >> +
> >>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >>  		if (!needs_modeset(crtc_state))
> >>  			continue;
> >>  
> >> -		intel_crtc_disable_planes(crtc);
> >> +		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
> >>  		dev_priv->display.crtc_disable(crtc);
> >> -		if (!crtc_state->enable)
> >> -			drm_plane_helper_disable(crtc->primary);
> >>  	}
> >>  
> >>  	/* Only after disabling all output pipelines that will be changed can we
> >> @@ -12305,8 +12363,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> >>  
> >>  	modeset_update_crtc_power_domains(state);
> >>  
> >> -	drm_atomic_helper_commit_planes(dev, state);
> >> -
> >>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> >>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >>  		if (!crtc->state->active)
> >> @@ -12314,13 +12370,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
> >>  
> >>  		update_scanline_offset(to_intel_crtc(crtc));
> >>  
> >> -		dev_priv->display.crtc_enable(crtc);
> >> -		intel_crtc_enable_planes(crtc);
> >> +		if (needs_modeset(crtc->state))
> >> +			dev_priv->display.crtc_enable(crtc);
> >>  	}
> >>  
> >> -	/* FIXME: add subpixel order */
> >> +	__intel_set_mode_cleanup_planes(dev, state);
> > which is only cleared in atomic_flush here. We need both of these at the
> > bottom here. In atomic-helpers-speak the sequence should be
> >
> > drm_atomic_helper_commit_modeset_disables();
> > drm_atomic_helper_commit_modeset_enables();
> > drm_atomic_helper_commit_planes();
> >
> > with the note that our crtc_disable must do the unconditional plane
> > disabling itself. I.e. you can't just remove disable_planes without
> > replacement, that one needs to be open-coded (using plane->atomic_disable
> > if possible).
> >
> > Doing things this way would also alleviate any need to split up swap_state
> > as you do in this patch here.
> You're right it could be done like this, but doing commit_planes around the modesets allows
> us to test the same code for calculating disable planes during modeset as when togging planes
> separately. For example this is useful when a connector moves to a different crtc.

Moving connectors will force a full modeset on both crtcs (at least with
atomic helpers, current i915 code isn't fully there yet without your
series here). So I'm not sure how much this helps.

And I don't think code reuse is that useful really since crtc disabling is
a special case anyway.

> In any case this is cosmetic, and done for bisecting. I can move the swap_states in this commit,
> but chose to do it 2 commits later for easier bisection.
> 
> I can't move it to before this commit because calling the plane helpers messes up the state.

Yeah that direction is clear. But I'm not too clear on why we have to move
things now already? It won't work with your approach, but with my idea of
a special-purpose disable_planes function only used for crtc_disable we
only need the full atomic state in crtc_enable. And with the current
position of swap_states that's the right one already.

Then we can still move swap_states up in the sequence in a later patch
like you do, but without the big intermediate split. That's my other
reason for doing the special_disable_planes + commit_planes after modeset
is all done.

> >> -	drm_atomic_helper_cleanup_planes(dev, state);
> > Please keep this separate and don't hide it in another function, it's a
> > distinct step from the hw update (which is now done in the misnamed
> > intel_set_mode_cleanup_planes).
> Ok.
> >
> >> +	/* FIXME: add subpixel order */
> >>  
> >>  	drm_atomic_state_free(state);
> >>  
> >> @@ -12568,20 +12624,11 @@ intel_modeset_stage_output_state(struct drm_device *dev,
> >>  	return 0;
> >>  }
> >>  
> >> -static bool primary_plane_visible(struct drm_crtc *crtc)
> >> -{
> >> -	struct intel_plane_state *plane_state =
> >> -		to_intel_plane_state(crtc->primary->state);
> >> -
> >> -	return plane_state->visible;
> >> -}
> >> -
> >>  static int intel_crtc_set_config(struct drm_mode_set *set)
> >>  {
> >>  	struct drm_device *dev;
> >>  	struct drm_atomic_state *state = NULL;
> >>  	struct intel_crtc_state *pipe_config;
> >> -	bool primary_plane_was_visible;
> >>  	int ret;
> >>  
> >>  	BUG_ON(!set);
> >> @@ -12620,38 +12667,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >>  
> >>  	intel_update_pipe_size(to_intel_crtc(set->crtc));
> >>  
> >> -	primary_plane_was_visible = primary_plane_visible(set->crtc);
> >> -
> >>  	ret = intel_set_mode_with_config(set->crtc, pipe_config);
> >> -
> >> -	if (ret == 0 &&
> >> -	    pipe_config->base.enable &&
> >> -	    pipe_config->base.planes_changed &&
> >> -	    !needs_modeset(&pipe_config->base)) {
> >> -		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> >> -
> >> -		/*
> >> -		 * We need to make sure the primary plane is re-enabled if it
> >> -		 * has previously been turned off.
> >> -		 */
> >> -		if (ret == 0 && !primary_plane_was_visible &&
> >> -		    primary_plane_visible(set->crtc)) {
> >> -			WARN_ON(!intel_crtc->active);
> >> -			intel_post_enable_primary(set->crtc);
> >> -		}
> >> -
> >> -		/*
> >> -		 * In the fastboot case this may be our only check of the
> >> -		 * state after boot.  It would be better to only do it on
> >> -		 * the first update, but we don't have a nice way of doing that
> >> -		 * (and really, set_config isn't used much for high freq page
> >> -		 * flipping, so increasing its cost here shouldn't be a big
> >> -		 * deal).
> >> -		 */
> >> -		if (i915.fastboot && ret == 0)
> >> -			intel_modeset_check_state(set->crtc->dev);
> >> -	}
> >> -
> >>  	if (ret) {
> >>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >>  			      set->crtc->base.id, ret);
> >> @@ -12791,6 +12807,9 @@ bool intel_wm_need_update(struct drm_plane *plane,
> >>  	    plane->state->rotation != state->rotation)
> >>  		return true;
> >>  
> >> +	if (plane->state->crtc_w != state->crtc_w)
> >> +		return true;
> >> +
> >>  	return false;
> >>  }
> >>  
> >> @@ -12819,6 +12838,9 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >>  	unsigned frontbuffer_bits = 0;
> >>  	int ret = 0;
> >>  
> >> +	if (!to_intel_plane_state(plane->state)->visible)
> >> +		old_obj = NULL;
> >> +
> >>  	if (!obj)
> >>  		return 0;
> >>  
> >> @@ -12915,10 +12937,8 @@ intel_check_primary_plane(struct drm_plane *plane,
> >>  			  struct intel_plane_state *state)
> >>  {
> >>  	struct drm_device *dev = plane->dev;
> >> -	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct drm_crtc *crtc = state->base.crtc;
> >>  	struct intel_crtc *intel_crtc;
> >> -	struct intel_crtc_state *crtc_state;
> >>  	struct drm_framebuffer *fb = state->base.fb;
> >>  	struct drm_rect *dest = &state->dst;
> >>  	struct drm_rect *src = &state->src;
> >> @@ -12926,90 +12946,46 @@ intel_check_primary_plane(struct drm_plane *plane,
> >>  	bool can_position = false;
> >>  	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
> >>  	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
> >> -	int ret;
> >>  
> >> -	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >> -	crtc_state = state->base.state ?
> >> -		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
> >>  
> >>  	if (INTEL_INFO(dev)->gen >= 9) {
> >> +		struct intel_crtc_state *crtc_state =
> >> +			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
> >> +
> >>  		min_scale = 1;
> >>  		max_scale = skl_max_scale(intel_crtc, crtc_state);
> >>  		can_position = true;
> >>  	}
> >>  
> >> -	ret = drm_plane_helper_check_update(plane, crtc, fb,
> >> -					    src, dest, clip,
> >> -					    min_scale,
> >> -					    max_scale,
> >> -					    can_position, true,
> >> -					    &state->visible);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	if (intel_crtc->active) {
> >> -		struct intel_plane_state *old_state =
> >> -			to_intel_plane_state(plane->state);
> >> -
> >> -		intel_crtc->atomic.wait_for_flips = true;
> >> -
> >> -		/*
> >> -		 * FBC does not work on some platforms for rotated
> >> -		 * planes, so disable it when rotation is not 0 and
> >> -		 * update it when rotation is set back to 0.
> >> -		 *
> >> -		 * FIXME: This is redundant with the fbc update done in
> >> -		 * the primary plane enable function except that that
> >> -		 * one is done too late. We eventually need to unify
> >> -		 * this.
> >> -		 */
> >> -		if (state->visible &&
> >> -		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> >> -		    dev_priv->fbc.crtc == intel_crtc &&
> >> -		    state->base.rotation != BIT(DRM_ROTATE_0)) {
> >> -			intel_crtc->atomic.disable_fbc = true;
> >> -		}
> >> -
> >> -		if (state->visible && !old_state->visible) {
> >> -			/*
> >> -			 * BDW signals flip done immediately if the plane
> >> -			 * is disabled, even if the plane enable is already
> >> -			 * armed to occur at the next vblank :(
> >> -			 */
> >> -			if (IS_BROADWELL(dev))
> >> -				intel_crtc->atomic.wait_vblank = true;
> >> -		}
> >> -
> >> -		intel_crtc->atomic.fb_bits |=
> >> -			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >> -
> >> -		intel_crtc->atomic.update_fbc = true;
> >> -
> >> -		if (intel_wm_need_update(plane, &state->base))
> >> -			intel_crtc->atomic.update_wm = true;
> >> -	}
> >> +	return drm_plane_helper_check_update(plane, crtc, fb,
> >> +					     src, dest, clip,
> >> +					     min_scale, max_scale,
> >> +					     can_position, true,
> >> +					     &state->visible);
> >> +}
> >>  
> >> -	if (INTEL_INFO(dev)->gen >= 9) {
> >> -		ret = skl_update_scaler_users(intel_crtc, crtc_state,
> >> -			to_intel_plane(plane), state, 0);
> >> -		if (ret)
> >> -			return ret;
> >> -	}
> >> +static void
> >> +intel_disable_primary_plane(struct drm_plane *plane,
> >> +			    struct drm_crtc *crtc,
> >> +			    bool force)
> >> +{
> >> +	struct drm_device *dev = plane->dev;
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  
> >> -	return 0;
> >> +	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
> >>  }
> >>  
> >>  static void
> >>  intel_commit_primary_plane(struct drm_plane *plane,
> >> -			   struct intel_plane_state *state)
> >> +			   struct intel_plane_state *new_state)
> >>  {
> >> -	struct drm_crtc *crtc = state->base.crtc;
> >> -	struct drm_framebuffer *fb = state->base.fb;
> >> +	struct drm_crtc *crtc = new_state->base.crtc;
> >> +	struct drm_framebuffer *fb = new_state->base.fb;
> >>  	struct drm_device *dev = plane->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	struct intel_crtc *intel_crtc;
> >> -	struct drm_rect *src = &state->src;
> >> +	struct drm_rect *src = &new_state->src;
> >>  
> >>  	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >> @@ -13018,25 +12994,178 @@ intel_commit_primary_plane(struct drm_plane *plane,
> >>  	crtc->x = src->x1 >> 16;
> >>  	crtc->y = src->y1 >> 16;
> >>  
> >> -	if (intel_crtc->active) {
> >> -		if (state->visible)
> >> -			/* FIXME: kill this fastboot hack */
> >> -			intel_update_pipe_size(intel_crtc);
> >> +	if (!new_state->visible ||
> >> +	    WARN_ON(new_state->visible && !crtc->state->active)) {
> >> +		intel_disable_primary_plane(plane, crtc, false);
> >> +	} else {
> >> +		/* FIXME: kill this fastboot hack */
> >> +		intel_update_pipe_size(intel_crtc);
> >>  
> >> -		dev_priv->display.update_primary_plane(crtc, plane->fb,
> >> -						       crtc->x, crtc->y);
> >> +		dev_priv->display.update_primary_plane(crtc, fb,
> >> +						       src->x1 >> 16,
> >> +						       src->y1 >> 16);
> >>  	}
> >>  }
> >>  
> >> -static void
> >> -intel_disable_primary_plane(struct drm_plane *plane,
> >> -			    struct drm_crtc *crtc,
> >> -			    bool force)
> >> +/* Transitional checking here, mostly for plane updates */
> >> +static int intel_atomic_check_crtc(struct drm_crtc *crtc,
> >> +				   struct drm_crtc_state *crtc_state)
> >>  {
> >> -	struct drm_device *dev = plane->dev;
> >> +	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> >> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> +	struct drm_atomic_state *state = crtc_state->state;
> >> +	struct drm_device *dev = crtc->dev;
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct drm_plane *plane;
> >> +	unsigned plane_mask;
> >> +	int idx, ret;
> >> +	bool mode_changed = needs_modeset(crtc_state);
> >> +	bool is_crtc_enabled = crtc_state->active;
> >> +	bool was_crtc_enabled = crtc->state->active;
> >>  
> >> -	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
> >> +	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
> >> +		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
> >> +		return -EINVAL;
> >> +	}
> > I think we need two have 2 blocks in the crtc_check function: One part
> > that just updates watermarks (when state->plane_changed is set) and one
> > part that deals with the modeset checks (adding planes, checking cloning
> > and all that) when needs_modeset(state) is true.
> Makes sense.
> 
> >> +
> >> +	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> >> +	intel_crtc->atomic.update_wm = mode_changed;
> >> +
> >> +	idx = crtc->base.id;
> >> +	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> >> +		"Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
> >> +		idx, crtc->state->active, intel_crtc->active);
> > State computation is async, you cant check this in the atomic_check
> > functions. If you think this is useful then we'd need to check this every
> > time before we update intel_crtc->active. But we have the various WARN_ON
> > all over the place (converted over to crtc->state->active) so I think
> > we're more than covered and can just rip out our own intel_crtc->active
> > eventually. At least that's kinda been my idea with state->active too.
> Yeah, it's a remainder when I had the commits in a different order.
> >> +
> >> +	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
> >> +			 idx, was_crtc_enabled, is_crtc_enabled);
> >> +
> >> +	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
> >> +	drm_for_each_plane_mask(plane, dev, plane_mask) {
> >> +		int i = drm_plane_index(plane);
> >> +		struct drm_plane_state *plane_state = state->plane_states[i];
> >> +		struct intel_plane_state *old_plane_state;
> >> +		bool turn_off, turn_on, visible, was_visible;
> >> +		struct drm_framebuffer *fb;
> >> +
> >> +		if (!plane_state) {
> >> +			const struct drm_plane_helper_funcs *funcs;
> >> +			int ret;
> >> +
> >> +			if (!mode_changed || !plane->state->fb)
> >> +				continue;
> >> +
> >> +			plane_state = drm_atomic_get_plane_state(state, plane);
> > We need to check for the crtc constrains before acquiring the plane state,
> > otherwise all the locking goes single-threaded. On intel hw we can do that
> > because the plane->crtc links are static, you can look at
> > intel_plane->pipe.
> This code is to make sure all planes are checked when mode_changed && plane->state->fb,
> plane_state->visible needs to be updated in that case.

Yes we need to recompute plane state, which is why modeset check should be
done first (including adding any additional plane states because
mode_changed is true). Then after all that we can call the atomic_check
functions for the plane update. There's more detail in my reply further
down, but see also Ville's reply.

> 
> >> +			if (IS_ERR(plane_state))
> >> +				return PTR_ERR(plane_state);
> >> +
> >> +			funcs = plane->helper_private;
> >> +			ret = funcs->atomic_check(plane, plane_state);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >> +		old_plane_state = to_intel_plane_state(plane->state);
> >> +
> >> +		was_visible = was_crtc_enabled && (old_plane_state->visible ||
> >> +			      old_plane_state->hw_enabled);
> >> +		visible = to_intel_plane_state(plane_state)->visible &&
> >> +			      is_crtc_enabled;
> >> +
> >> +		if (plane->state->crtc != crtc)
> >> +			was_visible = false;
> >> +		if (plane_state->crtc != crtc)
> >> +			visible = false;
> >> +
> >> +		if (!was_visible && !visible)
> >> +			continue;
> >> +
> >> +		turn_off = was_visible && (!visible || mode_changed);
> >> +		turn_on = visible && (!was_visible || mode_changed);
> >> +		fb = plane_state->fb;
> >> +
> >> +		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
> >> +			plane->base.id, fb ? fb->base.id : -1);
> >> +		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
> >> +			was_visible, visible, turn_off, turn_on, mode_changed);
> >> +
> >> +		/* plane being turned off as part of modeset or changes? */
> >> +		if (intel_wm_need_update(plane, plane_state))
> >> +			intel_crtc->atomic.update_wm = true;
> >> +
> >> +		if (INTEL_INFO(dev)->gen >= 9 &&
> >> +		    plane->base.type != DRM_PLANE_TYPE_CURSOR) {
> >> +			ret = skl_update_scaler_users(intel_crtc, pipe_config,
> >> +					to_intel_plane(plane),
> >> +					to_intel_plane_state(plane_state), 0);
> >> +			if (ret)
> >> +				return ret;
> >> +		}
> >> +
> >> +		/*
> >> +		 * 'prepare' is never called when plane is being disabled, so
> >> +		 * we need to handle frontbuffer tracking as a special case
> >> +		 */
> >> +		if (old_plane_state->base.fb && !visible)
> >> +			intel_crtc->atomic.disabled_planes |= 1 << i;
> >> +
> >> +		switch (plane->base.type) {
> > We should probably store the frontbuffer tracking mask in intel_plane
> > somewhere.
> 
> Indeed, but this code's already complicated enough so I felt that initially doing
> a 1:1 moving from plane_check to crtc_check would be easier to debug.
> 
> 
> >> <snip>
> > I'm completely lost as to why we need to move all the plane->atomic_check
> > code into a loop in the crtc_check function?
> To calculate what needs to happen during atomic_begin and atomic_flush() on each crtc.

Well we need to run the code, that's clear. But why do we need to move it
out of the plane->atomic_check functions? With the following sequence

1. update modeset states, ensure crtc_state->mode_changed is accurate
2. add all plane_states for a crtc if mode_changed is set
3. call helper_check_planes

plane->atomic_check should always be called, and always with the right
state. Is there some refactor ordering issue I'm missing that prevents us
going for this directly?

> 
> >> <snip>
> >> @@ -13306,20 +13427,26 @@ intel_disable_cursor_plane(struct drm_plane *plane,
> >>  
> >>  static void
> >>  intel_commit_cursor_plane(struct drm_plane *plane,
> >> -			  struct intel_plane_state *state)
> >> +			  struct intel_plane_state *new_state)
> >>  {
> >> -	struct drm_crtc *crtc = state->base.crtc;
> >> +	struct drm_crtc *crtc = new_state->base.crtc;
> >>  	struct drm_device *dev = plane->dev;
> >>  	struct intel_crtc *intel_crtc;
> >> -	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
> >> +	struct drm_i915_gem_object *obj = intel_fb_obj(new_state->base.fb);
> >>  	uint32_t addr;
> >>  
> >>  	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >>  
> >> -	plane->fb = state->base.fb;
> >> -	crtc->cursor_x = state->base.crtc_x;
> >> -	crtc->cursor_y = state->base.crtc_y;
> >> +	plane->fb = new_state->base.fb;
> >> +	crtc->cursor_x = new_state->base.crtc_x;
> >> +	crtc->cursor_y = new_state->base.crtc_y;
> >> +
> >> +	if (!new_state->visible ||
> >> +	    WARN_ON(new_state->visible && !crtc->state->active)) {
> > As an aside: We must move over to compute ->visible irrespective of
> > ->active. I haven't check whether you fix that later on already, but this
> > is important to be able to check wm constraints correctly. The
> > crtc_state->active handling should imo be done in the common code by
> > simply not calling any of the plane or crtc functions hw update if the
> > crtc is off.
> In the crtc_check function I'm setting:
> intel_crtc->atomic.update_wm = mode_changed; ?
> 
> It means recalculating wm on dpms off->on too but is that really such a burden?
> Treating crtc->state->active as the only knob for crtc on/off simplifies things.

Not from the computation pov, just from correctnes. We must reject plane
configs that don't have enough fifo space for watermarks irrespective of
crtc_state->active. My comment was also just meant as a question/hint in
the larger picture of moving all the crtc_state->active checks up in the
layering (since platform plane code shouldn't ever care to make sure
there's no bugs).

> > Of course that means we need to add all planes both for ->mode_changed and
> > for ->active_changed, but needs_modeset(crtc_state) takes care of that
> > already.
> This is exactly why I add all planes in the check function.
> 
> > Aside: The idea behind the split between drm_atomic_helper_check_planes
> > and drm_atomic_helper_check_modeset is that the former handles planes-only
> > updates while the latter handles modeset. We can't fully use
> > check_modesets because we don't use crtc helpers, but I think reusing the
> > same idea still has merit:
> >
> > - We create an intel_check_modeset which calls the helper's check_modeset
> >   plus afterwards all the modeset checks (dpll, cloning checks, adding
> >   plane states and global resources) that we do in crtc_compute_config and
> >   friends right now. That one would also add plane states as needed (but
> >   not call plane->atomic_check). All of this would be skipped if
> >   needs_modeset isn't true.
> Might be doable if we do add planes between check_modeset and check_planes.
> 
> Originally I tried it in intel_modeset_compute_config, but that didn't work when
> the crtc was different from the one that had the argument passed.

Right now that should only be possible (in the legacy modeset code) if we
move a connector, which means the other crtc is guaranteed to get
disabled. If adding plane states for that case doesn't (yet) work we could
do an intermediate step and just force-disable all planes if their current
state indicates their on. Not as pretty as walking over old states in the
disable code, but ok as an intermediate step imo.

Once the multi-crtc patch is done we can then switch over to adding plane
states for this case too and the usual state walking loop.

> > - After that we'd call the helper's plane_check functions which does
> >   exclusively plane-update related checks. crtc/plane->atomic_check
> >   wouldn't contain any modeset related checks (like the encoder cloning or
> >   plane adding you're doing right now).
> I think encoder cloning ought to be part of the crtc check since its done on each crtc.
> >> <snip>
> > Ok I think overall this patch is too big and needs to be split a bit. The
> > following should be doable as prep-patches:
> > - set_base_atomic (if we dare to touch it at all)
> I think I'll drop it.
> > - shuffling the crtc modeset check logic around to add plane states as
> >   needed, making sure we only run that if needs_modeset indicates so.
> I think splitting this out would break the transitional helpers, they
> rely on the plane state not being there so the state is correctly
> updated when the transitional plane helpers run, else we would cause
> stale state to be swapped. :-(

Hm right that's an ugly transition indeed. We could pamper over it by
open-coding the helpers and passing in the plane_state to update. Would
mean even more patches ...

> > - Pushing the checks for intel_crtc->active or crtc->state->active out of
> >   the low-level plane code up into higher level atomic plane code (and the
> >   few legacy entry points for plane updates that we still have maybe, but
> >   those should be covered I think with the universal plane + transitional
> >   helpers).
> I fear this might break the transitional helper calls too, the
> transitional helpers require the transitional state to be mostly in sync
> with the atomic state.

Hm yeah. But if we go with the above intermediate step then
crtc_state->active should be always correct and we can move things around
in the plane code without everything getting out of sync. As long as we've
first switched any usage of crtc->active over to crtc_state->active.

> > Then we can put in the meat of this patch, i.e. replacing the helpers with
> > atomic updates in the modeset code.
> I wish that would work. ;-)

Yeah this is a really tricky one. And it's big enough to really scare me,
so if possible I'd prefer a more gradual transition. We've had some really
crazy regression already with all that atomic work, tbh I don't think I'd
have a clue what breaks if a bisect leads to this patch :(

Not sure what to do here really ...
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 86ba4b2c3a65..85b87e4d4b6e 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -110,32 +110,40 @@  static int intel_plane_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
+	intel_state->visible = false;
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
 	 * anything driver-specific we need to test in that case, so
 	 * just return success.
 	 */
-	if (!crtc)
+	if (!crtc) {
+		DRM_DEBUG_ATOMIC("Invisible: no crtc\n");
 		return 0;
+	}
+
+	crtc_state = state->state->crtc_states[drm_crtc_index(crtc)];
+	if (WARN_ON(!crtc_state))
+		return 0;
+
+	if (!crtc_state->enable) {
+		DRM_DEBUG_ATOMIC("Invisible: crtc off\n");
 
-	/* FIXME: temporary hack necessary while we still use the plane update
-	 * helper. */
-	if (state->state) {
-		crtc_state =
-			intel_atomic_get_crtc_state(state->state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	} else {
-		crtc_state = intel_crtc->config;
+		/*
+		 * Probably allowed after converting to atomic. Right
+		 * now it probably means we have the state confused.
+		 */
+		I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY);
+		return 0;
+	}
+
+	if (!crtc_state->active) {
+		DRM_DEBUG_ATOMIC("Invisible: dpms off\n");
+		return 0;
 	}
 
 	/*
@@ -155,24 +163,9 @@  static int intel_plane_atomic_check(struct drm_plane *plane,
 	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
-	intel_state->clip.x2 =
-		crtc_state->base.active ? crtc_state->pipe_src_w : 0;
-	intel_state->clip.y2 =
-		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
-
-	/*
-	 * Disabling a plane is always okay; we just need to update
-	 * fb tracking in a special way since cleanup_fb() won't
-	 * get called by the plane helpers.
-	 */
-	if (state->fb == NULL && plane->state->fb != NULL) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking as a special case
-		 */
-		intel_crtc->atomic.disabled_planes |=
-			(1 << drm_plane_index(plane));
-	}
+	drm_crtc_get_hv_timing(&crtc_state->mode,
+			       &intel_state->clip.x2,
+			       &intel_state->clip.y2);
 
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 956c9964275d..9610f76a2489 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -100,14 +100,16 @@  static void vlv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config);
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state);
 static void intel_begin_crtc_commit(struct drm_crtc *crtc);
 static void intel_finish_crtc_commit(struct drm_crtc *crtc);
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
 static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
 			   int num_connectors);
-static void intel_crtc_enable_planes(struct drm_crtc *crtc);
-static void intel_crtc_disable_planes(struct drm_crtc *crtc);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
+static void intel_post_enable_primary(struct drm_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -2220,28 +2222,6 @@  void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
 	POSTING_READ(reg);
 }
 
-/**
- * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
- * @plane:  plane to be enabled
- * @crtc: crtc for the plane
- *
- * Enable @plane on @crtc, making sure that the pipe is running first.
- */
-static void intel_enable_primary_hw_plane(struct drm_plane *plane,
-					  struct drm_crtc *crtc)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* If the pipe isn't enabled, we can't pump pixels and may hang */
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
-	to_intel_plane_state(plane->state)->visible = true;
-
-	dev_priv->display.update_primary_plane(crtc, plane->fb,
-					       crtc->x, crtc->y);
-}
-
 static bool need_vtd_wa(struct drm_device *dev)
 {
 #ifdef CONFIG_INTEL_IOMMU
@@ -3161,11 +3141,20 @@  intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane_state *plane_state =
+		to_intel_plane_state(crtc->primary->state);
+	bool was_visible = plane_state->visible;
 
-	if (dev_priv->display.disable_fbc)
+	/* Not supported right now by the helper, but lets be thorough. */
+	if (was_visible && !fb)
+		intel_pre_disable_primary(crtc);
+	else if (was_visible && dev_priv->display.disable_fbc)
 		dev_priv->display.disable_fbc(dev);
 
+	plane_state->visible = !!fb;
 	dev_priv->display.update_primary_plane(crtc, fb, x, y);
+	if (!was_visible && fb)
+		intel_post_enable_primary(crtc);
 
 	return 0;
 }
@@ -3192,16 +3181,17 @@  static void intel_update_primary_planes(struct drm_device *dev)
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		drm_modeset_lock(&crtc->mutex, NULL);
-		/*
-		 * FIXME: Once we have proper support for primary planes (and
-		 * disabling them without disabling the entire crtc) allow again
-		 * a NULL crtc->primary->fb.
-		 */
-		if (intel_crtc->active && crtc->primary->fb)
+
+		if (intel_crtc->active) {
+			const struct intel_plane_state *state =
+				to_intel_plane_state(crtc->primary->state);
+
 			dev_priv->display.update_primary_plane(crtc,
-							       crtc->primary->fb,
-							       crtc->x,
-							       crtc->y);
+							state->base.fb,
+							state->src.x1 >> 16,
+							state->src.y1 >> 16);
+		}
+
 		drm_modeset_unlock(&crtc->mutex);
 	}
 }
@@ -4572,20 +4562,6 @@  static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_sprite_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
-	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
-
-	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		intel_plane = to_intel_plane(plane);
-		if (intel_plane->pipe == pipe)
-			intel_plane_restore(&intel_plane->base);
-	}
-}
-
 void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4815,44 +4791,6 @@  intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_crtc_enable_planes(struct drm_crtc *crtc)
-{
-	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_sprite_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	intel_post_enable_primary(crtc);
-}
-
-static void intel_crtc_disable_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-	int pipe = intel_crtc->pipe;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	intel_pre_disable_primary(crtc);
-
-	intel_crtc_dpms_overlay_disable(intel_crtc);
-	for_each_intel_plane(dev, intel_plane) {
-		if (intel_plane->pipe == pipe) {
-			struct drm_crtc *from = intel_plane->base.crtc;
-
-			intel_plane->disable_plane(&intel_plane->base,
-						   from ?: crtc, true);
-		}
-	}
-
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
-}
-
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -11061,6 +10999,7 @@  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.load_lut = intel_crtc_load_lut,
 	.atomic_begin = intel_begin_crtc_commit,
 	.atomic_flush = intel_finish_crtc_commit,
+	.atomic_check = intel_atomic_check_crtc,
 };
 
 /* Transitional helper to copy current connector/encoder state to
@@ -11426,16 +11365,6 @@  intel_modeset_pipe_config(struct drm_crtc *crtc,
 	int i;
 	bool retry = true;
 
-	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
-		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
-		return -EINVAL;
-	}
-
-	if (!check_digital_port_conflicts(state)) {
-		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
-		return -EINVAL;
-	}
-
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =
@@ -11553,9 +11482,27 @@  intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
 
 	intel_shared_dpll_commit(dev_priv);
-	drm_atomic_helper_swap_state(state->dev, state);
+
+	/*
+	 * swap crtc and connector state, plane state is already swapped in
+	 * __intel_set_mode_update_planes. Once .crtc_disable is fixed
+	 * all state should be swapped before disabling crtc's.
+	 */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		crtc->state->state = state;
+		swap(state->crtc_states[i], crtc->state);
+		crtc->state->state = NULL;
+	}
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		connector->state->state = state;
+		swap(state->connector_states[i], connector->state);
+		connector->state->state = NULL;
+	}
 
 	for_each_intel_encoder(dev, intel_encoder) {
 		if (!intel_encoder->base.crtc)
@@ -12163,8 +12110,8 @@  intel_modeset_compute_config(struct drm_crtc *crtc,
 	    WARN_ON(pipe_config->base.active))
 		pipe_config->base.active = false;
 
-	if (!pipe_config->base.enable)
-		return pipe_config;
+	if (!pipe_config->base.active)
+		goto done;
 
 	ret = intel_modeset_pipe_config(crtc, state, pipe_config);
 	if (ret)
@@ -12182,8 +12129,8 @@  intel_modeset_compute_config(struct drm_crtc *crtc,
 	 * required changes and forcing a mode set.
 	 */
 
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
-
+	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
+done:
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);
@@ -12247,6 +12194,11 @@  static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	int ret;
 
+	if (!check_digital_port_conflicts(state)) {
+		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -12267,6 +12219,112 @@  static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void __intel_set_mode_update_planes(struct drm_device *dev,
+					   struct drm_atomic_state *state)
+{
+	int i;
+	struct drm_plane_state *old_plane_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+
+	/*
+	 * For now only swap plane state, will be replaced with a
+	 * call to drm_atomic_helper_swap_state
+	 */
+	for_each_plane_in_state(state, plane, old_plane_state, i) {
+		struct drm_plane *plane = state->planes[i];
+
+		if (!plane)
+			continue;
+
+		plane->state->state = state;
+		swap(state->plane_states[i], plane->state);
+		plane->state->state = NULL;
+	}
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		const struct drm_crtc_helper_funcs *funcs;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_begin)
+			continue;
+
+		/* XXX: Hack because crtc state is not swapped */
+		crtc->state->mode_changed = crtc_state->mode_changed;
+		crtc->state->active_changed = crtc_state->active_changed;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i);
+		funcs->atomic_begin(crtc);
+	}
+
+	for_each_plane_in_state(state, plane, old_plane_state, i) {
+		bool visible = to_intel_plane_state(plane->state)->visible;
+		struct intel_plane *intel_plane = to_intel_plane(plane);
+		const struct drm_plane_helper_funcs *funcs =
+			plane->helper_private;
+
+		crtc = plane->state->crtc;
+
+		/* no point in disabling if already disabled */
+		if (!to_intel_plane_state(old_plane_state)->visible &&
+		    !to_intel_plane_state(old_plane_state)->hw_enabled)
+			continue;
+
+		to_intel_plane_state(plane->state)->hw_enabled = false;
+		DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible);
+
+		if (!visible)
+			funcs->atomic_update(plane, old_plane_state);
+		else if (needs_modeset(crtc->state))
+			intel_plane->disable_plane(plane, crtc, true);
+	}
+}
+
+static void __intel_set_mode_cleanup_planes(struct drm_device *dev,
+					    struct drm_atomic_state *old_state)
+{
+	int nplanes = dev->mode_config.num_total_plane;
+	int ncrtcs = dev->mode_config.num_crtc;
+	int i;
+
+	for (i = 0; i < nplanes; i++) {
+		const struct drm_plane_helper_funcs *funcs;
+		struct drm_plane *plane = old_state->planes[i];
+		struct drm_plane_state *old_plane_state;
+
+		if (!plane)
+			continue;
+
+		funcs = plane->helper_private;
+		old_plane_state = old_state->plane_states[i];
+
+		if (to_intel_plane_state(plane->state)->visible) {
+			DRM_DEBUG_ATOMIC("Plane %i is updated\n", i);
+			funcs->atomic_update(plane, old_plane_state);
+		} else
+			DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i);
+	}
+
+	for (i = 0; i < ncrtcs; i++) {
+		const struct drm_crtc_helper_funcs *funcs;
+		struct drm_crtc *crtc = old_state->crtcs[i];
+
+		if (!crtc)
+			continue;
+
+		funcs = crtc->helper_private;
+
+		if (!funcs || !funcs->atomic_flush)
+			continue;
+
+		DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i);
+		funcs->atomic_flush(crtc);
+	}
+	drm_atomic_helper_cleanup_planes(dev, old_state);
+}
+
 static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			    struct intel_crtc_state *pipe_config)
 {
@@ -12275,7 +12333,7 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	struct drm_atomic_state *state = pipe_config->base.state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
+	int ret;
 	int i;
 
 	ret = __intel_set_mode_checks(state);
@@ -12286,14 +12344,14 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 	if (ret)
 		return ret;
 
+	__intel_set_mode_update_planes(dev, state);
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state))
 			continue;
 
-		intel_crtc_disable_planes(crtc);
+		intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc));
 		dev_priv->display.crtc_disable(crtc);
-		if (!crtc_state->enable)
-			drm_plane_helper_disable(crtc->primary);
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -12305,8 +12363,6 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	modeset_update_crtc_power_domains(state);
 
-	drm_atomic_helper_commit_planes(dev, state);
-
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!crtc->state->active)
@@ -12314,13 +12370,13 @@  static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 		update_scanline_offset(to_intel_crtc(crtc));
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
+		if (needs_modeset(crtc->state))
+			dev_priv->display.crtc_enable(crtc);
 	}
 
-	/* FIXME: add subpixel order */
+	__intel_set_mode_cleanup_planes(dev, state);
 
-	drm_atomic_helper_cleanup_planes(dev, state);
+	/* FIXME: add subpixel order */
 
 	drm_atomic_state_free(state);
 
@@ -12568,20 +12624,11 @@  intel_modeset_stage_output_state(struct drm_device *dev,
 	return 0;
 }
 
-static bool primary_plane_visible(struct drm_crtc *crtc)
-{
-	struct intel_plane_state *plane_state =
-		to_intel_plane_state(crtc->primary->state);
-
-	return plane_state->visible;
-}
-
 static int intel_crtc_set_config(struct drm_mode_set *set)
 {
 	struct drm_device *dev;
 	struct drm_atomic_state *state = NULL;
 	struct intel_crtc_state *pipe_config;
-	bool primary_plane_was_visible;
 	int ret;
 
 	BUG_ON(!set);
@@ -12620,38 +12667,7 @@  static int intel_crtc_set_config(struct drm_mode_set *set)
 
 	intel_update_pipe_size(to_intel_crtc(set->crtc));
 
-	primary_plane_was_visible = primary_plane_visible(set->crtc);
-
 	ret = intel_set_mode_with_config(set->crtc, pipe_config);
-
-	if (ret == 0 &&
-	    pipe_config->base.enable &&
-	    pipe_config->base.planes_changed &&
-	    !needs_modeset(&pipe_config->base)) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
-
-		/*
-		 * We need to make sure the primary plane is re-enabled if it
-		 * has previously been turned off.
-		 */
-		if (ret == 0 && !primary_plane_was_visible &&
-		    primary_plane_visible(set->crtc)) {
-			WARN_ON(!intel_crtc->active);
-			intel_post_enable_primary(set->crtc);
-		}
-
-		/*
-		 * In the fastboot case this may be our only check of the
-		 * state after boot.  It would be better to only do it on
-		 * the first update, but we don't have a nice way of doing that
-		 * (and really, set_config isn't used much for high freq page
-		 * flipping, so increasing its cost here shouldn't be a big
-		 * deal).
-		 */
-		if (i915.fastboot && ret == 0)
-			intel_modeset_check_state(set->crtc->dev);
-	}
-
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
 			      set->crtc->base.id, ret);
@@ -12791,6 +12807,9 @@  bool intel_wm_need_update(struct drm_plane *plane,
 	    plane->state->rotation != state->rotation)
 		return true;
 
+	if (plane->state->crtc_w != state->crtc_w)
+		return true;
+
 	return false;
 }
 
@@ -12819,6 +12838,9 @@  intel_prepare_plane_fb(struct drm_plane *plane,
 	unsigned frontbuffer_bits = 0;
 	int ret = 0;
 
+	if (!to_intel_plane_state(plane->state)->visible)
+		old_obj = NULL;
+
 	if (!obj)
 		return 0;
 
@@ -12915,10 +12937,8 @@  intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
 	struct drm_framebuffer *fb = state->base.fb;
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
@@ -12926,90 +12946,46 @@  intel_check_primary_plane(struct drm_plane *plane,
 	bool can_position = false;
 	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
-	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
 
 	if (INTEL_INFO(dev)->gen >= 9) {
+		struct intel_crtc_state *crtc_state =
+			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
+
 		min_scale = 1;
 		max_scale = skl_max_scale(intel_crtc, crtc_state);
 		can_position = true;
 	}
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    src, dest, clip,
-					    min_scale,
-					    max_scale,
-					    can_position, true,
-					    &state->visible);
-	if (ret)
-		return ret;
-
-	if (intel_crtc->active) {
-		struct intel_plane_state *old_state =
-			to_intel_plane_state(plane->state);
-
-		intel_crtc->atomic.wait_for_flips = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (state->visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    state->base.rotation != BIT(DRM_ROTATE_0)) {
-			intel_crtc->atomic.disable_fbc = true;
-		}
-
-		if (state->visible && !old_state->visible) {
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev))
-				intel_crtc->atomic.wait_vblank = true;
-		}
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
-
-		intel_crtc->atomic.update_fbc = true;
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-	}
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     src, dest, clip,
+					     min_scale, max_scale,
+					     can_position, true,
+					     &state->visible);
+}
 
-	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_users(intel_crtc, crtc_state,
-			to_intel_plane(plane), state, 0);
-		if (ret)
-			return ret;
-	}
+static void
+intel_disable_primary_plane(struct drm_plane *plane,
+			    struct drm_crtc *crtc,
+			    bool force)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	return 0;
+	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
 }
 
 static void
 intel_commit_primary_plane(struct drm_plane *plane,
-			   struct intel_plane_state *state)
+			   struct intel_plane_state *new_state)
 {
-	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_framebuffer *fb = state->base.fb;
+	struct drm_crtc *crtc = new_state->base.crtc;
+	struct drm_framebuffer *fb = new_state->base.fb;
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	struct drm_rect *src = &state->src;
+	struct drm_rect *src = &new_state->src;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
@@ -13018,25 +12994,178 @@  intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	if (intel_crtc->active) {
-		if (state->visible)
-			/* FIXME: kill this fastboot hack */
-			intel_update_pipe_size(intel_crtc);
+	if (!new_state->visible ||
+	    WARN_ON(new_state->visible && !crtc->state->active)) {
+		intel_disable_primary_plane(plane, crtc, false);
+	} else {
+		/* FIXME: kill this fastboot hack */
+		intel_update_pipe_size(intel_crtc);
 
-		dev_priv->display.update_primary_plane(crtc, plane->fb,
-						       crtc->x, crtc->y);
+		dev_priv->display.update_primary_plane(crtc, fb,
+						       src->x1 >> 16,
+						       src->y1 >> 16);
 	}
 }
 
-static void
-intel_disable_primary_plane(struct drm_plane *plane,
-			    struct drm_crtc *crtc,
-			    bool force)
+/* Transitional checking here, mostly for plane updates */
+static int intel_atomic_check_crtc(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state)
 {
-	struct drm_device *dev = plane->dev;
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *plane;
+	unsigned plane_mask;
+	int idx, ret;
+	bool mode_changed = needs_modeset(crtc_state);
+	bool is_crtc_enabled = crtc_state->active;
+	bool was_crtc_enabled = crtc->state->active;
 
-	dev_priv->display.update_primary_plane(crtc, NULL, 0, 0);
+	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
+		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
+		return -EINVAL;
+	}
+
+	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	intel_crtc->atomic.update_wm = mode_changed;
+
+	idx = crtc->base.id;
+	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
+		"Crtc %i mismatch between state->active(%i) and crtc->active (%i)\n",
+		idx, crtc->state->active, intel_crtc->active);
+
+	DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n",
+			 idx, was_crtc_enabled, is_crtc_enabled);
+
+	plane_mask = crtc_state->plane_mask | crtc->state->plane_mask;
+	drm_for_each_plane_mask(plane, dev, plane_mask) {
+		int i = drm_plane_index(plane);
+		struct drm_plane_state *plane_state = state->plane_states[i];
+		struct intel_plane_state *old_plane_state;
+		bool turn_off, turn_on, visible, was_visible;
+		struct drm_framebuffer *fb;
+
+		if (!plane_state) {
+			const struct drm_plane_helper_funcs *funcs;
+			int ret;
+
+			if (!mode_changed || !plane->state->fb)
+				continue;
+
+			plane_state = drm_atomic_get_plane_state(state, plane);
+			if (IS_ERR(plane_state))
+				return PTR_ERR(plane_state);
+
+			funcs = plane->helper_private;
+			ret = funcs->atomic_check(plane, plane_state);
+			if (ret)
+				return ret;
+		}
+		old_plane_state = to_intel_plane_state(plane->state);
+
+		was_visible = was_crtc_enabled && (old_plane_state->visible ||
+			      old_plane_state->hw_enabled);
+		visible = to_intel_plane_state(plane_state)->visible &&
+			      is_crtc_enabled;
+
+		if (plane->state->crtc != crtc)
+			was_visible = false;
+		if (plane_state->crtc != crtc)
+			visible = false;
+
+		if (!was_visible && !visible)
+			continue;
+
+		turn_off = was_visible && (!visible || mode_changed);
+		turn_on = visible && (!was_visible || mode_changed);
+		fb = plane_state->fb;
+
+		DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx,
+			plane->base.id, fb ? fb->base.id : -1);
+		DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n",
+			was_visible, visible, turn_off, turn_on, mode_changed);
+
+		/* plane being turned off as part of modeset or changes? */
+		if (intel_wm_need_update(plane, plane_state))
+			intel_crtc->atomic.update_wm = true;
+
+		if (INTEL_INFO(dev)->gen >= 9 &&
+		    plane->base.type != DRM_PLANE_TYPE_CURSOR) {
+			ret = skl_update_scaler_users(intel_crtc, pipe_config,
+					to_intel_plane(plane),
+					to_intel_plane_state(plane_state), 0);
+			if (ret)
+				return ret;
+		}
+
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking as a special case
+		 */
+		if (old_plane_state->base.fb && !visible)
+			intel_crtc->atomic.disabled_planes |= 1 << i;
+
+		switch (plane->base.type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			if (visible)
+				intel_crtc->atomic.fb_bits |=
+				    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+			intel_crtc->atomic.wait_for_flips = true;
+			intel_crtc->atomic.pre_disable_primary = turn_off;
+			intel_crtc->atomic.post_enable_primary = turn_on;
+
+			if (turn_off)
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * FBC does not work on some platforms for rotated
+			 * planes, so disable it when rotation is not 0 and
+			 * update it when rotation is set back to 0.
+			 *
+			 * FIXME: This is redundant with the fbc update done in
+			 * the primary plane enable function except that that
+			 * one is done too late. We eventually need to unify
+			 * this.
+			 */
+
+			if (visible &&
+			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+			    dev_priv->fbc.crtc == intel_crtc &&
+			    plane_state->rotation != BIT(DRM_ROTATE_0))
+				intel_crtc->atomic.disable_fbc = true;
+
+			/*
+			 * BDW signals flip done immediately if the plane
+			 * is disabled, even if the plane enable is already
+			 * armed to occur at the next vblank :(
+			 */
+			if (turn_on && IS_BROADWELL(dev))
+				intel_crtc->atomic.wait_vblank = true;
+
+			intel_crtc->atomic.update_fbc = true;
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			if (visible)
+				intel_crtc->atomic.fb_bits |=
+				    INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			if (visible)
+				intel_crtc->atomic.fb_bits |=
+				    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+			if (turn_off) {
+				intel_crtc->atomic.wait_vblank = true;
+				intel_crtc->atomic.update_sprite_watermarks |=
+					1 << i;
+			}
+			break;
+		}
+	}
+	return 0;
 }
 
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
@@ -13087,10 +13216,13 @@  static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (intel_crtc->active)
+	if (intel_crtc->active && !needs_modeset(crtc->state) &&
+	    !dev_priv->power_domains.init_power_on)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
+	else
+		intel_crtc->atomic.evade = false;
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -13099,6 +13231,7 @@  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *p;
+	unsigned plane_mask;
 
 	if (intel_crtc->atomic.evade)
 		intel_pipe_update_end(intel_crtc,
@@ -13120,12 +13253,13 @@  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.post_enable_primary)
 		intel_post_enable_primary(crtc);
 
-	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
-		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
-			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
-						       false, false);
+	plane_mask = intel_crtc->atomic.update_sprite_watermarks;
+	drm_for_each_plane_mask(p, dev, plane_mask)
+		intel_update_sprite_watermarks(p, crtc, 0, 0, 0, false, false);
 
 	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	crtc->state->mode_changed = false;
+	crtc->state->active_changed = false;
 }
 
 /**
@@ -13238,13 +13372,9 @@  intel_check_cursor_plane(struct drm_plane *plane,
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc;
 	unsigned stride;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
@@ -13256,7 +13386,7 @@  intel_check_cursor_plane(struct drm_plane *plane,
 
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		goto finish;
+		return 0;
 
 	/* Check for which cursor types we support */
 	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
@@ -13273,19 +13403,10 @@  intel_check_cursor_plane(struct drm_plane *plane,
 
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
-		ret = -EINVAL;
-	}
-
-finish:
-	if (intel_crtc->active) {
-		if (plane->state->crtc_w != state->base.crtc_w)
-			intel_crtc->atomic.update_wm = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static void
@@ -13306,20 +13427,26 @@  intel_disable_cursor_plane(struct drm_plane *plane,
 
 static void
 intel_commit_cursor_plane(struct drm_plane *plane,
-			  struct intel_plane_state *state)
+			  struct intel_plane_state *new_state)
 {
-	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_crtc *crtc = new_state->base.crtc;
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc;
-	struct drm_i915_gem_object *obj = intel_fb_obj(state->base.fb);
+	struct drm_i915_gem_object *obj = intel_fb_obj(new_state->base.fb);
 	uint32_t addr;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
-	plane->fb = state->base.fb;
-	crtc->cursor_x = state->base.crtc_x;
-	crtc->cursor_y = state->base.crtc_y;
+	plane->fb = new_state->base.fb;
+	crtc->cursor_x = new_state->base.crtc_x;
+	crtc->cursor_y = new_state->base.crtc_y;
+
+	if (!new_state->visible ||
+	    WARN_ON(new_state->visible && !crtc->state->active)) {
+		intel_disable_cursor_plane(plane, crtc, false);
+		return;
+	}
 
 	if (intel_crtc->cursor_bo == obj)
 		goto update;
@@ -13333,10 +13460,9 @@  intel_commit_cursor_plane(struct drm_plane *plane,
 
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
-update:
 
-	if (intel_crtc->active)
-		intel_crtc_update_cursor(crtc, state->visible);
+update:
+	intel_crtc_update_cursor(crtc, new_state->visible);
 }
 
 static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
@@ -14695,7 +14821,14 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev,
 		crtc->base.enabled = crtc->active;
 
 		plane_state = to_intel_plane_state(primary->state);
-		plane_state->visible = primary_get_hw_state(crtc);
+		plane_state->hw_enabled = plane_state->visible =
+			primary_get_hw_state(crtc);
+		if (plane_state->visible)
+			crtc->base.state->plane_mask |=
+				1 << drm_plane_index(primary);
+		else
+			crtc->base.state->plane_mask &=
+				~(1 << drm_plane_index(primary));
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1e892098eea2..9ef89c91aa5c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -235,7 +235,7 @@  struct intel_plane_state {
 	struct drm_rect src;
 	struct drm_rect dst;
 	struct drm_rect clip;
-	bool visible;
+	bool visible, hw_enabled;
 
 	/*
 	 * scaler_id
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 497e7953ad4d..28291ab0993f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -759,7 +759,6 @@  intel_check_sprite_plane(struct drm_plane *plane,
 {
 	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
-	struct intel_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
@@ -771,16 +770,9 @@  intel_check_sprite_plane(struct drm_plane *plane,
 	int hscale, vscale;
 	int max_scale, min_scale;
 	int pixel_size;
-	int ret;
-
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
 
-	if (!fb) {
-		state->visible = false;
-		goto finish;
-	}
+	if (!fb)
+		return 0;
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -803,6 +795,9 @@  intel_check_sprite_plane(struct drm_plane *plane,
 	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
 
 	if (INTEL_INFO(dev)->gen >= 9) {
+		struct intel_crtc_state *crtc_state =
+			intel_atomic_get_crtc_state(state->base.state, intel_crtc);
+
 		min_scale = 1;
 		max_scale = skl_max_scale(intel_crtc, crtc_state);
 	}
@@ -816,7 +811,7 @@  intel_check_sprite_plane(struct drm_plane *plane,
 	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(vscale < 0);
 
-	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	crtc_x = dst->x1;
 	crtc_y = dst->y1;
@@ -921,36 +916,6 @@  intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
-finish:
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	if (intel_crtc->active) {
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-
-		if (!state->visible) {
-			/*
-			 * Avoid underruns when disabling the sprite.
-			 * FIXME remove once watermark updates are done properly.
-			 */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
-	}
-
-	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
-			state, 0);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
@@ -959,7 +924,6 @@  intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
@@ -967,26 +931,22 @@  intel_commit_sprite_plane(struct drm_plane *plane,
 	uint32_t src_x, src_y, src_w, src_h;
 
 	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
 	plane->fb = fb;
 
-	if (intel_crtc->active) {
-		if (state->visible) {
-			crtc_x = state->dst.x1;
-			crtc_y = state->dst.y1;
-			crtc_w = drm_rect_width(&state->dst);
-			crtc_h = drm_rect_height(&state->dst);
-			src_x = state->src.x1 >> 16;
-			src_y = state->src.y1 >> 16;
-			src_w = drm_rect_width(&state->src) >> 16;
-			src_h = drm_rect_height(&state->src) >> 16;
-			intel_plane->update_plane(plane, crtc, fb,
-						  crtc_x, crtc_y, crtc_w, crtc_h,
-						  src_x, src_y, src_w, src_h);
-		} else {
-			intel_plane->disable_plane(plane, crtc, false);
-		}
+	if (state->visible && crtc->state->active) {
+		crtc_x = state->dst.x1;
+		crtc_y = state->dst.y1;
+		crtc_w = drm_rect_width(&state->dst);
+		crtc_h = drm_rect_height(&state->dst);
+		src_x = state->src.x1 >> 16;
+		src_y = state->src.y1 >> 16;
+		src_w = drm_rect_width(&state->src) >> 16;
+		src_h = drm_rect_height(&state->src) >> 16;
+		intel_plane->update_plane(plane, crtc, fb,
+					  crtc_x, crtc_y, crtc_w, crtc_h,
+					  src_x, src_y, src_w, src_h);
+	} else {
+		intel_plane->disable_plane(plane, crtc, false);
 	}
 }