Re: [PATCH v2 07/14] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> By handling this after the atomic helper waits for vblanks there will
> be one less wait for vblank in the atomic path.

I found this a bit confusing. Sounds like you are referring to calling
intel_post_enable_update() after the call to drm_atomic_helper_wait_for_vblanks(
), except that move wasn't mentioned before. Also, the patch replaces that call
with our own wait for vblank function, so it is probably good to mention why in
the commit message.

> 
> Also get rid of the double wait_for_vblank on broadwell, looks like
> it's a bug doing the vblank wait twice.

Separate patch with more details in the commit message.

Ander

> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 95 +++++++++++++++++++++++------------
> -
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 64 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 3e390db9d22b..2ba0cd698f9b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->visible_changed = false;
>  	crtc_state->wm_changed = false;
> +	crtc_state->fb_changed = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 356e3a9e1741..2708899d9767 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4669,14 +4669,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  
>  	/*
> -	 * 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_wait_for_vblank(dev, pipe);
> -
> -	/*
>  	 * FIXME IPS should be fine as long as one plane is
>  	 * enabled, but in practice it seems to have problems
>  	 * when going from primary only to sprite only and vice
> @@ -4758,9 +4750,6 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	crtc->wm.cxsr_allowed = true;
> @@ -11660,6 +11649,9 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  	if (!was_visible && !visible)
>  		return 0;
>  
> +	if (fb != old_plane_state->base.fb)
> +		pipe_config->fb_changed = true;
> +
>  	turn_off = was_visible && (!visible || mode_changed);
>  	turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -11676,8 +11668,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  			pipe_config->visible_changed = true;
> -			if (is_crtc_enabled)
> -				intel_crtc->atomic.wait_vblank = true;
>  		}
>  	} else if ((was_visible || visible) &&
>  		   intel_wm_need_update(plane, plane_state)) {
> @@ -11724,14 +11714,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		    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 |= visible || mode_changed;
>  		break;
>  	case DRM_PLANE_TYPE_CURSOR:
> @@ -11746,12 +11728,10 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>  		if (IS_IVYBRIDGE(dev) &&
>  		    needs_scaling(to_intel_plane_state(plane_state)) &&
>  		    !needs_scaling(old_plane_state)) {
> -			to_intel_crtc_state(crtc_state)->disable_lp_wm =
> true;
> -		} else if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
> +			pipe_config->disable_lp_wm = true;
> +		} else if (turn_off && !mode_changed)
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
> -		}
>  
>  		break;
>  	}
> @@ -13261,6 +13241,48 @@ static int intel_atomic_prepare_commit(struct
> drm_device *dev,
>  	return ret;
>  }
>  
> +static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> +					  struct drm_i915_private *dev_priv,
> +					  unsigned crtc_mask)
> +{
> +	unsigned last_vblank_count[I915_MAX_PIPES];
> +	enum pipe pipe;
> +	int ret;
> +
> +	if (!crtc_mask)
> +		return;
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		ret = drm_crtc_vblank_get(crtc);
> +		if (ret != 0) {
> +			crtc_mask &= ~(1 << pipe);
> +			continue;
> +		}
> +
> +		last_vblank_count[pipe] = drm_crtc_vblank_count(crtc);
> +	}
> +
> +	for_each_pipe(dev_priv, pipe) {
> +		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +		if (!((1 << pipe) & crtc_mask))
> +			continue;
> +
> +		wait_event_timeout(dev->vblank[pipe].queue,
> +				last_vblank_count[pipe] !=
> +					drm_crtc_vblank_count(crtc),
> +				msecs_to_jiffies(50));
> +
> +		drm_crtc_vblank_put(crtc);
> +	}
> +}
> +
> +
>  /**
>   * intel_atomic_commit - commit validated state object
>   * @dev: DRM device
> @@ -13283,11 +13305,11 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  {
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool hw_check = intel_state->modeset;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> -	int ret = 0;
> -	int i;
> -	bool hw_check = intel_state->modeset;
> +	int ret = 0, i;
> +	unsigned crtc_vblank_mask = 0;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13335,8 +13357,9 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  		bool modeset = needs_modeset(crtc->state);
> -		bool update_pipe = !modeset &&
> -			to_intel_crtc_state(crtc->state)->update_pipe;
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc->state);
> +		bool update_pipe = !modeset && pipe_config->update_pipe;
>  		unsigned long put_domains = 0;
>  
>  		if (modeset && crtc->state->active) {
> @@ -13358,15 +13381,21 @@ static int intel_atomic_commit(struct drm_device
> *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>  
> +		if (pipe_config->base.active &&
> +		    (pipe_config->wm_changed || pipe_config->visible_changed
> ||
> +		     pipe_config->fb_changed))
> +			crtc_vblank_mask |= 1 << i;
> +
>  		if (put_domains)
>  			modeset_put_power_domains(dev_priv, put_domains);
> -
> -		intel_post_plane_update(intel_crtc);
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
> -	drm_atomic_helper_wait_for_vblanks(dev, state);
> +	intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +		intel_post_plane_update(to_intel_crtc(crtc));
>  
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 59367453d24b..6f99c7398af3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -375,6 +375,7 @@ struct intel_crtc_state {
>  	bool update_pipe; /* can a fast modeset be performed? */
>  	bool visible_changed; /* plane visibility changed */
>  	bool wm_changed; /* wm changed */
> +	bool fb_changed; /* fb on any of the planes is changed */
>  
>  	/* Pipe source size (ie. panel fitter input size)
>  	 * All planes will be positioned inside this space,
> @@ -541,7 +542,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx