[v4,3/8] drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4.

Submitted by Maarten Lankhorst on Feb. 10, 2016, 12:49 p.m.

Details

Message ID 1455108583-29227-4-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show
Series "Kill off intel_crtc->atomic!" ( rev: 10 9 8 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Maarten Lankhorst Feb. 10, 2016, 12:49 p.m.
Currently we perform our own wait in post_plane_update,
but the atomic core performs another one in wait_for_vblanks.
This means that 2 vblanks are done when a fb is changed,
which is a bit overkill.

Merge them by creating a helper function that takes a crtc mask
for the planes to wait on.

The broadwell vblank workaround may look gone entirely but this is
not the case. pipe_config->wm_changed is set to true
when any plane is turned on, which forces a vblank wait.

Changes since v1:
- Removing the double vblank wait on broadwell moved to its own commit.
Changes since v2:
- Move out POWER_DOMAIN_MODESET handling to its own commit.
Changes since v3:
- Do not wait for vblank on legacy cursor updates. (Ville)
- Move broadwell vblank workaround comment to page_flip_finished. (Ville)
Changes since v4:
- Compile fix, legacy_cursor_flip -> *_update.

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 | 86 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 67 insertions(+), 22 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4785,9 +4785,6 @@  static void intel_post_plane_update(struct intel_crtc *crtc)
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
 
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
 	intel_frontbuffer_flip(dev, atomic->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;
@@ -10902,6 +10899,12 @@  static bool page_flip_finished(struct intel_crtc *crtc)
 		return 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 :(
+	 */
+
+	/*
 	 * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
 	 * used the same base address. In that case the mmio flip might
 	 * have completed, but the CS hasn't even executed the flip yet.
@@ -11778,6 +11781,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);
 
@@ -11793,8 +11799,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) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank = true;
 			pipe_config->disable_cxsr = true;
 		}
 	} else if (intel_wm_need_update(plane, plane_state)) {
@@ -11810,14 +11814,6 @@  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.post_enable_primary = turn_on;
 		intel_crtc->atomic.update_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;
-
 		break;
 	case DRM_PLANE_TYPE_CURSOR:
 		break;
@@ -11831,12 +11827,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;
 	}
@@ -13370,6 +13364,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
@@ -13397,6 +13433,7 @@  static int intel_atomic_commit(struct drm_device *dev,
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
 	unsigned long put_domains[I915_MAX_PIPES] = {};
+	unsigned crtc_vblank_mask = 0;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13470,8 +13507,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;
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
@@ -13488,14 +13526,20 @@  static int intel_atomic_commit(struct drm_device *dev,
 		    (crtc->state->planes_changed || update_pipe))
 			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config->disable_cxsr ||
+		     pipe_config->fb_changed))
+			crtc_vblank_mask |= 1 << i;
 	}
 
 	/* FIXME: add subpixel order */
 
-	drm_atomic_helper_wait_for_vblanks(dev, state);
+	if (!state->legacy_cursor_update)
+		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));
+
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 335e6b24b0bc..e911c86f873b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -379,6 +379,7 @@  struct intel_crtc_state {
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */
+	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,
@@ -547,7 +548,6 @@  struct intel_crtc_atomic_commit {
 
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
-	bool wait_vblank;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
 

Comments

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Currently we perform our own wait in post_plane_update,

> but the atomic core performs another one in wait_for_vblanks.

> This means that 2 vblanks are done when a fb is changed,

> which is a bit overkill.

> 

> Merge them by creating a helper function that takes a crtc mask

> for the planes to wait on.

> 

> The broadwell vblank workaround may look gone entirely but this is

> not the case. pipe_config->wm_changed is set to true

> when any plane is turned on, which forces a vblank wait.

> 

> Changes since v1:

> - Removing the double vblank wait on broadwell moved to its own

> commit.

> Changes since v2:

> - Move out POWER_DOMAIN_MODESET handling to its own commit.

> Changes since v3:

> - Do not wait for vblank on legacy cursor updates. (Ville)

> - Move broadwell vblank workaround comment to page_flip_finished.

> (Ville)

> Changes since v4:

> - Compile fix, legacy_cursor_flip -> *_update.

> 

> 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 | 86

> +++++++++++++++++++++++++++---------

>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-

>  3 files changed, 67 insertions(+), 22 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_atomic.c

> b/drivers/gpu/drm/i915/intel_atomic.c

> index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644

> --- a/drivers/gpu/drm/i915/intel_display.c

> +++ b/drivers/gpu/drm/i915/intel_display.c

> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct

> intel_crtc *crtc)

>  		to_intel_crtc_state(crtc->base.state);

>  	struct drm_device *dev = crtc->base.dev;

>  

> -	if (atomic->wait_vblank)

> -		intel_wait_for_vblank(dev, crtc->pipe);

> -

>  	intel_frontbuffer_flip(dev, atomic->fb_bits);

>  

>  	crtc->wm.cxsr_allowed = true;

> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct

> intel_crtc *crtc)

>  		return 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 :(

> +	 */


Having this comment here is just... weird. I think it removes a lot of
the context that was present before.

> +

> +	/*

>  	 * A DSPSURFLIVE check isn't enough in case the mmio and CS

> flips

>  	 * used the same base address. In that case the mmio flip

> might

>  	 * have completed, but the CS hasn't even executed the flip

> yet.

> @@ -11778,6 +11781,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);

>  

> @@ -11793,8 +11799,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) {

> -			if (is_crtc_enabled)

> -				intel_crtc->atomic.wait_vblank =

> true;

>  			pipe_config->disable_cxsr = true;

>  		}


We could have killed the brackets here :)

>  	} else if (intel_wm_need_update(plane, plane_state)) {

> @@ -11810,14 +11814,6 @@ int intel_plane_atomic_calc_changes(struct

> drm_crtc_state *crtc_state,

>  		intel_crtc->atomic.post_enable_primary = turn_on;

>  		intel_crtc->atomic.update_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;

> -

>  		break;

>  	case DRM_PLANE_TYPE_CURSOR:

>  		break;

> @@ -11831,12 +11827,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;

> -		}


Weird brackets here. Either kill on both sides of the if statement (the
preferred way), or none.

Also, shouldn't we introduce pipe_config->sprite_changed? What's
guaranteeing that we're going to definitely wait for a vblank during
sprite updates? I like explicit dumb-proof code instead of implications
such as "we're doing waits during sprite updates because whenever we
update sprites, a specific unrelated variable that's used for a
different purpose gets set to true, and we check for this variable".

>  

>  		break;

>  	}

> @@ -13370,6 +13364,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;


Shouldn't we DRM_ERROR here?

> +		}

> +

> +		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));


Also maybe DRM_ERROR in case we hit the timeout?

I know the original code doesn't have this, but now that we're doing or
own thing, we may scream in unexpected cases.

> +

> +		drm_crtc_vblank_put(crtc);

> +	}

> +}

> +

> +


Two newlines :)

>  /**

>   * intel_atomic_commit - commit validated state object

>   * @dev: DRM device

> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct

> drm_device *dev,

>  	int ret = 0, i;

>  	bool hw_check = intel_state->modeset;

>  	unsigned long put_domains[I915_MAX_PIPES] = {};

> +	unsigned crtc_vblank_mask = 0;

>  

>  	ret = intel_atomic_prepare_commit(dev, state, async);

>  	if (ret) {

> @@ -13470,8 +13507,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;

>  

>  		if (modeset && crtc->state->active) {

>  			update_scanline_offset(to_intel_crtc(crtc));

> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct

> drm_device *dev,

>  		    (crtc->state->planes_changed || update_pipe))

>  			drm_atomic_helper_commit_planes_on_crtc(crtc

> _state);

>  

> -		intel_post_plane_update(intel_crtc);

> +		if (pipe_config->base.active &&

> +		    (pipe_config->wm_changed || pipe_config-

> >disable_cxsr ||

> +		     pipe_config->fb_changed))


So the wm_changed is just for the BDW workaround + sprites? What about
this disable_cxsr, why is it here? Also see my comment above about
sprite_changed. Maybe we need some comments here to explain the complex
checks.


> +			crtc_vblank_mask |= 1 << i;

>  	}

>  

>  	/* FIXME: add subpixel order */

>  

> -	drm_atomic_helper_wait_for_vblanks(dev, state);

> +	if (!state->legacy_cursor_update)

> +		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));

> +

>  		if (put_domains[i])

>  			modeset_put_power_domains(dev_priv,

> put_domains[i]);

>  	}

> diff --git a/drivers/gpu/drm/i915/intel_drv.h

> b/drivers/gpu/drm/i915/intel_drv.h

> index 335e6b24b0bc..e911c86f873b 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -379,6 +379,7 @@ struct intel_crtc_state {

>  	bool update_pipe; /* can a fast modeset be performed? */

>  	bool disable_cxsr;

>  	bool wm_changed; /* watermarks are updated */

> +	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,

> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {

>  

>  	/* Sleepable operations to perform after commit */

>  	unsigned fb_bits;

> -	bool wait_vblank;


One of the things that I like about the code without this patch is that
it's very explicit on when we need to wait for vblanks (except for the
problem where we wait twice). The new code is not as easy to
read/understand as the old one. This comment is similar to the one I
made in patch 6: I'm not sure if sacrificing readability is worth it.


I wonder that maybe the cleanest fix to the "we're waiting 2 vblanks"
problem is to just remove the call to
drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
Then we'd have a second patch introducing
intel_atomic_wait_for_vblanks() for the "wait for all vblanks at the
same time" optimization, and then a last commit possibly replacing
commit->wait_vblank for state->fb_changed. Just an idea, not a request.


I'll wait for your answers before reaching any conclusions of what I
think should be done, since I may be misunderstanding something.

>  	bool post_enable_primary;

>  	unsigned update_sprite_watermarks;

>
Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>> Currently we perform our own wait in post_plane_update,
>> but the atomic core performs another one in wait_for_vblanks.
>> This means that 2 vblanks are done when a fb is changed,
>> which is a bit overkill.
>>
>> Merge them by creating a helper function that takes a crtc mask
>> for the planes to wait on.
>>
>> The broadwell vblank workaround may look gone entirely but this is
>> not the case. pipe_config->wm_changed is set to true
>> when any plane is turned on, which forces a vblank wait.
>>
>> Changes since v1:
>> - Removing the double vblank wait on broadwell moved to its own
>> commit.
>> Changes since v2:
>> - Move out POWER_DOMAIN_MODESET handling to its own commit.
>> Changes since v3:
>> - Do not wait for vblank on legacy cursor updates. (Ville)
>> - Move broadwell vblank workaround comment to page_flip_finished.
>> (Ville)
>> Changes since v4:
>> - Compile fix, legacy_cursor_flip -> *_update.
>>
>> 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 | 86
>> +++++++++++++++++++++++++++---------
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>  3 files changed, 67 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
>> intel_crtc *crtc)
>>  		to_intel_crtc_state(crtc->base.state);
>>  	struct drm_device *dev = crtc->base.dev;
>>  
>> -	if (atomic->wait_vblank)
>> -		intel_wait_for_vblank(dev, crtc->pipe);
>> -
>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>  
>>  	crtc->wm.cxsr_allowed = true;
>> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
>> intel_crtc *crtc)
>>  		return 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 :(
>> +	 */
> Having this comment here is just... weird. I think it removes a lot of
> the context that was present before.
>
>> +
>> +	/*
>>  	 * A DSPSURFLIVE check isn't enough in case the mmio and CS
>> flips
>>  	 * used the same base address. In that case the mmio flip
>> might
>>  	 * have completed, but the CS hasn't even executed the flip
>> yet.
>> @@ -11778,6 +11781,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);
>>  
>> @@ -11793,8 +11799,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) {
>> -			if (is_crtc_enabled)
>> -				intel_crtc->atomic.wait_vblank =
>> true;
>>  			pipe_config->disable_cxsr = true;
>>  		}
> We could have killed the brackets here :)
Indeed, will do so in next version.
>>  	} else if (intel_wm_need_update(plane, plane_state)) {
>> @@ -11810,14 +11814,6 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>  		intel_crtc->atomic.post_enable_primary = turn_on;
>>  		intel_crtc->atomic.update_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;
>> -
>>  		break;
>>  	case DRM_PLANE_TYPE_CURSOR:
>>  		break;
>> @@ -11831,12 +11827,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;
>> -		}
> Weird brackets here. Either kill on both sides of the if statement (the
> preferred way), or none.
>
> Also, shouldn't we introduce pipe_config->sprite_changed? What's
> guaranteeing that we're going to definitely wait for a vblank during
> sprite updates? I like explicit dumb-proof code instead of implications
> such as "we're doing waits during sprite updates because whenever we
> update sprites, a specific unrelated variable that's used for a
> different purpose gets set to true, and we check for this variable".
sprite_changed would be same as fb_changed + wm_changed, and update_sprite_watermarks gets removed in this series
because nothing uses it.
>>  
>>  		break;
>>  	}
>> @@ -13370,6 +13364,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;
> Shouldn't we DRM_ERROR here?
WARN_ON is enough, this shouldn't ever happen.
>> +		}
>> +
>> +		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));
> Also maybe DRM_ERROR in case we hit the timeout?
>
> I know the original code doesn't have this, but now that we're doing or
> own thing, we may scream in unexpected cases.
I'll make it a WARN_ON, shouldn't happen.
>> +
>> +		drm_crtc_vblank_put(crtc);
>> +	}
>> +}
>> +
>> +
> Two newlines :)
Indeed!
>>  /**
>>   * intel_atomic_commit - commit validated state object
>>   * @dev: DRM device
>> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  	int ret = 0, i;
>>  	bool hw_check = intel_state->modeset;
>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>> +	unsigned crtc_vblank_mask = 0;
>>  
>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>  	if (ret) {
>> @@ -13470,8 +13507,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;
>>  
>>  		if (modeset && crtc->state->active) {
>>  			update_scanline_offset(to_intel_crtc(crtc));
>> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
>> drm_device *dev,
>>  		    (crtc->state->planes_changed || update_pipe))
>>  			drm_atomic_helper_commit_planes_on_crtc(crtc
>> _state);
>>  
>> -		intel_post_plane_update(intel_crtc);
>> +		if (pipe_config->base.active &&
>> +		    (pipe_config->wm_changed || pipe_config-
>>> disable_cxsr ||
>> +		     pipe_config->fb_changed))
> So the wm_changed is just for the BDW workaround + sprites? What about
> this disable_cxsr, why is it here? Also see my comment above about
> sprite_changed. Maybe we need some comments here to explain the complex
> checks.
No it's waiting for a vblank for post_plane_update so all optimized wm values
can get written, it's not just for the BDW workaround.
It just happens to be that the BDW w/a gets applied too as a side effect.
>> +			crtc_vblank_mask |= 1 << i;
>>  	}
>>  
>>  	/* FIXME: add subpixel order */
>>  
>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>> +	if (!state->legacy_cursor_update)
>> +		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));
>> +
>>  		if (put_domains[i])
>>  			modeset_put_power_domains(dev_priv,
>> put_domains[i]);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 335e6b24b0bc..e911c86f873b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>>  	bool update_pipe; /* can a fast modeset be performed? */
>>  	bool disable_cxsr;
>>  	bool wm_changed; /* watermarks are updated */
>> +	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,
>> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>>  
>>  	/* Sleepable operations to perform after commit */
>>  	unsigned fb_bits;
>> -	bool wait_vblank;
> One of the things that I like about the code without this patch is that
> it's very explicit on when we need to wait for vblanks (except for the
> problem where we wait twice). The new code is not as easy to
> read/understand as the old one. This comment is similar to the one I
> made in patch 6: I'm not sure if sacrificing readability is worth it.
>
> I wonder that maybe the cleanest fix to the "we're waiting 2 vblanks"
> problem is to just remove the call to
> drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
> Then we'd have a second patch introducing
> intel_atomic_wait_for_vblanks() for the "wait for all vblanks at the
> same time" optimization, and then a last commit possibly replacing
> commit->wait_vblank for state->fb_changed. Just an idea, not a request.
There were cases in which the atomic helper would wait for vblanks which
wouldn't trigger any of the other changes, so it can't be blindly removed. Mostly when
updating a plane with a same sized fb.
Wait for vblank is required for unpinning, it would be bad if the current fb is unpinned.

> I'll wait for your answers before reaching any conclusions of what I
> think should be done, since I may be misunderstanding something.
I want to call all post flip work scheduled later on so it happens after the next vblank.
That will remove all confusion on when a vblank is required, because all post_plane_update
and unpin will always wait until next vblank.
Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:

> > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:

> > > Currently we perform our own wait in post_plane_update,

> > > but the atomic core performs another one in wait_for_vblanks.

> > > This means that 2 vblanks are done when a fb is changed,

> > > which is a bit overkill.

> > > 

> > > Merge them by creating a helper function that takes a crtc mask

> > > for the planes to wait on.

> > > 

> > > The broadwell vblank workaround may look gone entirely but this

> > > is

> > > not the case. pipe_config->wm_changed is set to true

> > > when any plane is turned on, which forces a vblank wait.

> > > 

> > > Changes since v1:

> > > - Removing the double vblank wait on broadwell moved to its own

> > > commit.

> > > Changes since v2:

> > > - Move out POWER_DOMAIN_MODESET handling to its own commit.

> > > Changes since v3:

> > > - Do not wait for vblank on legacy cursor updates. (Ville)

> > > - Move broadwell vblank workaround comment to page_flip_finished.

> > > (Ville)

> > > Changes since v4:

> > > - Compile fix, legacy_cursor_flip -> *_update.

> > > 

> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c

> > > om>

> > > ---

> > >  drivers/gpu/drm/i915/intel_atomic.c  |  1 +

> > >  drivers/gpu/drm/i915/intel_display.c | 86

> > > +++++++++++++++++++++++++++---------

> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-

> > >  3 files changed, 67 insertions(+), 22 deletions(-)

> > > 

> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c

> > > b/drivers/gpu/drm/i915/intel_atomic.c

> > > index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644

> > > --- a/drivers/gpu/drm/i915/intel_display.c

> > > +++ b/drivers/gpu/drm/i915/intel_display.c

> > > @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct

> > > intel_crtc *crtc)

> > >  		to_intel_crtc_state(crtc->base.state);

> > >  	struct drm_device *dev = crtc->base.dev;

> > >  

> > > -	if (atomic->wait_vblank)

> > > -		intel_wait_for_vblank(dev, crtc->pipe);

> > > -

> > >  	intel_frontbuffer_flip(dev, atomic->fb_bits);

> > >  

> > >  	crtc->wm.cxsr_allowed = true;

> > > @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct

> > > intel_crtc *crtc)

> > >  		return 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 :(

> > > +	 */

> > Having this comment here is just... weird. I think it removes a lot

> > of

> > the context that was present before.

> > 

> > > +

> > > +	/*

> > >  	 * A DSPSURFLIVE check isn't enough in case the mmio and

> > > CS

> > > flips

> > >  	 * used the same base address. In that case the mmio

> > > flip

> > > might

> > >  	 * have completed, but the CS hasn't even executed the

> > > flip

> > > yet.

> > > @@ -11778,6 +11781,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);

> > >  

> > > @@ -11793,8 +11799,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) {

> > > -			if (is_crtc_enabled)

> > > -				intel_crtc->atomic.wait_vblank =

> > > true;

> > >  			pipe_config->disable_cxsr = true;

> > >  		}

> > We could have killed the brackets here :)

> Indeed, will do so in next version.

> > >  	} else if (intel_wm_need_update(plane, plane_state)) {

> > > @@ -11810,14 +11814,6 @@ int

> > > intel_plane_atomic_calc_changes(struct

> > > drm_crtc_state *crtc_state,

> > >  		intel_crtc->atomic.post_enable_primary =

> > > turn_on;

> > >  		intel_crtc->atomic.update_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;

> > > -

> > >  		break;

> > >  	case DRM_PLANE_TYPE_CURSOR:

> > >  		break;

> > > @@ -11831,12 +11827,10 @@ int

> > > intel_plane_atomic_calc_changes(struct

> > > drm_crtc_state *crtc_state,

> > >  		if (IS_IVYBRIDGE(dev) &&

> > >  		    needs_scaling(to_intel_plane_state(plane_sta

> > > te))

> > > &&

> > >  		    !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;

> > > -		}

> > Weird brackets here. Either kill on both sides of the if statement

> > (the

> > preferred way), or none.

> > 

> > Also, shouldn't we introduce pipe_config->sprite_changed? What's

> > guaranteeing that we're going to definitely wait for a vblank

> > during

> > sprite updates? I like explicit dumb-proof code instead of

> > implications

> > such as "we're doing waits during sprite updates because whenever

> > we

> > update sprites, a specific unrelated variable that's used for a

> > different purpose gets set to true, and we check for this

> > variable".

> sprite_changed would be same as fb_changed + wm_changed, and

> update_sprite_watermarks gets removed in this series

> because nothing uses it.


Hmmm, right. For some reason, I was interpreting fb_changed as being
only valid for the primary fb, not for spr/cur. My bad. So fb_changed
means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
could be a better name, or fbs_changed (plural), since we're talking
about more then one possible plane here. Not a requirement, just
throwing the idea.

> > >  

> > >  		break;

> > >  	}

> > > @@ -13370,6 +13364,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;

> > Shouldn't we DRM_ERROR here?

> WARN_ON is enough, this shouldn't ever happen.


Even better :)

> > > +		}

> > > +

> > > +		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(cr

> > > tc),

> > > +				msecs_to_jiffies(50));

> > Also maybe DRM_ERROR in case we hit the timeout?

> > 

> > I know the original code doesn't have this, but now that we're

> > doing or

> > own thing, we may scream in unexpected cases.

> I'll make it a WARN_ON, shouldn't happen.

> > > +

> > > +		drm_crtc_vblank_put(crtc);

> > > +	}

> > > +}

> > > +

> > > +

> > Two newlines :)

> Indeed!

> > >  /**

> > >   * intel_atomic_commit - commit validated state object

> > >   * @dev: DRM device

> > > @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct

> > > drm_device *dev,

> > >  	int ret = 0, i;

> > >  	bool hw_check = intel_state->modeset;

> > >  	unsigned long put_domains[I915_MAX_PIPES] = {};

> > > +	unsigned crtc_vblank_mask = 0;

> > >  

> > >  	ret = intel_atomic_prepare_commit(dev, state, async);

> > >  	if (ret) {

> > > @@ -13470,8 +13507,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;

> > >  

> > >  		if (modeset && crtc->state->active) {

> > >  			update_scanline_offset(to_intel_crtc(crt

> > > c));

> > > @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct

> > > drm_device *dev,

> > >  		    (crtc->state->planes_changed ||

> > > update_pipe))

> > >  			drm_atomic_helper_commit_planes_on_crtc(

> > > crtc

> > > _state);

> > >  

> > > -		intel_post_plane_update(intel_crtc);

> > > +		if (pipe_config->base.active &&

> > > +		    (pipe_config->wm_changed || pipe_config-

> > > > disable_cxsr ||

> > > +		     pipe_config->fb_changed))

> > So the wm_changed is just for the BDW workaround + sprites? What

> > about

> > this disable_cxsr, why is it here? Also see my comment above about

> > sprite_changed. Maybe we need some comments here to explain the

> > complex

> > checks.

> No it's waiting for a vblank for post_plane_update so all optimized

> wm values

> can get written, it's not just for the BDW workaround.

> It just happens to be that the BDW w/a gets applied too as a side

> effect.


So maybe some comment regarding the BDW WA could be here.

What about disable_cxsr? Why is this here?

> > > +			crtc_vblank_mask |= 1 << i;

> > >  	}

> > >  

> > >  	/* FIXME: add subpixel order */

> > >  

> > > -	drm_atomic_helper_wait_for_vblanks(dev, state);

> > > +	if (!state->legacy_cursor_update)

> > > +		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));

> > > +

> > >  		if (put_domains[i])

> > >  			modeset_put_power_domains(dev_priv,

> > > put_domains[i]);

> > >  	}

> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h

> > > b/drivers/gpu/drm/i915/intel_drv.h

> > > index 335e6b24b0bc..e911c86f873b 100644

> > > --- a/drivers/gpu/drm/i915/intel_drv.h

> > > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > > @@ -379,6 +379,7 @@ struct intel_crtc_state {

> > >  	bool update_pipe; /* can a fast modeset be performed? */

> > >  	bool disable_cxsr;

> > >  	bool wm_changed; /* watermarks are updated */

> > > +	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,

> > > @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {

> > >  

> > >  	/* Sleepable operations to perform after commit */

> > >  	unsigned fb_bits;

> > > -	bool wait_vblank;

> > One of the things that I like about the code without this patch is

> > that

> > it's very explicit on when we need to wait for vblanks (except for

> > the

> > problem where we wait twice). The new code is not as easy to

> > read/understand as the old one. This comment is similar to the one

> > I

> > made in patch 6: I'm not sure if sacrificing readability is worth

> > it.

> > 

> > I wonder that maybe the cleanest fix to the "we're waiting 2

> > vblanks"

> > problem is to just remove the call to

> > drm_atomic_helper_wait_for_vblanks(), which would be a first patch.

> > Then we'd have a second patch introducing

> > intel_atomic_wait_for_vblanks() for the "wait for all vblanks at

> > the

> > same time" optimization, and then a last commit possibly replacing

> > commit->wait_vblank for state->fb_changed. Just an idea, not a

> > request.

> There were cases in which the atomic helper would wait for vblanks

> which

> wouldn't trigger any of the other changes, so it can't be blindly

> removed. Mostly when

> updating a plane with a same sized fb.


Those could be specifically addressed on their own patches, then :)

> Wait for vblank is required for unpinning, it would be bad if the

> current fb is unpinned.

> 

> > I'll wait for your answers before reaching any conclusions of what

> > I

> > think should be done, since I may be misunderstanding something.

> I want to call all post flip work scheduled later on so it happens

> after the next vblank.

> That will remove all confusion on when a vblank is required, because

> all post_plane_update

