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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Op 22-06-15 om 11:49 schreef Ville Syrjälä:
> 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.
Yes, but this will simplify with a unified atomic commit path.
>> 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
I completely agree, but this becomes a lot easier when all transitional helpers are gone.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx