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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 15/19] drm/i915: atomic plane updates in a nutshell
Date: Mon, 22 Jun 2015 12:49:16 +0300	[thread overview]
Message-ID: <20150622094916.GR5176@intel.com> (raw)
In-Reply-To: <55839403.9000104@linux.intel.com>

On Fri, Jun 19, 2015 at 06:01:07AM +0200, Maarten Lankhorst wrote:
> Op 18-06-15 om 17:28 schreef Ville Syrjälä:
> > 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
> >
> >
> I looked for other ways to do this, but they were ugly. Only updating sw state in commit_planes for disabled crtc's made things a lot cleaner without worrying about special cases.

The commit hooks shouldn't update any sw state.

As for special cases, the current code has one special case after another,
and it's giving me a headache every time I look at it.

> 
> plane_disable doesn't update sw state, so it performs 1. The commit function is still called because update plane->fb
> needs to be updated, even for disabled crtc's. Conditionally calling plane_commit caused more issues than it solved.

plane->fb should be clearly moved out of the commit hooks to where the
rest of the state gets swapped (assuming we can't just get rid of it).

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