> and unpin will always wait until next vblank.

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
> Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
>> Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
>>> Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
>>>> Currently we perform our own wait in post_plane_update,
>>>> but the atomic core performs another one in wait_for_vblanks.
>>>> This means that 2 vblanks are done when a fb is changed,
>>>> which is a bit overkill.
>>>>
>>>> Merge them by creating a helper function that takes a crtc mask
>>>> for the planes to wait on.
>>>>
>>>> The broadwell vblank workaround may look gone entirely but this
>>>> is
>>>> not the case. pipe_config->wm_changed is set to true
>>>> when any plane is turned on, which forces a vblank wait.
>>>>
>>>> Changes since v1:
>>>> - Removing the double vblank wait on broadwell moved to its own
>>>> commit.
>>>> Changes since v2:
>>>> - Move out POWER_DOMAIN_MODESET handling to its own commit.
>>>> Changes since v3:
>>>> - Do not wait for vblank on legacy cursor updates. (Ville)
>>>> - Move broadwell vblank workaround comment to page_flip_finished.
>>>> (Ville)
>>>> Changes since v4:
>>>> - Compile fix, legacy_cursor_flip -> *_update.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
>>>> om>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>>>>  drivers/gpu/drm/i915/intel_display.c | 86
>>>> +++++++++++++++++++++++++++---------
>>>>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>>>  3 files changed, 67 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>> index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4785,9 +4785,6 @@ static void intel_post_plane_update(struct
>>>> intel_crtc *crtc)
>>>>  		to_intel_crtc_state(crtc->base.state);
>>>>  	struct drm_device *dev = crtc->base.dev;
>>>>  
>>>> -	if (atomic->wait_vblank)
>>>> -		intel_wait_for_vblank(dev, crtc->pipe);
>>>> -
>>>>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>>>>  
>>>>  	crtc->wm.cxsr_allowed = true;
>>>> @@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
>>>> intel_crtc *crtc)
>>>>  		return 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 :(
>>>> +	 */
>>> Having this comment here is just... weird. I think it removes a lot
>>> of
>>> the context that was present before.
>>>
>>>> +
>>>> +	/*
>>>>  	 * A DSPSURFLIVE check isn't enough in case the mmio and
>>>> CS
>>>> flips
>>>>  	 * used the same base address. In that case the mmio
>>>> flip
>>>> might
>>>>  	 * have completed, but the CS hasn't even executed the
>>>> flip
>>>> yet.
>>>> @@ -11778,6 +11781,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);
>>>>  
>>>> @@ -11793,8 +11799,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) {
>>>> -			if (is_crtc_enabled)
>>>> -				intel_crtc->atomic.wait_vblank =
>>>> true;
>>>>  			pipe_config->disable_cxsr = true;
>>>>  		}
>>> We could have killed the brackets here :)
>> Indeed, will do so in next version.
>>>>  	} else if (intel_wm_need_update(plane, plane_state)) {
>>>> @@ -11810,14 +11814,6 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		intel_crtc->atomic.post_enable_primary =
>>>> turn_on;
>>>>  		intel_crtc->atomic.update_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;
>>>> -
>>>>  		break;
>>>>  	case DRM_PLANE_TYPE_CURSOR:
>>>>  		break;
>>>> @@ -11831,12 +11827,10 @@ int
>>>> intel_plane_atomic_calc_changes(struct
>>>> drm_crtc_state *crtc_state,
>>>>  		if (IS_IVYBRIDGE(dev) &&
>>>>  		    needs_scaling(to_intel_plane_state(plane_sta
>>>> te))
>>>> &&
>>>>  		    !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;
>>>> -		}
>>> Weird brackets here. Either kill on both sides of the if statement
>>> (the
>>> preferred way), or none.
>>>
>>> Also, shouldn't we introduce pipe_config->sprite_changed? What's
>>> guaranteeing that we're going to definitely wait for a vblank
>>> during
>>> sprite updates? I like explicit dumb-proof code instead of
>>> implications
>>> such as "we're doing waits during sprite updates because whenever
>>> we
>>> update sprites, a specific unrelated variable that's used for a
>>> different purpose gets set to true, and we check for this
>>> variable".
>> sprite_changed would be same as fb_changed + wm_changed, and
>> update_sprite_watermarks gets removed in this series
>> because nothing uses it.
> Hmmm, right. For some reason, I was interpreting fb_changed as being
> only valid for the primary fb, not for spr/cur. My bad. So fb_changed
> means "if any of pri/cur/spr changed" I guess. Maybe planes_changed
> could be a better name, or fbs_changed (plural), since we're talking
> about more then one possible plane here. Not a requirement, just
> throwing the idea.
planes_changed is already used, it means that any plane_state is being updated for this crtc.
fbs_changed could work though, most other *_changed are plural.
>>>>  
>>>>  		break;
>>>>  	}
>>>> @@ -13370,6 +13364,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;
>>> Shouldn't we DRM_ERROR here?
>> WARN_ON is enough, this shouldn't ever happen.
> Even better :)
>
>>>> +		}
>>>> +
>>>> +		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(cr
>>>> tc),
>>>> +				msecs_to_jiffies(50));
>>> Also maybe DRM_ERROR in case we hit the timeout?
>>>
>>> I know the original code doesn't have this, but now that we're
>>> doing or
>>> own thing, we may scream in unexpected cases.
>> I'll make it a WARN_ON, shouldn't happen.
>>>> +
>>>> +		drm_crtc_vblank_put(crtc);
>>>> +	}
>>>> +}
>>>> +
>>>> +
>>> Two newlines :)
>> Indeed!
>>>>  /**
>>>>   * intel_atomic_commit - commit validated state object
>>>>   * @dev: DRM device
>>>> @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  	int ret = 0, i;
>>>>  	bool hw_check = intel_state->modeset;
>>>>  	unsigned long put_domains[I915_MAX_PIPES] = {};
>>>> +	unsigned crtc_vblank_mask = 0;
>>>>  
>>>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>>>  	if (ret) {
>>>> @@ -13470,8 +13507,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;
>>>>  
>>>>  		if (modeset && crtc->state->active) {
>>>>  			update_scanline_offset(to_intel_crtc(crt
>>>> c));
>>>> @@ -13488,14 +13526,20 @@ static int intel_atomic_commit(struct
>>>> drm_device *dev,
>>>>  		    (crtc->state->planes_changed ||
>>>> update_pipe))
>>>>  			drm_atomic_helper_commit_planes_on_crtc(
>>>> crtc
>>>> _state);
>>>>  
>>>> -		intel_post_plane_update(intel_crtc);
>>>> +		if (pipe_config->base.active &&
>>>> +		    (pipe_config->wm_changed || pipe_config-
>>>>> disable_cxsr ||
>>>> +		     pipe_config->fb_changed))
>>> So the wm_changed is just for the BDW workaround + sprites? What
>>> about
>>> this disable_cxsr, why is it here? Also see my comment above about
>>> sprite_changed. Maybe we need some comments here to explain the
>>> complex
>>> checks.
>> No it's waiting for a vblank for post_plane_update so all optimized
>> wm values
>> can get written, it's not just for the BDW workaround.
>> It just happens to be that the BDW w/a gets applied too as a side
>> effect.
> So maybe some comment regarding the BDW WA could be here.
>
> What about disable_cxsr? Why is this here?
That's what matches the current code, see the calc_changes hunk which had a vblank_wait = true.
>>>> +			crtc_vblank_mask |= 1 << i;
>>>>  	}
>>>>  
>>>>  	/* FIXME: add subpixel order */
>>>>  
>>>> -	drm_atomic_helper_wait_for_vblanks(dev, state);
>>>> +	if (!state->legacy_cursor_update)
>>>> +		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));
>>>> +
>>>>  		if (put_domains[i])
>>>>  			modeset_put_power_domains(dev_priv,
>>>> put_domains[i]);
>>>>  	}
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>>> b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 335e6b24b0bc..e911c86f873b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -379,6 +379,7 @@ struct intel_crtc_state {
>>>>  	bool update_pipe; /* can a fast modeset be performed? */
>>>>  	bool disable_cxsr;
>>>>  	bool wm_changed; /* watermarks are updated */
>>>> +	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,
>>>> @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
>>>>  
>>>>  	/* Sleepable operations to perform after commit */
>>>>  	unsigned fb_bits;
>>>> -	bool wait_vblank;
>>> One of the things that I like about the code without this patch is
>>> that
>>> it's very explicit on when we need to wait for vblanks (except for
>>> the
>>> problem where we wait twice). The new code is not as easy to
>>> read/understand as the old one. This comment is similar to the one
>>> I
>>> made in patch 6: I'm not sure if sacrificing readability is worth
>>> it.
>>>
>>> I wonder that maybe the cleanest fix to the "we're waiting 2
>>> vblanks"
>>> problem is to just remove the call to
>>> drm_atomic_helper_wait_for_vblanks(), which would be a first patch.
>>> Then we'd have a second patch introducing
>>> intel_atomic_wait_for_vblanks() for the "wait for all vblanks at
>>> the
>>> same time" optimization, and then a last commit possibly replacing
>>> commit->wait_vblank for state->fb_changed. Just an idea, not a
>>> request.
>> There were cases in which the atomic helper would wait for vblanks
>> which
>> wouldn't trigger any of the other changes, so it can't be blindly
>> removed. Mostly when
>> updating a plane with a same sized fb.
> Those could be specifically addressed on their own patches, then :)
It would break things though.

I think what might make the most sense is adding a inline function for needs_vblank,
with comments why certain flags require a vblank wait.
>> Wait for vblank is required for unpinning, it would be bad if the
>> current fb is unpinned.
>>
>>> I'll wait for your answers before reaching any conclusions of what
>>> I
>>> think should be done, since I may be misunderstanding something.
>> I want to call all post flip work scheduled later on so it happens
>> after the next vblank.
>> That will remove all confusion on when a vblank is required, because
>> all post_plane_update
>> and unpin will always wait until next vblank.
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qui, 2016-02-18 às 15:46 +0100, Maarten Lankhorst escreveu:
> Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:

> > Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:

> > > Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:

> > > > Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:

> > > > > Currently we perform our own wait in post_plane_update,

> > > > > but the atomic core performs another one in wait_for_vblanks.

> > > > > This means that 2 vblanks are done when a fb is changed,

> > > > > which is a bit overkill.

> > > > > 

> > > > > Merge them by creating a helper function that takes a crtc

> > > > > mask

> > > > > for the planes to wait on.

> > > > > 

> > > > > The broadwell vblank workaround may look gone entirely but

> > > > > this

