Re: [PATCH v3 15/19] drm/i915: atomic plane updates in a nutshell

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

On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote:
> Now that all planes are added during a modeset we can use the
> calculated changes before disabling a plane, and then either commit
> or force disable a plane before disabling the crtc.
> 
> The code is shared with atomic_begin/flush, except watermark updating
> and vblank evasion are not used.
> 
> This is needed for proper atomic suspend/resume support.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 103 ++++++++---------------------------
>  drivers/gpu/drm/i915/intel_sprite.c  |   4 +-
>  2 files changed, 23 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cc4ca4970716..beb69281f45c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc)
>  		intel_wait_for_pipe_off(crtc);
>  }
>  
> -/**
> - * 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
> @@ -4508,20 +4486,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;
> @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
>  		intel_pre_disable_primary(&crtc->base);
>  }
>  
> -static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
> -
> -	intel_enable_primary_hw_plane(crtc->primary, crtc);
> -	intel_enable_sprite_planes(crtc);
> -	if (to_intel_plane_state(crtc->cursor->state)->visible)
> -		intel_crtc_update_cursor(crtc, true);
> -
> -	intel_post_enable_primary(crtc);
> -
> -	/*
> -	 * 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 intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask
>  	struct drm_plane *p;
>  	int pipe = intel_crtc->pipe;
>  
> -	intel_crtc_wait_for_pending_flips(crtc);
> -
> -	intel_pre_disable_primary(crtc);
> -
>  	intel_crtc_dpms_overlay_disable(intel_crtc);
>  
>  	drm_for_each_plane_mask(p, dev, plane_mask)
> @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> +	if (to_intel_plane_state(crtc->primary->state)->visible) {
> +		intel_crtc_wait_for_pending_flips(crtc);
> +		intel_pre_disable_primary(crtc);
> +	}
> +
>  	intel_crtc_disable_planes(crtc, crtc->state->plane_mask);
>  	dev_priv->display.crtc_disable(crtc);
>  
> @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	if (old_plane_state->base.fb && !fb)
>  		intel_crtc->atomic.disabled_planes |= 1 << i;
>  
> -	/* don't run rest during modeset yet */
> -	if (!intel_crtc->active || mode_changed)
> -		return 0;
> -
>  	was_visible = old_plane_state->visible;
>  	visible = to_intel_plane_state(plane_state)->visible;
>  
> @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  	drm_atomic_helper_swap_state(dev, state);
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
>  		any_ms = true;
> -		if (!crtc_state->active)
> -			continue;
> +		intel_pre_plane_update(intel_crtc);
>  
> -		intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
> -		dev_priv->display.crtc_disable(crtc);
> +		if (crtc_state->active) {
> +			intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
> +			dev_priv->display.crtc_disable(crtc);
> +		}
>  	}
>  
>  	/* Only after disabling all output pipelines that will be changed can we
> @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
> -
> -		if (!needs_modeset(crtc->state) || !crtc->state->active)
> -			continue;
> -
> -		update_scanline_offset(to_intel_crtc(crtc));
> +		if (needs_modeset(crtc->state) && crtc->state->active) {
> +			update_scanline_offset(to_intel_crtc(crtc));
> +			dev_priv->display.crtc_enable(crtc);
> +		}
>  
> -		dev_priv->display.crtc_enable(crtc);
> -		intel_crtc_enable_planes(crtc);
> +		drm_atomic_helper_commit_planes_on_crtc(crtc_state);

Why are we still commiting planes on a disabled pipes?

The way I envisioned the modeset path looking is something like this:

1. atomically disable all planes for pipes getting disabled
2. disable all pipes that need disabling
3. enable all pipes that need enabling
4. atomically update all planes that require it on now active pipes

>  	}
>  
>  	/* FIXME: add subpixel order */
> @@ -13858,7 +13798,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	crtc->x = src->x1 >> 16;
>  	crtc->y = src->y1 >> 16;
>  
> -	if (!intel_crtc->active)
> +	if (!crtc->state->active)
>  		return;
>  
>  	if (state->visible)
> @@ -13885,7 +13825,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -	intel_pre_plane_update(intel_crtc);
> +	if (!needs_modeset(crtc->state))
> +		intel_pre_plane_update(intel_crtc);
>  
>  	if (intel_crtc->atomic.update_wm)
>  		intel_update_watermarks(crtc);
> @@ -13893,7 +13834,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (crtc->state->active && !needs_modeset(crtc->state))
> +	if (crtc->state->active)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> @@ -14102,7 +14043,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  
>  update:
> -	if (intel_crtc->active)
> +	if (crtc->state->active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index f7eb3bd0a7c7..168f90f346c2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -921,16 +921,14 @@ 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;
>  
>  	crtc = crtc ? crtc : plane->crtc;
> -	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = fb;
>  
> -	if (!intel_crtc->active)
> +	if (!crtc->state->active)
>  		return;
>  
>  	if (state->visible) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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