> > > > > is

> > > > > not the case. pipe_config->wm_changed is set to true

> > > > > when any plane is turned on, which forces a vblank wait.

> > > > > 

> > > > > Changes since v1:

> > > > > - Removing the double vblank wait on broadwell moved to its

> > > > > own

> > > > > commit.

> > > > > Changes since v2:

> > > > > - Move out POWER_DOMAIN_MODESET handling to its own commit.

> > > > > Changes since v3:

> > > > > - Do not wait for vblank on legacy cursor updates. (Ville)

> > > > > - Move broadwell vblank workaround comment to

> > > > > page_flip_finished.

> > > > > (Ville)

> > > > > Changes since v4:

> > > > > - Compile fix, legacy_cursor_flip -> *_update.

> > > > > 

> > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.int

> > > > > el.c

> > > > > om>

> > > > > ---

> > > > >  drivers/gpu/drm/i915/intel_atomic.c  |  1 +

> > > > >  drivers/gpu/drm/i915/intel_display.c | 86

> > > > > +++++++++++++++++++++++++++---------

> > > > >  drivers/gpu/drm/i915/intel_drv.h     |  2 +-

> > > > >  3 files changed, 67 insertions(+), 22 deletions(-)

> > > > > 

> > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c

> > > > > b/drivers/gpu/drm/i915/intel_atomic.c

> > > > > index 4625f8a9ba12..8e579a8505ac 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->disable_cxsr = 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 804f2c6f260d..4d4dddc1f970 100644

> > > > > --- a/drivers/gpu/drm/i915/intel_display.c

> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c

> > > > > @@ -4785,9 +4785,6 @@ static void

> > > > > intel_post_plane_update(struct

> > > > > intel_crtc *crtc)

> > > > >  		to_intel_crtc_state(crtc->base.state);

> > > > >  	struct drm_device *dev = crtc->base.dev;

> > > > >  

> > > > > -	if (atomic->wait_vblank)

> > > > > -		intel_wait_for_vblank(dev, crtc->pipe);

> > > > > -

> > > > >  	intel_frontbuffer_flip(dev, atomic->fb_bits);

> > > > >  

> > > > >  	crtc->wm.cxsr_allowed = true;

> > > > > @@ -10902,6 +10899,12 @@ static bool

> > > > > page_flip_finished(struct

> > > > > intel_crtc *crtc)

> > > > >  		return 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 :(

> > > > > +	 */

> > > > Having this comment here is just... weird. I think it removes a

> > > > lot

> > > > of

> > > > the context that was present before.

> > > > 

> > > > > +

> > > > > +	/*

> > > > >  	 * A DSPSURFLIVE check isn't enough in case the mmio

> > > > > and

> > > > > CS

> > > > > flips

> > > > >  	 * used the same base address. In that case the mmio

> > > > > flip

> > > > > might

> > > > >  	 * have completed, but the CS hasn't even executed

> > > > > the

> > > > > flip

> > > > > yet.

> > > > > @@ -11778,6 +11781,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);

> > > > >  

> > > > > @@ -11793,8 +11799,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) {

> > > > > -			if (is_crtc_enabled)

> > > > > -				intel_crtc-

> > > > > >atomic.wait_vblank =

> > > > > true;

> > > > >  			pipe_config->disable_cxsr = true;

> > > > >  		}

> > > > We could have killed the brackets here :)

> > > Indeed, will do so in next version.

> > > > >  	} else if (intel_wm_need_update(plane, plane_state))

> > > > > {

> > > > > @@ -11810,14 +11814,6 @@ int

> > > > > intel_plane_atomic_calc_changes(struct

> > > > > drm_crtc_state *crtc_state,

> > > > >  		intel_crtc->atomic.post_enable_primary =

> > > > > turn_on;

> > > > >  		intel_crtc->atomic.update_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;

> > > > > -

> > > > >  		break;

> > > > >  	case DRM_PLANE_TYPE_CURSOR:

> > > > >  		break;

> > > > > @@ -11831,12 +11827,10 @@ int

> > > > > intel_plane_atomic_calc_changes(struct

> > > > > drm_crtc_state *crtc_state,

> > > > >  		if (IS_IVYBRIDGE(dev) &&

> > > > >  		    needs_scaling(to_intel_plane_state(plane

> > > > > _sta

> > > > > te))

> > > > > &&

> > > > >  		    !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;

> > > > > -		}

> > > > Weird brackets here. Either kill on both sides of the if

> > > > statement

> > > > (the

> > > > preferred way), or none.


I noticed that you fix the brackets on patch 4/8 by removing the "else"
part. So feel free to keep this chunk like this, so you won't need to
resend patch 4.


> > > > 

> > > > Also, shouldn't we introduce pipe_config->sprite_changed?

> > > > What's

> > > > guaranteeing that we're going to definitely wait for a vblank

> > > > during

> > > > sprite updates? I like explicit dumb-proof code instead of

> > > > implications

> > > > such as "we're doing waits during sprite updates because

> > > > whenever

> > > > we

> > > > update sprites, a specific unrelated variable that's used for a

> > > > different purpose gets set to true, and we check for this

> > > > variable".

> > > sprite_changed would be same as fb_changed + wm_changed, and

> > > update_sprite_watermarks gets removed in this series

> > > because nothing uses it.

> > Hmmm, right. For some reason, I was interpreting fb_changed as

> > being

> > only valid for the primary fb, not for spr/cur. My bad. So

> > fb_changed

> > means "if any of pri/cur/spr changed" I guess. Maybe planes_changed

> > could be a better name, or fbs_changed (plural), since we're

> > talking

> > about more then one possible plane here. Not a requirement, just

> > throwing the idea.

> planes_changed is already used, it means that any plane_state is

> being updated for this crtc.

> fbs_changed could work though, most other *_changed are plural.

> > > > >  

> > > > >  		break;

> > > > >  	}

> > > > > @@ -13370,6 +13364,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;

> > > > Shouldn't we DRM_ERROR here?

> > > WARN_ON is enough, this shouldn't ever happen.

> > Even better :)

> > 

> > > > > +		}

> > > > > +

> > > > > +		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_coun

> > > > > t(cr

> > > > > tc),

> > > > > +				msecs_to_jiffies(50));

> > > > Also maybe DRM_ERROR in case we hit the timeout?

> > > > 

> > > > I know the original code doesn't have this, but now that we're

> > > > doing or

> > > > own thing, we may scream in unexpected cases.

> > > I'll make it a WARN_ON, shouldn't happen.

> > > > > +

> > > > > +		drm_crtc_vblank_put(crtc);

> > > > > +	}

> > > > > +}

> > > > > +

> > > > > +

> > > > Two newlines :)

> > > Indeed!

> > > > >  /**

> > > > >   * intel_atomic_commit - commit validated state object

> > > > >   * @dev: DRM device

> > > > > @@ -13397,6 +13433,7 @@ static int intel_atomic_commit(struct

> > > > > drm_device *dev,

> > > > >  	int ret = 0, i;

> > > > >  	bool hw_check = intel_state->modeset;

> > > > >  	unsigned long put_domains[I915_MAX_PIPES] = {};

> > > > > +	unsigned crtc_vblank_mask = 0;

> > > > >  

> > > > >  	ret = intel_atomic_prepare_commit(dev, state,

> > > > > async);

> > > > >  	if (ret) {

> > > > > @@ -13470,8 +13507,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;

> > > > >  

> > > > >  		if (modeset && crtc->state->active) {

> > > > >  			update_scanline_offset(to_intel_crtc

> > > > > (crt

> > > > > c));

> > > > > @@ -13488,14 +13526,20 @@ static int

> > > > > intel_atomic_commit(struct

> > > > > drm_device *dev,

> > > > >  		    (crtc->state->planes_changed ||

> > > > > update_pipe))

> > > > >  			drm_atomic_helper_commit_planes_on_c

> > > > > rtc(

> > > > > crtc

> > > > > _state);

> > > > >  

> > > > > -		intel_post_plane_update(intel_crtc);

> > > > > +		if (pipe_config->base.active &&

> > > > > +		    (pipe_config->wm_changed || pipe_config-

> > > > > > disable_cxsr ||

> > > > > +		     pipe_config->fb_changed))

> > > > So the wm_changed is just for the BDW workaround + sprites?

> > > > What

> > > > about

> > > > this disable_cxsr, why is it here? Also see my comment above

> > > > about

> > > > sprite_changed. Maybe we need some comments here to explain the

> > > > complex

> > > > checks.

> > > No it's waiting for a vblank for post_plane_update so all

> > > optimized

> > > wm values

> > > can get written, it's not just for the BDW workaround.

> > > It just happens to be that the BDW w/a gets applied too as a side

> > > effect.

> > So maybe some comment regarding the BDW WA could be here.

> > 

> > What about disable_cxsr? Why is this here?

> That's what matches the current code, see the calc_changes hunk which

> had a vblank_wait = true.


Hmm, right. I missed that, sorry.

> > > > > +			crtc_vblank_mask |= 1 << i;

> > > > >  	}

> > > > >  

> > > > >  	/* FIXME: add subpixel order */

> > > > >  

> > > > > -	drm_atomic_helper_wait_for_vblanks(dev, state);

> > > > > +	if (!state->legacy_cursor_update)

> > > > > +		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))

> > > > > ;

> > > > > +

> > > > >  		if (put_domains[i])

> > > > >  			modeset_put_power_domains(dev_priv,

> > > > > put_domains[i]);

> > > > >  	}

> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h

> > > > > b/drivers/gpu/drm/i915/intel_drv.h

> > > > > index 335e6b24b0bc..e911c86f873b 100644

> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h

> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h

> > > > > @@ -379,6 +379,7 @@ struct intel_crtc_state {

> > > > >  	bool update_pipe; /* can a fast modeset be

> > > > > performed? */

> > > > >  	bool disable_cxsr;

> > > > >  	bool wm_changed; /* watermarks are updated */

> > > > > +	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,

> > > > > @@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {

> > > > >  

> > > > >  	/* Sleepable operations to perform after commit */

> > > > >  	unsigned fb_bits;

> > > > > -	bool wait_vblank;

> > > > One of the things that I like about the code without this patch

> > > > is

> > > > that

> > > > it's very explicit on when we need to wait for vblanks (except

> > > > for

> > > > the

> > > > problem where we wait twice). The new code is not as easy to

> > > > read/understand as the old one. This comment is similar to the

> > > > one

> > > > I

> > > > made in patch 6: I'm not sure if sacrificing readability is

> > > > worth

> > > > it.

> > > > 

> > > > I wonder that maybe the cleanest fix to the "we're waiting 2

> > > > vblanks"

> > > > problem is to just remove the call to

> > > > drm_atomic_helper_wait_for_vblanks(), which would be a first

> > > > patch.

> > > > Then we'd have a second patch introducing

> > > > intel_atomic_wait_for_vblanks() for the "wait for all vblanks

> > > > at

> > > > the

> > > > same time" optimization, and then a last commit possibly

> > > > replacing

> > > > commit->wait_vblank for state->fb_changed. Just an idea, not a

> > > > request.

> > > There were cases in which the atomic helper would wait for

> > > vblanks

> > > which

> > > wouldn't trigger any of the other changes, so it can't be blindly

> > > removed. Mostly when

> > > updating a plane with a same sized fb.

> > Those could be specifically addressed on their own patches, then :)

> It would break things though.


Ok, let's discard the idea then.

> 

> I think what might make the most sense is adding a inline function

> for needs_vblank,

> with comments why certain flags require a vblank wait.


That could be good.

I have no further questions/requests for this patch. In the end, it
seems the changes needed are small :). I'll wait for the next version.

> > > Wait for vblank is required for unpinning, it would be bad if the

> > > current fb is unpinned.

> > > 

> > > > I'll wait for your answers before reaching any conclusions of

> > > > what

> > > > I

> > > > think should be done, since I may be misunderstanding

> > > > something.

> > > I want to call all post flip work scheduled later on so it

> > > happens

> > > after the next vblank.

> > > That will remove all confusion on when a vblank is required,

> > > because

> > > all post_plane_update

> > > and unpin will always wait until next vblank.

> > > 

> > > _______________________________________________

> > > Intel-gfx mailing list

> > > Intel-gfx@lists.freedesktop.org

> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>