Kill off intel_crtc->atomic!

PATCH In review Maarten Lankhorst 3 participants 34 messages Feb 10, 2016 – Mar 23, 2016

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Maarten Lankhorst

author

2016-02-10 12:49 UTC #0 lore raw

v4 cover Kill off intel_crtc->atomic!

This is a rebased version of the original series. The upstream fbc changes required a rework of some of the patches, but with atomic encoder masks this series will work as intended.

v4 because v3 had a small issue with a compiler error in wait_vblank removal, and I forgot to send it to the ml.

Maarten Lankhorst (8):
drm/i915: Pass crtc state to modeset_get_crtc_power_domains. drm/i915: Unify power domain handling.
drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4. drm/i915: Remove update_sprite_watermarks.
drm/i915: Remove some post-commit members from intel_crtc->atomic, v2. drm/i915: Nuke fbc members from intel_crtc->atomic, v2. drm/i915: Do not compute watermarks on a noop.
drm/i915: Remove vblank wait from hsw_enable_ips.

 drivers/gpu/drm/i915/intel_atomic.c  |   2 +
 drivers/gpu/drm/i915/intel_display.c | 265 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h     |  36 ++---
 drivers/gpu/drm/i915/intel_pm.c      |  61 ++++----
 4 files changed, 196 insertions(+), 168 deletions(-)

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #1 lore raw

PATCH v4 1/8 drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

Use our newly created encoder_mask to iterate over the encoders.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/intel_display.c +21 ~~-12~~

index 30d4db0d776f..b479ba6238d7 100644
@@ -5301,31 +5301,37 @@ intel_display_port_aux_power_domain(struct intel_encoder *intel_encoder)
 	}
 }
 
-static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
+static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
+					    struct intel_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
+	struct drm_encoder *encoder;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned long mask;
-	enum transcoder transcoder = intel_crtc->config->cpu_transcoder;
+	enum transcoder transcoder = crtc_state->cpu_transcoder;
 
-	if (!crtc->state->active)
+	if (!crtc_state->base.active)
 		return 0;
 
 	mask = BIT(POWER_DOMAIN_PIPE(pipe));
 	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
-	if (intel_crtc->config->pch_pfit.enabled ||
-	    intel_crtc->config->pch_pfit.force_thru)
+	if (crtc_state->pch_pfit.enabled ||
+	    crtc_state->pch_pfit.force_thru)
 		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
 
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
+	drm_for_each_encoder_mask(encoder, dev, crtc_state->base.encoder_mask) {
+		struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
+
 		mask |= BIT(intel_display_port_power_domain(intel_encoder));
+	}
 
 	return mask;
 }
 
-static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
+static unsigned long
+modeset_get_crtc_power_domains(struct drm_crtc *crtc,
+			       struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

@@ -5333,7 +5339,8 @@ static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
 	unsigned long domains, new_domains, old_domains;
 
 	old_domains = intel_crtc->enabled_power_domains;
-	intel_crtc->enabled_power_domains = new_domains = get_crtc_power_domains(crtc);
+	intel_crtc->enabled_power_domains = new_domains =
+		get_crtc_power_domains(crtc, crtc_state);
 
 	domains = new_domains & ~old_domains;

@@ -5365,7 +5372,8 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (needs_modeset(crtc->state))
 			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc);
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc->state));
 	}
 
 	if (dev_priv->display.modeset_commit_cdclk &&

@@ -13486,7 +13494,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		}
 
 		if (update_pipe) {
-			put_domains = modeset_get_crtc_power_domains(crtc);
+			put_domains = modeset_get_crtc_power_domains(crtc,
+					      to_intel_crtc_state(crtc->state));
 
 			/* make sure intel_modeset_check_state runs */
 			hw_check = true;

@@ -15830,7 +15839,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 	for_each_intel_crtc(dev, crtc) {
 		unsigned long put_domains;
 
-		put_domains = modeset_get_crtc_power_domains(&crtc->base);
+		put_domains = modeset_get_crtc_power_domains(&crtc->base, crtc->config);
 		if (WARN_ON(put_domains))
 			modeset_put_power_domains(dev_priv, put_domains);
 	}

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

Reviewed-by on v4 1/8

2016-02-17 17:54 UTC #2 lore raw

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

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

Quoted 1 line

Use our newly created encoder_mask to iterate over the encoders.

As someone who was not paying attention to the discussion of the previous patches related to this, I think it would be really good if your commit message could tell me why we should use the newly created encoder_mask instead of the current patch. What's bad about the current version? Please sell me your patch. If you think the answer is trivial, remember that it's not trivial to many people, and that random people may find this patch through git-bisect and have to judge its importance. Also, an explanation really helps the reviewers :)

The patch looks correct, so if you improve the commit message:

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 114 lines

Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 33 +++++++++++++++++++++-----
-------
 1 file changed, 21 insertions(+), 12 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 30d4db0d776f..b479ba6238d7 100644
@@ -5301,31 +5301,37 @@ intel_display_port_aux_power_domain(struct
 intel_encoder *intel_encoder)
  	}
  }
drivers/gpu/drm/i915/intel_display.c +2 -1
-static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
+static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
+					    struct intel_crtc_state
*crtc_state)
{
struct drm_device *dev = crtc->dev;
drivers/gpu/drm/i915/intel_display.c +1 -1
-	struct intel_encoder *intel_encoder;
+	struct drm_encoder *encoder;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc); enum pipe pipe = intel_crtc->pipe; unsigned long mask;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	enum transcoder transcoder = intel_crtc->config-
Quoted 1 line
cpu_transcoder;
drivers/gpu/drm/i915/intel_display.c +1 -0
+	enum transcoder transcoder = crtc_state->cpu_transcoder;
drivers/gpu/drm/i915/intel_display.c +1 -1
-	if (!crtc->state->active)
+	if (!crtc_state->base.active)
return 0;

mask = BIT(POWER_DOMAIN_PIPE(pipe)); mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
drivers/gpu/drm/i915/intel_display.c +2 -2
-	if (intel_crtc->config->pch_pfit.enabled ||
-	    intel_crtc->config->pch_pfit.force_thru)
+	if (crtc_state->pch_pfit.enabled ||
+	    crtc_state->pch_pfit.force_thru)
mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
drivers/gpu/drm/i915/intel_display.c +1 -1
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
+	drm_for_each_encoder_mask(encoder, dev, crtc_state-
Quoted 1 line
base.encoder_mask) {
drivers/gpu/drm/i915/intel_display.c +1 -0
+		struct intel_encoder *intel_encoder =
to_intel_encoder(encoder);
+
mask |= BIT(intel_display_port_power_domain(intel_encoder));
drivers/gpu/drm/i915/intel_display.c +1 -0
+	}
return mask;
}

-static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
drivers/gpu/drm/i915/intel_display.c +3 -0
+static unsigned long
+modeset_get_crtc_power_domains(struct drm_crtc *crtc,
+			       struct intel_crtc_state *crtc_state)
{
struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -5333,7 +5339,8 @@ static unsigned long
 modeset_get_crtc_power_domains(struct drm_crtc *crtc)
  	unsigned long domains, new_domains, old_domains;
old_domains = intel_crtc->enabled_power_domains;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	intel_crtc->enabled_power_domains = new_domains =
get_crtc_power_domains(crtc);
drivers/gpu/drm/i915/intel_display.c +2 -0
+	intel_crtc->enabled_power_domains = new_domains =
+		get_crtc_power_domains(crtc, crtc_state);
domains = new_domains & ~old_domains;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -5365,7 +5372,8 @@ static void
 modeset_update_crtc_power_domains(struct drm_atomic_state *state)
  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
  		if (needs_modeset(crtc->state))
put_domains[to_intel_crtc(crtc)->pipe] =
drivers/gpu/drm/i915/intel_display.c +0 -1
-				modeset_get_crtc_power_domains(crtc)
;
drivers/gpu/drm/i915/intel_display.c +2 -0
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc-
Quoted 1 line
state));
}

if (dev_priv->display.modeset_commit_cdclk &&
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13486,7 +13494,8 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		}
if (update_pipe) {
drivers/gpu/drm/i915/intel_display.c +0 -1
-			put_domains =
modeset_get_crtc_power_domains(crtc);
drivers/gpu/drm/i915/intel_display.c +1 -0
+			put_domains =
modeset_get_crtc_power_domains(crtc,
drivers/gpu/drm/i915/intel_display.c +1 -0
+					      to_intel_crtc_state(cr
tc->state));

/* make sure intel_modeset_check_state runs */
hw_check = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -15830,7 +15839,7 @@ intel_modeset_setup_hw_state(struct
 drm_device *dev)
  	for_each_intel_crtc(dev, crtc) {
  		unsigned long put_domains;
drivers/gpu/drm/i915/intel_display.c +0 -1
-		put_domains = modeset_get_crtc_power_domains(&crtc-
Quoted 1 line
base);
drivers/gpu/drm/i915/intel_display.c +1 -0
+		put_domains = modeset_get_crtc_power_domains(&crtc-
Quoted 1 line
base, crtc->config);

Quoted 4 lines

if (WARN_ON(put_domains)) modeset_put_power_domains(dev_priv, put_domains);
}

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-18 09:51 UTC #3 lore raw

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:

Quoted 11 lines

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 1 line
Use our newly created encoder_mask to iterate over the encoders.
As someone who was not paying attention to the discussion of the previous patches related to this, I think it would be really good if your commit message could tell me why we should use the newly created encoder_mask instead of the current patch. What's bad about the current version? Please sell me your patch. If you think the answer is trivial, remember that it's not trivial to many people, and that random people may find this patch through git-bisect and have to judge its importance. Also, an explanation really helps the reviewers :)

Quoted 2 lines

The patch looks correct, so if you improve the commit message:
Reviewed-by Paulo Zanoni <[email protected]>

Does this sound good?

Use our newly created encoder_mask to iterate over the encoders. This makes it possible to get the crtc power domains from the crtc_state at any time, without any locks or having to look at the legacy state.

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-18 13:08 UTC #4 lore raw

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

Em Qui, 2016-02-18 às 10:51 +0100, Maarten Lankhorst escreveu:

Quoted 20 lines

Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:
Quoted 16 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 1 line
Use our newly created encoder_mask to iterate over the encoders.
As someone who was not paying attention to the discussion of the previous patches related to this, I think it would be really good if
your commit message could tell me why we should use the newly created
encoder_mask instead of the current patch. What's bad about the current
version? Please sell me your patch. If you think the answer is trivial,
remember that it's not trivial to many people, and that random people
may find this patch through git-bisect and have to judge its importance. Also, an explanation really helps the reviewers :)
Quoted 2 lines
The patch looks correct, so if you improve the commit message:
Reviewed-by Paulo Zanoni <[email protected]>

Quoted 6 lines

Does this sound good?
Use our newly created encoder_mask to iterate over the encoders. This makes it possible to get the crtc power domains from the crtc_state at any time, without any locks or having to look at the legacy state.

But we were already not grabbing any locks. Is this a bug fix? It would be good to point it if it's an actual fix.

Anyway, it's definitely better now :)

Quoted 5 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-18 13:21 UTC #5 lore raw

Re: [PATCH v4 1/8] drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

Op 18-02-16 om 14:08 schreef Zanoni, Paulo R:

Quoted 22 lines

Em Qui, 2016-02-18 às 10:51 +0100, Maarten Lankhorst escreveu:
Quoted 21 lines
Op 17-02-16 om 18:54 schreef Zanoni, Paulo R:
Quoted 16 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 1 line
Use our newly created encoder_mask to iterate over the encoders.
As someone who was not paying attention to the discussion of the previous patches related to this, I think it would be really good if
your commit message could tell me why we should use the newly created
encoder_mask instead of the current patch. What's bad about the current
version? Please sell me your patch. If you think the answer is trivial,
remember that it's not trivial to many people, and that random people
may find this patch through git-bisect and have to judge its importance. Also, an explanation really helps the reviewers :)
Quoted 2 lines
The patch looks correct, so if you improve the commit message:
Reviewed-by Paulo Zanoni <[email protected]>
Does this sound good?

Quoted 9 lines

Quoted 4 lines
Use our newly created encoder_mask to iterate over the encoders. This makes it possible to get the crtc power domains from the crtc_state at any time, without any locks or having to look at the legacy state.
But we were already not grabbing any locks. Is this a bug fix? It would be good to point it if it's an actual fix.
Anyway, it's definitely better now :)

But we're holding locks, crtc.mutex and mode_config.connection_mutex.

The changes allows us to remove the need of the locks, just a crtc_state is enough.
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #6 lore raw

PATCH v4 2/8 drm/i915: Unify power domain handling.

In reply to Maarten Lankhorst

Right now there's separate power domain handling for update_pipe and modesets. Unify this and only grab POWER_DOMAIN_MODESET once.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++-----------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

drivers/gpu/drm/i915/intel_display.c +24 ~~-45~~

index b479ba6238d7..804f2c6f260d 100644
@@ -5359,32 +5359,6 @@ static void modeset_put_power_domains(struct drm_i915_private *dev_priv,
 		intel_display_power_put(dev_priv, domain);
 }
 
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
-{
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long put_domains[I915_MAX_PIPES] = {};
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc *crtc;
-	int i;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state))
-			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc,
-					to_intel_crtc_state(crtc->state));
-	}
-
-	if (dev_priv->display.modeset_commit_cdclk &&
-	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
-		dev_priv->display.modeset_commit_cdclk(state);
-
-	for (i = 0; i < I915_MAX_PIPES; i++)
-		if (put_domains[i])
-			modeset_put_power_domains(dev_priv, put_domains[i]);
-}
-
 static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
 {
 	int max_cdclk_freq = dev_priv->max_cdclk_freq;

@@ -13422,6 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
+	unsigned long put_domains[I915_MAX_PIPES] = {};
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {

@@ -13437,11 +13412,22 @@ static int intel_atomic_commit(struct drm_device *dev,
 		       sizeof(intel_state->min_pixclk));
 		dev_priv->active_crtcs = intel_state->active_crtcs;
 		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
+
+		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
+		if (needs_modeset(crtc->state) ||
+		    to_intel_crtc_state(crtc->state)->update_pipe) {
+			hw_check = true;
+
+			put_domains[to_intel_crtc(crtc)->pipe] =
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc->state));
+		}
+
 		if (!needs_modeset(crtc->state))
 			continue;

@@ -13474,7 +13460,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 		intel_shared_dpll_commit(state);
 
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-		modeset_update_crtc_power_domains(state);
+
+		if (dev_priv->display.modeset_commit_cdclk &&
+		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
+			dev_priv->display.modeset_commit_cdclk(state);
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */

@@ -13483,24 +13472,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 		bool modeset = needs_modeset(crtc->state);
 		bool update_pipe = !modeset &&
 			to_intel_crtc_state(crtc->state)->update_pipe;
-		unsigned long put_domains = 0;
-
-		if (modeset)
-			intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
 
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
 		}
 
-		if (update_pipe) {
-			put_domains = modeset_get_crtc_power_domains(crtc,
-					      to_intel_crtc_state(crtc->state));
-
-			/* make sure intel_modeset_check_state runs */
-			hw_check = true;
-		}
-
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));

@@ -13511,19 +13488,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 (put_domains)
-			modeset_put_power_domains(dev_priv, put_domains);
-
 		intel_post_plane_update(intel_crtc);
-
-		if (modeset)
-			intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
 	}
 
 	/* FIXME: add subpixel order */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (put_domains[i])
+			modeset_put_power_domains(dev_priv, put_domains[i]);
+	}
+
+	if (intel_state->modeset)
+		intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
+
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

Reviewed-by on v4 2/8

2016-02-17 19:54 UTC #7 lore raw

Re: [PATCH v4 2/8] drm/i915: Unify power domain handling.

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

Quoted 2 lines

Right now there's separate power domain handling for update_pipe and modesets. Unify this and only grab POWER_DOMAIN_MODESET once.

I would have split this in two separate commits: one for POWER_DOMAIN_MODESET, and the other for killing intel_modeset_update_crtc_power_domains. Let's hope no one bisects anything to this patch :)

With or without the split:

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 163 lines

Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++-------------
----------
 1 file changed, 24 insertions(+), 45 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index b479ba6238d7..804f2c6f260d 100644
@@ -5359,32 +5359,6 @@ static void modeset_put_power_domains(struct
 drm_i915_private *dev_priv,
  		intel_display_power_put(dev_priv, domain);
  }
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
drivers/gpu/drm/i915/intel_display.c +0 -2
-{
-	struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
drivers/gpu/drm/i915/intel_display.c +0 ~~-12~~
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long put_domains[I915_MAX_PIPES] = {};
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc *crtc;
-	int i;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state))
-			put_domains[to_intel_crtc(crtc)->pipe] =
-				modeset_get_crtc_power_domains(crtc,
-					to_intel_crtc_state(crtc-
Quoted 1 line
state));
drivers/gpu/drm/i915/intel_display.c +0 -9
-	}
-
-	if (dev_priv->display.modeset_commit_cdclk &&
-	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
-		dev_priv->display.modeset_commit_cdclk(state);
-
-	for (i = 0; i < I915_MAX_PIPES; i++)
-		if (put_domains[i])
-			modeset_put_power_domains(dev_priv,
put_domains[i]);
drivers/gpu/drm/i915/intel_display.c +0 -2
-}
-
static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
{
int max_cdclk_freq = dev_priv->max_cdclk_freq;
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -13422,6 +13396,7 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  	struct drm_crtc *crtc;
  	int ret = 0, i;
  	bool hw_check = intel_state->modeset;
+	unsigned long put_domains[I915_MAX_PIPES] = {};
ret = intel_atomic_prepare_commit(dev, state, async); if (ret) {
drivers/gpu/drm/i915/intel_display.c +2 -0
@@ -13437,11 +13412,22 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		       sizeof(intel_state->min_pixclk));
  		dev_priv->active_crtcs = intel_state->active_crtcs;
  		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
+
+		intel_display_power_get(dev_priv,
 POWER_DOMAIN_MODESET);
  	}
for_each_crtc_in_state(state, crtc, crtc_state, i) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
drivers/gpu/drm/i915/intel_display.c +7 -0
+		if (needs_modeset(crtc->state) ||
+		    to_intel_crtc_state(crtc->state)->update_pipe) {
+			hw_check = true;
+
+			put_domains[to_intel_crtc(crtc)->pipe] =
+				modeset_get_crtc_power_domains(crtc,
+					to_intel_crtc_state(crtc-
Quoted 1 line
state));
drivers/gpu/drm/i915/intel_display.c +2 -0
+		}
+
if (!needs_modeset(crtc->state))
continue;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13474,7 +13460,10 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		intel_shared_dpll_commit(state);
drm_atomic_helper_update_legacy_modeset_state(state-
Quoted 1 line
dev, state);
drivers/gpu/drm/i915/intel_display.c +4 -1
-		modeset_update_crtc_power_domains(state);
+
+		if (dev_priv->display.modeset_commit_cdclk &&
+		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
+			dev_priv-
Quoted 1 line
display.modeset_commit_cdclk(state);
}

/* Now enable the clocks, plane, pipe, and connectors that we set up. */
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13483,24 +13472,12 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		bool modeset = needs_modeset(crtc->state);
bool update_pipe = !modeset && to_intel_crtc_state(crtc->state)-
Quoted 1 line
update_pipe;
drivers/gpu/drm/i915/intel_display.c +0 -4
-		unsigned long put_domains = 0;
-
-		if (modeset)
-			intel_display_power_get(dev_priv,
POWER_DOMAIN_MODESET);

if (modeset && crtc->state->active) { update_scanline_offset(to_intel_crtc(crtc)); dev_priv->display.crtc_enable(crtc); }
drivers/gpu/drm/i915/intel_display.c +0 -2
-		if (update_pipe) {
-			put_domains =
modeset_get_crtc_power_domains(crtc,
drivers/gpu/drm/i915/intel_display.c +0 -1
-					      to_intel_crtc_state(cr
tc->state));
drivers/gpu/drm/i915/intel_display.c +0 -2
-
-			/* make sure intel_modeset_check_state runs
*/
drivers/gpu/drm/i915/intel_display.c +0 -3
-			hw_check = true;
-		}
-
if (!modeset) intel_pre_plane_update(to_intel_crtc_state(c rtc_state));
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13511,19 +13488,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);
drivers/gpu/drm/i915/intel_display.c +0 -2
-		if (put_domains)
-			modeset_put_power_domains(dev_priv,
put_domains);
-
intel_post_plane_update(intel_crtc);
drivers/gpu/drm/i915/intel_display.c +0 -3
-
-		if (modeset)
-			intel_display_power_put(dev_priv,
POWER_DOMAIN_MODESET);
}

/* FIXME: add subpixel order */

drm_atomic_helper_wait_for_vblanks(dev, state);
drivers/gpu/drm/i915/intel_display.c +3 -0
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (put_domains[i])
+			modeset_put_power_domains(dev_priv,
put_domains[i]);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	if (intel_state->modeset)
+		intel_display_power_put(dev_priv,

Quoted 5 lines

POWER_DOMAIN_MODESET);
+
mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #8 lore raw

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

In reply to Maarten Lankhorst

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.

Signed-off-by Maarten Lankhorst <[email protected]>

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

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
 }

drivers/gpu/drm/i915/intel_display.c +65 ~~-21~~

154 lines not on this page · raw message

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 335e6b24b0bc..e911c86f873b 100644
@@ -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;

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-17 21:20 UTC #9 lore raw

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

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

Quoted 66 lines

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 <[email protected]>
---
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(-)
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
index 804f2c6f260d..4d4dddc1f970 100644
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
intel_frontbuffer_flip(dev, atomic->fb_bits);
crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
 intel_crtc *crtc)
  		return true;
/*

drivers/gpu/drm/i915/intel_display.c static bool page_flip_finished(struct intel_crtc *crtc) · line 10905

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +4 -0

+	 * 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.

Quoted 26 lines

drivers/gpu/drm/i915/intel_display.c +2 -0
+
+	/*
* 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.
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11778,6 +11781,9 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  	if (!was_visible && !visible)
  		return 0;
drivers/gpu/drm/i915/intel_display.c +3 -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);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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) {
drivers/gpu/drm/i915/intel_display.c +0 -2
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank =

Quoted 3 lines

true;
pipe_config->disable_cxsr = true; }

We could have killed the brackets here :)

Quoted 32 lines

} else if (intel_wm_need_update(plane, plane_state)) {
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -8
-		/*
-		 * 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;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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)) {
drivers/gpu/drm/i915/intel_display.c +0 -1
-			to_intel_crtc_state(crtc_state)-
Quoted 1 line
disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +2 -2
-		} 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;

drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11839

Quoted 1 line

drivers/gpu/drm/i915/intel_display.c +0 -1
-		}

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".

Quoted 23 lines

break;
}

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -13370,6 +13364,48 @@ static int
 intel_atomic_prepare_commit(struct drm_device *dev,
  	return ret;
  }

drivers/gpu/drm/i915/intel_display.c +2 -0

+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private

*dev_priv,

drivers/gpu/drm/i915/intel_display.c +11 -0

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

Quoted 1 line

pipe_to_crtc_mapping[pipe];

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13387

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c +8 -0

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

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c +3 -0
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13402

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c +8 -0

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

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13408

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c +6 -0
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+

Two newlines :)

Quoted 37 lines

/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -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) {

drivers/gpu/drm/i915/intel_display.c +3 -2

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

drivers/gpu/drm/i915/intel_display.c +0 -0

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

drivers/gpu/drm/i915/intel_display.c +2 -1

-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config-

Quoted 1 line

disable_cxsr ||

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13531

Quoted 1 line

drivers/gpu/drm/i915/intel_display.c +1 -0
+		     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.

Quoted 34 lines

drivers/gpu/drm/i915/intel_display.c +1 -0
+			crtc_vblank_mask |= 1 << i;
}

/* FIXME: add subpixel order */
drivers/gpu/drm/i915/intel_display.c +2 -1
-	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) {
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_post_plane_update(to_intel_crtc(crtc));
+
if (put_domains[i]) modeset_put_power_domains(dev_priv, put_domains[i]);
}
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 335e6b24b0bc..e911c86f873b 100644
@@ -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 */
drivers/gpu/drm/i915/intel_drv.h +1 -0
+	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,
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
/* Sleepable operations to perform after commit */ unsigned fb_bits;

drivers/gpu/drm/i915/intel_drv.h struct intel_crtc_atomic_commit { · line 550

Quoted 1 line

drivers/gpu/drm/i915/intel_drv.h +0 -1
-	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.

Quoted 3 lines

bool post_enable_primary;
unsigned update_sprite_watermarks;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-18 13:22 UTC #10 lore raw

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

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

Quoted 96 lines

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 62 lines
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 <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
 }
drivers/gpu/drm/i915/intel_display.c +0 -3
index 804f2c6f260d..4d4dddc1f970 100644
@@ -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);
 
drivers/gpu/drm/i915/intel_display.c +0 -0
 	crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c static bool page_flip_finished(struct · line 10906
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +4 -0
@@ -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.
Quoted 22 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+
+	/*
 	 * 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.
drivers/gpu/drm/i915/intel_display.c +3 -0
@@ -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,
 

Quoted 8 lines

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.c +0 -2

 		/* 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.

Quoted 36 lines

Quoted 31 lines

drivers/gpu/drm/i915/intel_display.c +0 -8

 	} 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:

drivers/gpu/drm/i915/intel_display.c +0 -0

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

drivers/gpu/drm/i915/intel_display.c +0 -1

 		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)-

Quoted 1 line

disable_lp_wm = true;

drivers/gpu/drm/i915/intel_display.c +2 -2

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

drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11839

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +0 -1
 				1 << i;
-		}

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

Quoted 6 lines

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.

Quoted 23 lines

drivers/gpu/drm/i915/intel_display.c +13 -0

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

Quoted 9 lines

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13387
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +8 -0
+
+		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.

Quoted 19 lines

Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13402
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +8 -0
+
+		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?

Quoted 2 lines

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.

Quoted 7 lines

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13408
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6 -0
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
Two newlines :)

Indeed!

Quoted 36 lines

/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device

drivers/gpu/drm/i915/intel_display.c +1 -0

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

drivers/gpu/drm/i915/intel_display.c +3 -2

 	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;

drivers/gpu/drm/i915/intel_display.c +2 -1

 		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-

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13531
Quoted 2 lines
Quoted 1 line
disable_cxsr ||
drivers/gpu/drm/i915/intel_display.c +1 -0
+		     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.

Quoted 41 lines

Quoted 31 lines
drivers/gpu/drm/i915/intel_display.c +3 -1
+			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);
drivers/gpu/drm/i915/intel_display.c +2 -0
 	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]);
drivers/gpu/drm/i915/intel_display.c +0 -0
 	}
drivers/gpu/drm/i915/intel_drv.h +1 -0
index 335e6b24b0bc..e911c86f873b 100644
@@ -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,
drivers/gpu/drm/i915/intel_drv.h struct intel_crtc_atomic_commit { · line 550
Quoted 4 lines
drivers/gpu/drm/i915/intel_drv.h +0 -1
@@ -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.

Quoted 7 lines

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.

Quoted 2 lines

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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-18 14:14 UTC #11 lore raw

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

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

Quoted 170 lines

Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
Quoted 106 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 69 lines
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 <[email protected]
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(-)
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
index 804f2c6f260d..4d4dddc1f970 100644
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
intel_frontbuffer_flip(dev, atomic->fb_bits);
crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -10902,6 +10899,12 @@ static bool page_flip_finished(struct
 intel_crtc *crtc)
  		return true;
/*
drivers/gpu/drm/i915/intel_display.c static bool page_flip_finished(struct intel_crtc *crtc) · line 10905
Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +4 -0
+	 * 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.
Quoted 28 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+
+	/*
* 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.
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11778,6 +11781,9 @@ int
intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,
if (!was_visible && !visible)
return 0;
drivers/gpu/drm/i915/intel_display.c +3 -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);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11793,8 +11799,6 @@ int
intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,

/* must disable cxsr around plane enable/disable
Quoted 8 lines
Quoted 7 lines
*/
if (plane->type != DRM_PLANE_TYPE_CURSOR) {
drivers/gpu/drm/i915/intel_display.c +0 -2
-			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.
Quoted 44 lines
Quoted 39 lines
} else if (intel_wm_need_update(plane, plane_state)) {
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		/*
-		 * BDW signals flip done immediately if the
plane
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * is disabled, even if the plane enable is
already
drivers/gpu/drm/i915/intel_display.c +0 -5
-		 * 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;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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)) {
drivers/gpu/drm/i915/intel_display.c +0 -1
-			to_intel_crtc_state(crtc_state)-
Quoted 1 line
disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +2 -2
-		} 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-
Quoted 2 lines
atomic.update_sprite_watermarks
=
1 << i;
drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11839
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +0 -1
-		}
Weird brackets here. Either kill on both sides of the if statement (the
preferred way), or none.
Quoted 10 lines
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".

Quoted 3 lines

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.

Quoted 34 lines

Quoted 25 lines
Quoted 25 lines
break;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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,
drivers/gpu/drm/i915/intel_display.c +1 -0
+					  struct
drm_i915_private
*dev_priv,
drivers/gpu/drm/i915/intel_display.c +11 -0
+					  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-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13387
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +8 -0
+
+		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?

Quoted 1 line

WARN_ON is enough, this shouldn't ever happen.

Even better :)

Quoted 80 lines

Quoted 20 lines
Quoted 17 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c +7 -0
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(cr
tc),
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13402
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +1 -0
+				msecs_to_jiffies(50));
Also maybe DRM_ERROR in case we hit the timeout?
Quoted 3 lines
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.
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13408
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6 -0
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
Two newlines :)
Indeed!
Quoted 40 lines
Quoted 40 lines
/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -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) {
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +0 -2
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)-
Quoted 1 line
update_pipe;
drivers/gpu/drm/i915/intel_display.c +3 -0
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config-
Quoted 1 line
update_pipe;

if (modeset && crtc->state->active) { update_scanline_offset(to_intel_crtc(crt c));
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +2 -1
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config-
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13531
Quoted 2 lines
Quoted 1 line
disable_cxsr ||
drivers/gpu/drm/i915/intel_display.c +1 -0
+		     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.

Quoted 5 lines

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?

Quoted 56 lines

Quoted 46 lines
Quoted 35 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
+			crtc_vblank_mask |= 1 << i;
}

/* FIXME: add subpixel order */
drivers/gpu/drm/i915/intel_display.c +2 -1
-	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) {
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_post_plane_update(to_intel_crtc(crtc));
+
if (put_domains[i]) modeset_put_power_domains(dev_priv, put_domains[i]);
}
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 335e6b24b0bc..e911c86f873b 100644
@@ -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 */
drivers/gpu/drm/i915/intel_drv.h +1 -0
+	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,
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
/* Sleepable operations to perform after commit */ unsigned fb_bits;
drivers/gpu/drm/i915/intel_drv.h struct intel_crtc_atomic_commit { · line 550
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0 -1
-	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.
Quoted 10 lines
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.

Quoted 5 lines

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

Quoted 12 lines

Wait for vblank is required for unpinning, it would be bad if the current fb is unpinned.
Quoted 3 lines
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.

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-18 14:46 UTC #12 lore raw

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

Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:

Quoted 174 lines

Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
Quoted 170 lines
Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
Quoted 105 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 65 lines
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 <[email protected]
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(-)
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
}
drivers/gpu/drm/i915/intel_display.c +0 -3
index 804f2c6f260d..4d4dddc1f970 100644
@@ -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);
 
drivers/gpu/drm/i915/intel_display.c +0 -0
 	crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c static bool page_flip_finished(struct · line 10906
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +4 -0
@@ -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.
Quoted 27 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+
+	/*
 	 * 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.
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11778,6 +11781,9 @@ int
intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,
drivers/gpu/drm/i915/intel_display.c +3 -0
 	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,
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11803
Quoted 8 lines
/* must disable cxsr around plane enable/disable
*/
drivers/gpu/drm/i915/intel_display.c +0 -2
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank =
true;
drivers/gpu/drm/i915/intel_display.c +0 -0
 			pipe_config->disable_cxsr = true;
 		}
We could have killed the brackets here :)
Indeed, will do so in next version.
Quoted 44 lines
Quoted 38 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 	} 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;
drivers/gpu/drm/i915/intel_display.c +0 -2
 		intel_crtc->atomic.update_fbc = true;
 
-		/*
-		 * BDW signals flip done immediately if the
plane
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * is disabled, even if the plane enable is
already
drivers/gpu/drm/i915/intel_display.c +0 -5
-		 * 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,
drivers/gpu/drm/i915/intel_display.c +0 -0
 		if (IS_IVYBRIDGE(dev) &&
 		    needs_scaling(to_intel_plane_state(plane_sta
te))
&&
drivers/gpu/drm/i915/intel_display.c +0 -1
 		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)-
Quoted 1 line
disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +2 -2
-		} 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-
Quoted 2 lines
atomic.update_sprite_watermarks
=
drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11839
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +0 -1
 				1 << i;
-		}
Weird brackets here. Either kill on both sides of the if statement (the
preferred way), or none.
Quoted 10 lines
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".
Quoted 3 lines
sprite_changed would be same as fb_changed + wm_changed, and update_sprite_watermarks gets removed in this series because nothing uses it.

Quoted 6 lines

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.

Quoted 117 lines

Quoted 34 lines
Quoted 25 lines
Quoted 25 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
 		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,
drivers/gpu/drm/i915/intel_display.c +11 -0
+					  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-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13387
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +8 -0
+
+		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?
Quoted 1 line
WARN_ON is enough, this shouldn't ever happen.
Even better :)
Quoted 80 lines
Quoted 20 lines
Quoted 17 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c +7 -0
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(cr
tc),
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13402
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +1 -0
+				msecs_to_jiffies(50));
Also maybe DRM_ERROR in case we hit the timeout?
Quoted 3 lines
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.
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13408
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6 -0
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
Two newlines :)
Indeed!
Quoted 48 lines
Quoted 41 lines
/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +3 -2
 	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-
Quoted 1 line
update_pipe;
drivers/gpu/drm/i915/intel_display.c +0 -0
 		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crt
c));
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +2 -1
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config-
Quoted 1 line
disable_cxsr ||
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13531
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +1 -0
+		     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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 56 lines

Quoted 56 lines
Quoted 56 lines
Quoted 32 lines
drivers/gpu/drm/i915/intel_display.c +3 -1
+			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);
drivers/gpu/drm/i915/intel_display.c +2 -0
 	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]);
drivers/gpu/drm/i915/intel_display.c +0 -0
 	}
drivers/gpu/drm/i915/intel_drv.h +1 -0
index 335e6b24b0bc..e911c86f873b 100644
@@ -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)
drivers/gpu/drm/i915/intel_drv.h +0 -0
 	 * All planes will be positioned inside this space,
drivers/gpu/drm/i915/intel_drv.h struct intel_crtc_atomic_commit { · line 550
Quoted 4 lines
drivers/gpu/drm/i915/intel_drv.h +0 -1
@@ -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.

Quoted 6 lines

Quoted 5 lines
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.

Quoted 12 lines

Quoted 12 lines
Wait for vblank is required for unpinning, it would be bad if the current fb is unpinned.
Quoted 3 lines
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.

Quoted 4 lines

Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-18 17:03 UTC #13 lore raw

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

Em Qui, 2016-02-18 às 15:46 +0100, Maarten Lankhorst escreveu:

Quoted 179 lines

Op 18-02-16 om 15:14 schreef Zanoni, Paulo R:
Quoted 178 lines
Em Qui, 2016-02-18 às 14:22 +0100, Maarten Lankhorst escreveu:
Quoted 177 lines
Op 17-02-16 om 22:20 schreef Zanoni, Paulo R:
Quoted 122 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 76 lines
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 <[email protected]
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(-)
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
index 804f2c6f260d..4d4dddc1f970 100644
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
intel_frontbuffer_flip(dev, atomic->fb_bits);
crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -10902,6 +10899,12 @@ static bool
page_flip_finished(struct
intel_crtc *crtc)
return true;

/*
drivers/gpu/drm/i915/intel_display.c static bool page_flip_finished(struct intel_crtc *crtc) · line 10905
Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +4 -0
+	 * 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.
Quoted 36 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+
+	/*
* 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.
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11778,6 +11781,9 @@ int
intel_plane_atomic_calc_changes(struct
drm_crtc_state *crtc_state,
if (!was_visible && !visible)
return 0;
drivers/gpu/drm/i915/intel_display.c +3 -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);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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) {
drivers/gpu/drm/i915/intel_display.c +0 -2
-			if (is_crtc_enabled)
-				intel_crtc-
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
atomic.wait_vblank =
true;
pipe_config->disable_cxsr = true; }
We could have killed the brackets here :)
Indeed, will do so in next version.
Quoted 48 lines
Quoted 43 lines
} else if (intel_wm_need_update(plane, plane_state)) {
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		/*
-		 * BDW signals flip done immediately if the
plane
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * is disabled, even if the plane enable is
already
drivers/gpu/drm/i915/intel_display.c +0 -4
-		 * 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;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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)) {
drivers/gpu/drm/i915/intel_display.c +0 -1
-			to_intel_crtc_state(crtc_state)-
Quoted 1 line
disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank =
true;
drivers/gpu/drm/i915/intel_display.c +2 -0
+			pipe_config->disable_lp_wm = true;
+		} else if (turn_off && !mode_changed)
intel_crtc-
Quoted 2 lines
atomic.update_sprite_watermarks
=
1 << i;
drivers/gpu/drm/i915/intel_display.c int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, · line 11839
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +0 -1
-		}
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.

Quoted 159 lines

Quoted 16 lines
Quoted 13 lines
Quoted 13 lines
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".
Quoted 3 lines
sprite_changed would be same as fb_changed + wm_changed, and update_sprite_watermarks gets removed in this series because nothing uses it.
Quoted 9 lines
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.
Quoted 131 lines
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
break;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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,
drivers/gpu/drm/i915/intel_display.c +1 -0
+					  struct
drm_i915_private
*dev_priv,
drivers/gpu/drm/i915/intel_display.c +1 -0
+					  unsigned
crtc_mask)
drivers/gpu/drm/i915/intel_display.c +10 -0
+{
+	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-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13387
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +8 -0
+
+		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?
Quoted 1 line
WARN_ON is enough, this shouldn't ever happen.
Even better :)
Quoted 93 lines
Quoted 21 lines
Quoted 18 lines
drivers/gpu/drm/i915/intel_display.c +3 -0
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c +7 -0
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_coun
t(cr
tc),
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13402
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +1 -0
+				msecs_to_jiffies(50));
Also maybe DRM_ERROR in case we hit the timeout?
Quoted 3 lines
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.
Quoted 7 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_prepare_commit(struct drm_device *dev, · line 13408
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c +6 -0
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+
Two newlines :)
Indeed!
Quoted 45 lines
Quoted 45 lines
/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -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) {
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +0 -2
-		bool update_pipe = !modeset &&
-			to_intel_crtc_state(crtc->state)-
Quoted 1 line
update_pipe;
drivers/gpu/drm/i915/intel_display.c +3 -0
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc->state);
+		bool update_pipe = !modeset && pipe_config-
Quoted 1 line
update_pipe;

if (modeset && crtc->state->active) { update_scanline_offset(to_intel_crtc (crt
c));
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -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);
drivers/gpu/drm/i915/intel_display.c +2 -1
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
+		    (pipe_config->wm_changed || pipe_config-
Quoted 1 line
disable_cxsr ||
Quoted 9 lines
drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13531
Quoted 1 line
drivers/gpu/drm/i915/intel_display.c +1 -0
+		     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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 66 lines

Quoted 66 lines
Quoted 66 lines
Quoted 66 lines
Quoted 38 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
+			crtc_vblank_mask |= 1 << i;
}

/* FIXME: add subpixel order */
drivers/gpu/drm/i915/intel_display.c +2 -1
-	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) {
drivers/gpu/drm/i915/intel_display.c +1 -0
+		intel_post_plane_update(to_intel_crtc(crtc))
;
+
if (put_domains[i]) modeset_put_power_domains(dev_priv, put_domains[i]);
}
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 335e6b24b0bc..e911c86f873b 100644
@@ -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 */
drivers/gpu/drm/i915/intel_drv.h +1 -0
+	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,
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
/* Sleepable operations to perform after commit */ unsigned fb_bits;
drivers/gpu/drm/i915/intel_drv.h struct intel_crtc_atomic_commit { · line 550
Quoted 1 line
drivers/gpu/drm/i915/intel_drv.h +0 -1
-	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.

Quoted 8 lines

Quoted 7 lines
Quoted 6 lines
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.

Quoted 4 lines

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.

Quoted 16 lines

Quoted 16 lines
Quoted 16 lines
Wait for vblank is required for unpinning, it would be bad if the current fb is unpinned.
Quoted 5 lines
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.

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-24 13:46 UTC #14 lore raw

PATCH v6 3/8 drm/i915: Kill off intel_crtc->atomic.wait_vblank, v6.

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.
Changes since v5:
- Kill brackets.
- Add WARN_ON when wait_for_vblanks fails.
- Remove extra newlines.
- Split the checks whether vblank is needed to a separate function, with comments why a vblank is needed.

Signed-off-by Maarten Lankhorst <[email protected]>

---
v5 was skipped, previous version was v5 because it had the compile fix.

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
 }

drivers/gpu/drm/i915/intel_display.c +88 ~~-24~~

181 lines not on this page · raw message

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 285b0570be9c..74df4f818e03 100644
@@ -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;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

Reviewed-by on v6 3/8

2016-02-24 16:34 UTC #15 lore raw

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

Em Qua, 2016-02-24 às 11:24 +0100, Maarten Lankhorst escreveu:

Quoted 29 lines

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.
Changes since v5:
- Kill brackets.
- Add WARN_ON when wait_for_vblanks fails.
- Remove extra newlines.
- Split the checks whether vblank is needed to a separate function, with comments why a vblank is needed.

Quoted 1 line

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 258 lines

---
v5 was skipped, previous version was v5 because it had the compile fix.
drivers/gpu/drm/i915/intel_atomic.c +1 -0
index 4625f8a9ba12..8e579a8505ac 100644
@@ -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;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
index 2107e324cd9e..9f32cb0bf978 100644
@@ -4792,9 +4792,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;
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->wait_vblank)
-		intel_wait_for_vblank(dev, crtc->pipe);
-
intel_frontbuffer_flip(dev, atomic->fb_bits);
crtc->wm.cxsr_allowed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -10957,6 +10954,12 @@ static bool page_flip_finished(struct
 intel_crtc *crtc)
  		return true;
/*
drivers/gpu/drm/i915/intel_display.c +6 -0
+	 * 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.
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11833,6 +11836,9 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  	if (!was_visible && !visible)
  		return 0;
drivers/gpu/drm/i915/intel_display.c +3 -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);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11847,11 +11853,8 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  		pipe_config->wm_changed = true;
/* must disable cxsr around plane enable/disable */
drivers/gpu/drm/i915/intel_display.c +0 -3
-		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
-			if (is_crtc_enabled)
-				intel_crtc->atomic.wait_vblank =
true;
drivers/gpu/drm/i915/intel_display.c +1 -0
+		if (plane->type != DRM_PLANE_TYPE_CURSOR)
pipe_config->disable_cxsr = true;
drivers/gpu/drm/i915/intel_display.c +0 -1
-		}
} else if (intel_wm_need_update(plane, plane_state)) { pipe_config->wm_changed = true;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11865,14 +11868,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;
drivers/gpu/drm/i915/intel_display.c +0 -8
-		/*
-		 * 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;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11885,13 +11880,11 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  		 */
if (IS_IVYBRIDGE(dev) && needs_scaling(to_intel_plane_state(plane_state)) &&
drivers/gpu/drm/i915/intel_display.c +0 -2
-		    !needs_scaling(old_plane_state)) {
-			to_intel_crtc_state(crtc_state)-
Quoted 1 line
disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +3 -2
-		} else if (turn_off && !mode_changed) {
-			intel_crtc->atomic.wait_vblank = true;
+		    !needs_scaling(old_plane_state))
+			pipe_config->disable_lp_wm = true;
+		else if (turn_off && !mode_changed)
intel_crtc->atomic.update_sprite_watermarks |=
1 << i;
drivers/gpu/drm/i915/intel_display.c +0 -1
-		}
break;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13425,6 +13418,71 @@ static int
 intel_atomic_prepare_commit(struct drm_device *dev,
  	return ret;
  }
drivers/gpu/drm/i915/intel_display.c +2 -0
+static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
+					  struct drm_i915_private
*dev_priv,
drivers/gpu/drm/i915/intel_display.c +11 -0
+					  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-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c +11 -0
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		ret = drm_crtc_vblank_get(crtc);
+		if (WARN_ON(ret != 0)) {
+			crtc_mask &= ~(1 << pipe);
+			continue;
+		}
+
+		last_vblank_count[pipe] =
drm_crtc_vblank_count(crtc);
drivers/gpu/drm/i915/intel_display.c +4 -0
+	}
+
+	for_each_pipe(dev_priv, pipe) {
+		struct drm_crtc *crtc = dev_priv-
Quoted 1 line
pipe_to_crtc_mapping[pipe];
drivers/gpu/drm/i915/intel_display.c +37 -0
+		long lret;
+
+		if (!((1 << pipe) & crtc_mask))
+			continue;
+
+		lret = wait_event_timeout(dev->vblank[pipe].queue,
+				last_vblank_count[pipe] !=
+					drm_crtc_vblank_count(crtc),
+				msecs_to_jiffies(50));
+
+		WARN_ON(!lret);
+
+		drm_crtc_vblank_put(crtc);
+	}
+}
+
+static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
+{
+	/* fb updated, need to unpin old fb */
+	if (crtc_state->fb_changed)
+		return true;
+
+	/* wm changes, need vblank before final wm's */
+	if (crtc_state->wm_changed)
+		return true;
+
+	/*
+	 * cxsr is re-enabled after vblank.
+	 * This is already handled by crtc_state->wm_changed,
+	 * but added for clarity.
+	 */
+	if (crtc_state->disable_cxsr)
+		return true;
+
+	return false;
+}
+
/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device
drivers/gpu/drm/i915/intel_display.c +1 -0
@@ -13452,6 +13510,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) {
drivers/gpu/drm/i915/intel_display.c +3 -2
@@ -13525,8 +13584,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));
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13543,14 +13603,18 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		    (crtc->state->planes_changed || update_pipe))
drm_atomic_helper_commit_planes_on_crtc(crtc _state);
drivers/gpu/drm/i915/intel_display.c +1 -1
-		intel_post_plane_update(intel_crtc);
+		if (pipe_config->base.active &&
needs_vblank_wait(pipe_config))
drivers/gpu/drm/i915/intel_display.c +1 -0
+			crtc_vblank_mask |= 1 << i;
}

/* FIXME: add subpixel order */
drivers/gpu/drm/i915/intel_display.c +2 -1
-	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) {
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_post_plane_update(to_intel_crtc(crtc));
+
if (put_domains[i]) modeset_put_power_domains(dev_priv, put_domains[i]);
}
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 285b0570be9c..74df4f818e03 100644
@@ -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 */
drivers/gpu/drm/i915/intel_drv.h +1 -0
+	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,
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -547,7 +548,6 @@ struct intel_crtc_atomic_commit {
/* Sleepable operations to perform after commit */ unsigned fb_bits;
drivers/gpu/drm/i915/intel_drv.h +0 -1
-	bool wait_vblank;

Quoted 4 lines

bool post_enable_primary;
unsigned update_sprite_watermarks;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #16 lore raw

PATCH v4 4/8 drm/i915: Remove update_sprite_watermarks.

In reply to Maarten Lankhorst

Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks") removes the use of this variable, but forgot to remove it.

Reviewed-by Matt Roper <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 6 +-----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_display.c +1 -5

index 4d4dddc1f970..c1f94eee8028 100644
@@ -11748,7 +11748,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
-	int i = drm_plane_index(plane);
 	bool mode_changed = needs_modeset(crtc_state);
 	bool was_crtc_enabled = crtc->state->active;
 	bool is_crtc_enabled = crtc_state->active;

@@ -11826,11 +11825,8 @@ 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)) {
+		    !needs_scaling(old_plane_state))
 			pipe_config->disable_lp_wm = true;
-		} else if (turn_off && !mode_changed)
-			intel_crtc->atomic.update_sprite_watermarks |=
-				1 << i;
 
 		break;
 	}

drivers/gpu/drm/i915/intel_drv.h +0 -1

index e911c86f873b..d4e441f3aecf 100644
@@ -549,7 +549,6 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool post_enable_primary;
-	unsigned update_sprite_watermarks;
 
 	/* Sleepable operations to perform before and after commit */
 	bool update_fbc;

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #17 lore raw

PATCH v4 5/8 drm/i915: Remove some post-commit members from intel_crtc->atomic, v2.

In reply to Maarten Lankhorst

fb_bits is useful to have in the crtc_state for cs flips when the code is updated to use intel_frontbuffer_flip_prepare/complete. So calculate it in advance and move it to crtc_state. The other stuff can be calculated in post_plane_update, and aren't useful elsewhere.

Changes since v1:
- Changing wording, remove comment about loop.

Signed-off-by Maarten Lankhorst <[email protected]>

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

---
 drivers/gpu/drm/i915/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +----
 3 files changed, 23 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +1 -0

index 8e579a8505ac..9a45cff26767 100644
@@ -98,6 +98,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
 	crtc_state->fb_changed = false;
+	crtc_state->fb_bits = 0;
 
 	return &crtc_state->base;
 }

drivers/gpu/drm/i915/intel_display.c +21 -8

index c1f94eee8028..54be8a255f1f 100644
@@ -4778,14 +4778,20 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
-static void intel_post_plane_update(struct intel_crtc *crtc)
+static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
+	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
+	struct drm_atomic_state *old_state = old_crtc_state->base.state;
 	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *old_pri_state =
+		drm_atomic_get_existing_plane_state(old_state, primary);
 
-	intel_frontbuffer_flip(dev, atomic->fb_bits);
+	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
 	crtc->wm.cxsr_allowed = true;

@@ -4795,8 +4801,17 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 	if (atomic->update_fbc)
 		intel_fbc_post_update(crtc);
 
-	if (atomic->post_enable_primary)
-		intel_post_enable_primary(&crtc->base);
+	if (old_pri_state) {
+		struct intel_plane_state *primary_state =
+			to_intel_plane_state(primary->state);
+		struct intel_plane_state *old_primary_state =
+			to_intel_plane_state(old_pri_state);
+
+		if (primary_state->visible &&
+		    (needs_modeset(&pipe_config->base) ||
+		     !old_primary_state->visible))
+			intel_post_enable_primary(&crtc->base);
+	}
 
 	memset(atomic, 0, sizeof(*atomic));
 }

@@ -11805,12 +11820,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	}
 
 	if (visible || was_visible)
-		intel_crtc->atomic.fb_bits |=
-			to_intel_plane(plane)->frontbuffer_bit;
+		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.post_enable_primary = turn_on;
 		intel_crtc->atomic.update_fbc = true;
 
 		break;

@@ -13534,7 +13547,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		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));
+		intel_post_plane_update(to_intel_crtc_state(crtc_state));
 
 		if (put_domains[i])
 			modeset_put_power_domains(dev_priv, put_domains[i]);

drivers/gpu/drm/i915/intel_drv.h +1 -4

index d4e441f3aecf..803591d1e613 100644
@@ -376,6 +376,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 	unsigned long quirks;
 
+	unsigned fb_bits; /* framebuffers to flip */
 	bool update_pipe; /* can a fast modeset be performed? */
 	bool disable_cxsr;
 	bool wm_changed; /* watermarks are updated */

@@ -547,10 +548,6 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 
 	/* Sleepable operations to perform after commit */
-	unsigned fb_bits;
-	bool post_enable_primary;
-
-	/* Sleepable operations to perform before and after commit */
 	bool update_fbc;
 };

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #18 lore raw

PATCH v4 6/8 drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

In reply to Maarten Lankhorst

Factor out intel_fbc_supports_rotation and use it in pre_plane_update as well. This leaves intel_crtc->atomic empty, so remove it too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
 2 files changed, 20 insertions(+), 53 deletions(-)

drivers/gpu/drm/i915/intel_display.c +20 ~~-38~~

index 54be8a255f1f..00cb261c6787 100644
@@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);

@@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)

@@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;

@@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible);
+
+		intel_fbc_pre_update(crtc);
 
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
 			intel_pre_disable_primary(&crtc->base);
 	}

@@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }

@@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;

@@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
-		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+		if ((modeset || update_pipe) && crtc->state->active)
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&
-		    (crtc->state->planes_changed || update_pipe))
+		    (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->disable_cxsr ||

drivers/gpu/drm/i915/intel_drv.h +0 ~~-15~~

index 803591d1e613..8effb9ece21e 100644
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;

@@ -610,8 +597,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

Reviewed-by on v4 6/8

2016-02-12 13:56 UTC #19 lore raw

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

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

Quoted 1 line

Factor out intel_fbc_supports_rotation

^ not anymore

Quoted 6 lines

and use it in
pre_plane_update as well. This leaves intel_crtc->atomic empty, so remove it too.
Changes since v1:
- Add a intel_fbc_supports_rotation helper.

Changes since v2:
- No more need for rotation special-casing.

(I suppose you also have to edit the commit title to be v3)

Quoted 37 lines

Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
----------
drivers/gpu/drm/i915/intel_drv.h | 15 ----------
 2 files changed, 20 insertions(+), 53 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 54be8a255f1f..00cb261c6787 100644
@@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
  {
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_plane *primary = crtc->base.primary; struct drm_plane_state *old_pri_state = drm_atomic_get_existing_plane_state(old_state, primary);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
  	if (pipe_config->wm_changed && pipe_config->base.active)
  		intel_update_watermarks(&crtc->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-

Quoted 1 line

if (old_pri_state) {

For a code reader that is not very familiar with all the atomic history and its details, it's not trivial to conclude that "if (drm_atomic_get_existing_plane_state(primary))", then we're updating the primary plane on this atomic commit. And before this patch, it's much much easier to conclude that update_fbc will be true when an atomic update is touching the primary plane because that's explicitly stated by the cod.

So although "let's kill this redundant struct" sounds good, it seems to me that code clarity is going away with this patch, so I wonder if the benefits of the patch outweigh the downsides. Isn't there something else we could do about this, such as some renaming, or adding some function aliases or just extra commenting?

Quoted 43 lines

struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_fbc_post_update(crtc);
+
if (primary_state->visible && (needs_modeset(&pipe_config->base) || !old_primary_state->visible)) intel_post_enable_primary(&crtc->base); }
drivers/gpu/drm/i915/intel_display.c +0 -2
-
-	memset(atomic, 0, sizeof(*atomic));
}

static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
  		drm_atomic_get_existing_plane_state(old_state,
 primary);
  	bool modeset = needs_modeset(&pipe_config->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
if (old_pri_state) {
struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);

drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) · line 4833

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0

+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible);

Not really related to the patch, but ok to keep since it's trivial...

Quoted 74 lines

drivers/gpu/drm/i915/intel_display.c +2 -0
+
+		intel_fbc_pre_update(crtc);
drivers/gpu/drm/i915/intel_display.c +1 -2
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
intel_pre_disable_primary(&crtc->base); }
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  	if (visible || was_visible)
pipe_config->fb_bits |= to_intel_plane(plane)-
Quoted 1 line
frontbuffer_bit;
drivers/gpu/drm/i915/intel_display.c +0 ~~-12~~
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this
flag will
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * take effect when we commit and program
watermarks.
drivers/gpu/drm/i915/intel_display.c +0 -3
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state))
&&
drivers/gpu/drm/i915/intel_display.c +4 -2
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag
will
drivers/gpu/drm/i915/intel_display.c +3 -0
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
IS_IVYBRIDGE(dev) &&
drivers/gpu/drm/i915/intel_display.c +3 -0
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		break;
-	}
return 0;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
 drm_device *dev,
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
drivers/gpu/drm/i915/intel_display.c +0 -3
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
/* Catch I915_MODE_FLAG_INHERITED */
if (crtc_state->mode.private_flags != crtc->state-
Quoted 1 line
mode.private_flags)
crtc_state->mode_changed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		if (!modeset)
intel_pre_plane_update(to_intel_crtc_state(c rtc_state));
drivers/gpu/drm/i915/intel_display.c +0 -1
-		if (crtc->state->active && intel_crtc-
Quoted 1 line
atomic.update_fbc)

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13512

Quoted 1 line

drivers/gpu/drm/i915/intel_display.c +1 -0
+		if ((modeset || update_pipe) && crtc->state->active)

Same here: not easy for me to verify things are the same now. Even
worse: the check for "is this atomic commit touching the primary plane?" is now written in a completely different way than the one introduced above. Maybe there's something we could do to make the code easier to read.

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct drm_device *dev, · line 13518

Quoted 8 lines

intel_fbc_enable(intel_crtc);
if (crtc->state->active &&
drivers/gpu/drm/i915/intel_display.c +1 -1
-		    (crtc->state->planes_changed || update_pipe))
+		    (crtc->state->planes_changed || update_pipe)) {
drm_atomic_helper_commit_planes_on_crtc(crtc _state);
drivers/gpu/drm/i915/intel_display.c +1 -0
+		}

???

Besides the points mentioned above, the patch seems to pass the FBC tests, which is good :)

My suggestions are sorta bikesheds and although I'm not 100% confident, the patch seems to be correct, so:

Reviewed-by Paulo Zanoni <[email protected]>

But please consider the readability concerns pointed on this review, maybe by proposing v4 or a follow-up patch :)

Quoted 38 lines

if (pipe_config->base.active && (pipe_config->wm_changed || pipe_config-
Quoted 1 line
disable_cxsr ||
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 803591d1e613..8effb9ece21e 100644
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
  	unsigned int rotation;
  };
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
drivers/gpu/drm/i915/intel_drv.h +0 -7
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
struct intel_crtc {
struct drm_crtc base;
enum pipe pipe;
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -610,8 +597,6 @@ struct intel_crtc {
  		int scanline_start;
  	} debug;
drivers/gpu/drm/i915/intel_drv.h +0 -2
-	struct intel_crtc_atomic_commit atomic;
-

Quoted 3 lines

/* scalers available on this crtc */
int num_scalers;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-15 14:31 UTC #20 lore raw

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:

Quoted 186 lines

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 1 line
Factor out intel_fbc_supports_rotation
^ not anymore
Quoted 6 lines
and use it in
pre_plane_update as well. This leaves intel_crtc->atomic empty, so remove it too.
Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes since v2:
- No more need for rotation special-casing.
(I suppose you also have to edit the commit title to be v3)
Quoted 29 lines
Signed-off-by Maarten Lankhorst <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++-------------
----------
 drivers/gpu/drm/i915/intel_drv.h     | 15 ----------
 2 files changed, 20 insertions(+), 53 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -2
index 54be8a255f1f..00cb261c6787 100644
@@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
 > base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state-
 > base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/intel_display.c +0 -0
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state,
primary);
drivers/gpu/drm/i915/intel_display.c static void intel_post_plane_update(struct · line 4800
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +0 -3
@@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	if (old_pri_state) {
For a code reader that is not very familiar with all the atomic history and its details, it's not trivial to conclude that "if (drm_atomic_get_existing_plane_state(primary))", then we're updating the primary plane on this atomic commit. And before this patch, it's much much easier to conclude that update_fbc will be true when an atomic update is touching the primary plane because that's explicitly stated by the cod.
So although "let's kill this redundant struct" sounds good, it seems to me that code clarity is going away with this patch, so I wonder if the benefits of the patch outweigh the downsides. Isn't there something else we could do about this, such as some renaming, or adding some function aliases or just extra commenting?
Quoted 35 lines
drivers/gpu/drm/i915/intel_display.c +2 -2
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
static void intel_pre_plane_update(struct intel_crtc_state
*old_crtc_state)
drivers/gpu/drm/i915/intel_display.c +0 -1
@@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
 > base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
drivers/gpu/drm/i915/intel_display.c +0 -0
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state,
 primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct · line 4835
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +2 -3
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible); 
Not really related to the patch, but ok to keep since it's trivial...
Quoted 72 lines
drivers/gpu/drm/i915/intel_display.c +3 -2
+
+		intel_fbc_pre_update(crtc);
 
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
 			intel_pre_disable_primary(&crtc->base);
drivers/gpu/drm/i915/intel_display.c +0 ~~-12~~
 	}
@@ -11822,27 +11816,17 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)-
 > frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this
flag will
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * take effect when we commit and program
watermarks.
drivers/gpu/drm/i915/intel_display.c +0 -3
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state))
&&
drivers/gpu/drm/i915/intel_display.c +4 -2
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag
will
drivers/gpu/drm/i915/intel_display.c +3 -0
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
IS_IVYBRIDGE(dev) &&
drivers/gpu/drm/i915/intel_display.c +3 -5
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }
@@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
 drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state-
Quoted 1 line
mode.private_flags)
drivers/gpu/drm/i915/intel_display.c +0 -0
 			crtc_state->mode_changed = true;
@@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
 drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(c
 rtc_state));
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_display.c static int intel_atomic_commit(struct · line 13515
Quoted 3 lines
drivers/gpu/drm/i915/intel_display.c +1 -1
-		if (crtc->state->active && intel_crtc-
 > atomic.update_fbc)
+		if ((modeset || update_pipe) && crtc->state->active)
Same here: not easy for me to verify things are the same now. Even
worse: the check for "is this atomic commit touching the primary plane?" is now written in a completely different way than the one introduced above. Maybe there's something we could do to make the code easier to read.

No, this is called now every time when the crtc has a modeset or update_pipe, even with no primary plane. This is because atomic may set a mode without having a primary plane, and in that case fbc_enable may never be called. Seemed like a bug in the original code..

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-18 17:17 UTC #21 lore raw

Re: [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.

Em Seg, 2016-02-15 às 15:31 +0100, Maarten Lankhorst escreveu:

Quoted 212 lines

Op 12-02-16 om 14:56 schreef Zanoni, Paulo R:
Quoted 203 lines
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 1 line
Factor out intel_fbc_supports_rotation
^ not anymore
Quoted 6 lines
and use it in
pre_plane_update as well. This leaves intel_crtc->atomic empty, so remove it too.
Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes since v2:
- No more need for rotation special-casing.
(I suppose you also have to edit the commit title to be v3)
Quoted 39 lines
Signed-off-by Maarten Lankhorst <[email protected]
om>
---
drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++---------
----
----------
drivers/gpu/drm/i915/intel_drv.h | 15 ----------
 2 files changed, 20 insertions(+), 53 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 54be8a255f1f..00cb261c6787 100644
@@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
  {
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_plane *primary = crtc->base.primary; struct drm_plane_state *old_pri_state = drm_atomic_get_existing_plane_state(old_state, primary);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4798,22 +4796,19 @@ static void
intel_post_plane_update(struct
intel_crtc_state *old_crtc_state)
if (pipe_config->wm_changed && pipe_config->base.active) intel_update_watermarks(&crtc->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
Quoted 1 line
if (old_pri_state) {
For a code reader that is not very familiar with all the atomic history
and its details, it's not trivial to conclude that "if (drm_atomic_get_existing_plane_state(primary))", then we're updating
the primary plane on this atomic commit. And before this patch, it's
much much easier to conclude that update_fbc will be true when an atomic update is touching the primary plane because that's explicitly
stated by the cod.
So although "let's kill this redundant struct" sounds good, it seems to
me that code clarity is going away with this patch, so I wonder if the
benefits of the patch outweigh the downsides. Isn't there something else we could do about this, such as some renaming, or adding some function aliases or just extra commenting?
Quoted 43 lines
struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_fbc_post_update(crtc);
+
if (primary_state->visible && (needs_modeset(&pipe_config->base) || !old_primary_state->visible)) intel_post_enable_primary(&crtc->base); }
drivers/gpu/drm/i915/intel_display.c +0 -2
-
-	memset(atomic, 0, sizeof(*atomic));
}

static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
  		drm_atomic_get_existing_plane_state(old_state,
 primary);
  	bool modeset = needs_modeset(&pipe_config->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
if (old_pri_state) {
struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);
drivers/gpu/drm/i915/intel_display.c static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) · line 4833
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+		bool turn_off = old_primary_state->visible &&
+		    (modeset || !primary_state->visible); 
Not really related to the patch, but ok to keep since it's trivial...
Quoted 79 lines
drivers/gpu/drm/i915/intel_display.c +2 -0
+
+		intel_fbc_pre_update(crtc);
drivers/gpu/drm/i915/intel_display.c +1 -2
-		if (old_primary_state->visible &&
-		    (modeset || !primary_state->visible))
+		if (turn_off)
intel_pre_disable_primary(&crtc->base); }
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11822,27 +11816,17 @@ int
intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
if (visible || was_visible) pipe_config->fb_bits |= to_intel_plane(plane)-
Quoted 1 line
frontbuffer_bit;
drivers/gpu/drm/i915/intel_display.c +0 ~~-12~~
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so
this
flag will
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * take effect when we commit and program
watermarks.
drivers/gpu/drm/i915/intel_display.c +0 -3
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_sta
te))
&&
drivers/gpu/drm/i915/intel_display.c +4 -2
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag
will
drivers/gpu/drm/i915/intel_display.c +3 -0
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
IS_IVYBRIDGE(dev) &&
drivers/gpu/drm/i915/intel_display.c +3 -0
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		break;
-	}
return 0;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13241,9 +13225,6 @@ static int intel_atomic_check(struct
 drm_device *dev,
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
drivers/gpu/drm/i915/intel_display.c +0 -3
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
/* Catch I915_MODE_FLAG_INHERITED */ if (crtc_state->mode.private_flags != crtc-
Quoted 2 lines
state-
mode.private_flags)
crtc_state->mode_changed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13528,12 +13509,13 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		if (!modeset)
intel_pre_plane_update(to_intel_crtc_sta te(c
rtc_state));
drivers/gpu/drm/i915/intel_display.c +0 -1
-		if (crtc->state->active && intel_crtc-
Quoted 1 line
atomic.update_fbc)
Quoted 8 lines
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +1 -0
+		if ((modeset || update_pipe) && crtc->state-
Quoted 1 line
active)
Same here: not easy for me to verify things are the same now. Even
worse: the check for "is this atomic commit touching the primary plane?" is now written in a completely different way than the one introduced above. Maybe there's something we could do to make the code
easier to read.

Quoted 5 lines

No, this is called now every time when the crtc has a modeset or update_pipe, even with no primary plane.
This is because atomic may set a mode without having a primary plane, and in that case fbc_enable may never be called.
Seemed like a bug in the original code..

I suppose that proves my point that this code with all those similar- but-different boolean checks is really hard to read :)

FBC can only happen when there's a primary plane, so AFAIU even if we don't call intel_fbc_enable here because the primary is disable, we'll end up calling it later when we enable the primary plane.

Anyway, a change in behavior like this (which was supposed to be a bug fix) should be on its own separate patch, not hidden inside a refactor.

Quoted 6 lines

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-29 09:58 UTC #22 lore raw

PATCH v4 6/8 drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

Whenever there's an update to the primary plane, fbc_pre_update and fbc_post_update are called. Kill off intel_crtc->atomic.update_fbc and now that intel_crtc->atomic is empty, kill it off too.

Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes sinec v2:
- Remove intel_fbc_supports_rotation_helper.
- Remove unrelated changes.

Signed-off-by Maarten Lankhorst <[email protected]>

---
This version doesn't change intel_fbc_enable any more!

drivers/gpu/drm/i915/intel_display.c +16 ~~-35~~

index 78f72bef25aa..c8404092aee9 100644
@@ -4789,11 +4789,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);

@@ -4805,22 +4803,19 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	if (pipe_config->wm_changed && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_post_update(crtc);
+
 		if (primary_state->visible &&
 		    (needs_modeset(&pipe_config->base) ||
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
-
-	memset(atomic, 0, sizeof(*atomic));
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)

@@ -4828,7 +4823,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;

@@ -4837,15 +4831,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		drm_atomic_get_existing_plane_state(old_state, primary);
 	bool modeset = needs_modeset(&pipe_config->base);
 
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
 	if (old_pri_state) {
 		struct intel_plane_state *primary_state =
 			to_intel_plane_state(primary->state);
 		struct intel_plane_state *old_primary_state =
 			to_intel_plane_state(old_pri_state);
 
+		intel_fbc_pre_update(crtc);
+
 		if (old_primary_state->visible &&
 		    (modeset || !primary_state->visible))
 			intel_pre_disable_primary(&crtc->base);

@@ -11877,27 +11870,17 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (visible || was_visible)
 		pipe_config->fb_bits |= to_intel_plane(plane)->frontbuffer_bit;
 
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this flag will
-		 * take effect when we commit and program watermarks.
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state)) &&
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag will
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY && IS_IVYBRIDGE(dev) &&
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
 
-		break;
-	}
 	return 0;
 }

@@ -13296,9 +13279,6 @@ static int intel_atomic_check(struct drm_device *dev,
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
 
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
 		/* Catch I915_MODE_FLAG_INHERITED */
 		if (crtc_state->mode.private_flags != crtc->state->mode.private_flags)
 			crtc_state->mode_changed = true;

@@ -13606,7 +13586,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 		if (!modeset)
 			intel_pre_plane_update(to_intel_crtc_state(crtc_state));
 
-		if (crtc->state->active && intel_crtc->atomic.update_fbc)
+		if (crtc->state->active &&
+		    drm_atomic_get_existing_plane_state(state, crtc->primary))
 			intel_fbc_enable(intel_crtc);
 
 		if (crtc->state->active &&

drivers/gpu/drm/i915/intel_drv.h +0 ~~-15~~

index 058bb10e53ae..5e5b64fff30d 100644
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
 	unsigned int rotation;
 };
 
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;

@@ -610,8 +597,6 @@ struct intel_crtc {
 		int scanline_start;
 	} debug;
 
-	struct intel_crtc_atomic_commit atomic;
-
 	/* scalers available on this crtc */
 	int num_scalers;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-29 21:06 UTC #23 lore raw

Re: [PATCH v4.1 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:

Quoted 11 lines

Whenever there's an update to the primary plane, fbc_pre_update and fbc_post_update are called. Kill off intel_crtc->atomic.update_fbc and now that intel_crtc->atomic is empty, kill it off too.
Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes sinec v2:
- Remove intel_fbc_supports_rotation_helper.
- Remove unrelated changes.

Quoted 3 lines

Signed-off-by Maarten Lankhorst <[email protected]>
---
This version doesn't change intel_fbc_enable any more!

Ok, so I decided to test this.

First of all, patch 05 needs a rebase too. I did my own local rebase and I suppose I got things right.

Second, after I did actual testing, I discovered that, contrary to my previous belief, "old_pri_state" will also be valid when we're only changing the sprite or cursor planes. So if we apply this patch, we'll be calling pre_update+enable+post_update for every plane/cursor enable/disable operation, which is not what we want since these are unnecessary calls, as FBC only deals with the primary plane. I think we need to fix this.

The tests I've been using to check this were:
- kms_frontbuffer_tracking/fbc-1p-primscrn-cur-indfb-onoff
- kms_frontbuffer_tracking/fbc-1p-primscrn-spr-indfb-onoff

Quoted 189 lines

drivers/gpu/drm/i915/intel_display.c +0 -0
index 78f72bef25aa..c8404092aee9 100644
@@ -4789,11 +4789,9 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
  {
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_plane *primary = crtc->base.primary; struct drm_plane_state *old_pri_state = drm_atomic_get_existing_plane_state(old_state, primary);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4805,22 +4803,19 @@ static void intel_post_plane_update(struct
 intel_crtc_state *old_crtc_state)
  	if (pipe_config->wm_changed && pipe_config->base.active)
  		intel_update_watermarks(&crtc->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_post_update(crtc);
-
if (old_pri_state) {
struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_fbc_post_update(crtc);
+
if (primary_state->visible && (needs_modeset(&pipe_config->base) || !old_primary_state->visible)) intel_post_enable_primary(&crtc->base); }
drivers/gpu/drm/i915/intel_display.c +0 -2
-
-	memset(atomic, 0, sizeof(*atomic));
}

static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4828,7 +4823,6 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
Quoted 1 line
base.crtc);
struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_atomic_state *old_state = old_crtc_state-
Quoted 1 line
base.state;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -4837,15 +4831,14 @@ static void intel_pre_plane_update(struct
 intel_crtc_state *old_crtc_state)
  		drm_atomic_get_existing_plane_state(old_state,
 primary);
  	bool modeset = needs_modeset(&pipe_config->base);
drivers/gpu/drm/i915/intel_display.c +0 -3
-	if (atomic->update_fbc)
-		intel_fbc_pre_update(crtc);
-
if (old_pri_state) {
struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state);
drivers/gpu/drm/i915/intel_display.c +2 -0
+		intel_fbc_pre_update(crtc);
+
if (old_primary_state->visible && (modeset || !primary_state->visible)) intel_pre_disable_primary(&crtc->base);
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -11877,27 +11870,17 @@ int intel_plane_atomic_calc_changes(struct
 drm_crtc_state *crtc_state,
  	if (visible || was_visible)
pipe_config->fb_bits |= to_intel_plane(plane)-
Quoted 1 line
frontbuffer_bit;
drivers/gpu/drm/i915/intel_display.c +0 ~~-12~~
-	switch (plane->type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		intel_crtc->atomic.update_fbc = true;
-
-		break;
-	case DRM_PLANE_TYPE_CURSOR:
-		break;
-	case DRM_PLANE_TYPE_OVERLAY:
-		/*
-		 * WaCxSRDisabledForSpriteScaling:ivb
-		 *
-		 * cstate->update_wm was already set above, so this
flag will
drivers/gpu/drm/i915/intel_display.c +0 -1
-		 * take effect when we commit and program
watermarks.
drivers/gpu/drm/i915/intel_display.c +0 -3
-		 */
-		if (IS_IVYBRIDGE(dev) &&
-		    needs_scaling(to_intel_plane_state(plane_state))
&&
drivers/gpu/drm/i915/intel_display.c +4 -2
-		    !needs_scaling(old_plane_state))
-			pipe_config->disable_lp_wm = true;
+	/*
+	 * WaCxSRDisabledForSpriteScaling:ivb
+	 *
+	 * cstate->update_wm was already set above, so this flag
will
drivers/gpu/drm/i915/intel_display.c +3 -0
+	 * take effect when we commit and program watermarks.
+	 */
+	if (plane->type == DRM_PLANE_TYPE_OVERLAY &&
IS_IVYBRIDGE(dev) &&
drivers/gpu/drm/i915/intel_display.c +3 -0
+	    needs_scaling(to_intel_plane_state(plane_state)) &&
+	    !needs_scaling(old_plane_state))
+		pipe_config->disable_lp_wm = true;
drivers/gpu/drm/i915/intel_display.c +0 -2
-		break;
-	}
return 0;
}
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13296,9 +13279,6 @@ static int intel_atomic_check(struct
 drm_device *dev,
struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
drivers/gpu/drm/i915/intel_display.c +0 -3
-		memset(&to_intel_crtc(crtc)->atomic, 0,
-		       sizeof(struct intel_crtc_atomic_commit));
-
/* Catch I915_MODE_FLAG_INHERITED */
if (crtc_state->mode.private_flags != crtc->state-
Quoted 1 line
mode.private_flags)
crtc_state->mode_changed = true;
drivers/gpu/drm/i915/intel_display.c +0 -0
@@ -13606,7 +13586,8 @@ static int intel_atomic_commit(struct
 drm_device *dev,
  		if (!modeset)
intel_pre_plane_update(to_intel_crtc_state(c rtc_state));
drivers/gpu/drm/i915/intel_display.c +0 -1
-		if (crtc->state->active && intel_crtc-
Quoted 1 line
atomic.update_fbc)
drivers/gpu/drm/i915/intel_display.c +2 -0
+		if (crtc->state->active &&
+		    drm_atomic_get_existing_plane_state(state, crtc-
Quoted 1 line
primary))
intel_fbc_enable(intel_crtc);
if (crtc->state->active &&
drivers/gpu/drm/i915/intel_drv.h +0 -0
index 058bb10e53ae..5e5b64fff30d 100644
@@ -538,19 +538,6 @@ struct intel_mmio_flip {
  	unsigned int rotation;
  };
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
drivers/gpu/drm/i915/intel_drv.h +0 -7
-struct intel_crtc_atomic_commit {
-	/* Sleepable operations to perform before commit */
-
-	/* Sleepable operations to perform after commit */
-	bool update_fbc;
-};
-
struct intel_crtc {
struct drm_crtc base;
enum pipe pipe;
drivers/gpu/drm/i915/intel_drv.h +0 -0
@@ -610,8 +597,6 @@ struct intel_crtc {
  		int scanline_start;
  	} debug;
drivers/gpu/drm/i915/intel_drv.h +0 -2
-	struct intel_crtc_atomic_commit atomic;
-
/* scalers available on this crtc */
int num_scalers;

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-03-01 08:27 UTC #24 lore raw

Re: [PATCH v4.1 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v3.

Op 29-02-16 om 22:06 schreef Zanoni, Paulo R:

Quoted 20 lines

Em Seg, 2016-02-29 às 10:58 +0100, Maarten Lankhorst escreveu:
Quoted 11 lines
Whenever there's an update to the primary plane, fbc_pre_update and fbc_post_update are called. Kill off intel_crtc->atomic.update_fbc and now that intel_crtc->atomic is empty, kill it off too.
Changes since v1:
- Add a intel_fbc_supports_rotation helper.
Changes sinec v2:
- Remove intel_fbc_supports_rotation_helper.
- Remove unrelated changes.
Quoted 3 lines
Signed-off-by Maarten Lankhorst <[email protected]>
---
This version doesn't change intel_fbc_enable any more!
Ok, so I decided to test this.
First of all, patch 05 needs a rebase too. I did my own local rebase and I suppose I got things right.

Quoted 7 lines

Second, after I did actual testing, I discovered that, contrary to my previous belief, "old_pri_state" will also be valid when we're only changing the sprite or cursor planes. So if we apply this patch, we'll be calling pre_update+enable+post_update for every plane/cursor enable/disable operation, which is not what we want since these are unnecessary calls, as FBC only deals with the primary plane. I think we need to fix this.

This is a bug addressed by the next patch.

drm/i915: Do not compute watermarks on a noop.

I'll send the improved version for that. I didn't do it yet because I was waiting for the ILK wm patch to land.

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #25 lore raw

PATCH v4 7/8 drm/i915: Do not compute watermarks on a noop.

In reply to Maarten Lankhorst

No atomic state should be included after all validation when nothing has changed. During modeset all active planes will be added to the state, while disabled planes won't change their state.

Signed-off-by Maarten Lankhorst <[email protected]>

Cc Matt Roper <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++---------------
 3 files changed, 51 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -1

index 00cb261c6787..6bb1f5dbc7a0 100644
@@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	}
 
 	ret = 0;
-	if (dev_priv->display.compute_pipe_wm) {
+	if (dev_priv->display.compute_pipe_wm &&
+	    (mode_changed || pipe_config->update_pipe || crtc_state->planes_changed)) {
 		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
 		if (ret)
 			return ret;

drivers/gpu/drm/i915/intel_drv.h +13 -0

index 8effb9ece21e..144597ac74e3 100644
@@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
+				      struct intel_plane *plane)
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
+
+	return to_intel_plane_state(plane_state);
+}
+
+
 int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);

drivers/gpu/drm/i915/intel_pm.c +36 ~~-25~~

104 lines not on this page · raw message

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-18 20:51 UTC #26 lore raw

Re: [PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.

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

Quoted 4 lines

No atomic state should be included after all validation when nothing has
changed. During modeset all active planes will be added to the state, while disabled planes won't change their state.

As someone who is also not super familiar with the new watermarks code, I really really wish I had a more detailed commit message to allow me to understand your train of thought. I'll ask some questions below to validate my understanding.

Quoted 24 lines

Signed-off-by Maarten Lankhorst <[email protected]>
Cc Matt Roper <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 3 +- drivers/gpu/drm/i915/intel_drv.h | 13 ++++++++ drivers/gpu/drm/i915/intel_pm.c | 61 +++++++++++++++++++++-----
----------
 3 files changed, 51 insertions(+), 26 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 00cb261c6787..6bb1f5dbc7a0 100644
@@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
 drm_crtc *crtc,
  	}
ret = 0;
drivers/gpu/drm/i915/intel_display.c +2 -1
-	if (dev_priv->display.compute_pipe_wm) {
+	if (dev_priv->display.compute_pipe_wm &&
+	    (mode_changed || pipe_config->update_pipe || crtc_state-
Quoted 1 line
planes_changed)) {

Quoted 4 lines

ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
if (ret)
return ret;

Can't this chunk be on its own separate commit? I'm not sure why the rest of the commit is related to this change.

It seems the rest of the commit is aimed reducing the number of planes we have to lock, not about not computing WMs if nothing in the pipe has changed.

Quoted 20 lines

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 8effb9ece21e..144597ac74e3 100644
@@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
 drm_atomic_state *state,

return to_intel_crtc_state(crtc_state);
}

drivers/gpu/drm/i915/intel_drv.h +3 -0

+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state

*state,

drivers/gpu/drm/i915/intel_drv.h +5 -0

+				      struct intel_plane *plane)
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state,

&plane->base);

drivers/gpu/drm/i915/intel_drv.h intel_atomic_get_crtc_state(struct drm_atomic_state *state, · line 1598

Quoted 5 lines

drivers/gpu/drm/i915/intel_drv.h +5 -0
+
+	return to_intel_plane_state(plane_state);
+}
+
+

Two newlines above.

It seems you'll be able to simplify a lot of stuff with this new function. I'm looking forward to review that patch :)

Quoted 64 lines

int intel_atomic_setup_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
struct intel_crtc_state *crtc_state);
drivers/gpu/drm/i915/intel_pm.c +0 -0
index 379eabe093cb..8fb8c6891ae6 100644
@@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
 drm_i915_private *dev_priv,
  		cur_latency *= 5;
  	}
drivers/gpu/drm/i915/intel_pm.c +0 -3
-	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
-					     pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
spr_latency);
drivers/gpu/drm/i915/intel_pm.c +0 -1
-	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
cur_latency);
drivers/gpu/drm/i915/intel_pm.c +0 -1
-	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
result->pri_val);
drivers/gpu/drm/i915/intel_pm.c +2 -0
+	if (pristate) {
+		result->pri_val = ilk_compute_pri_wm(cstate,
pristate,
drivers/gpu/drm/i915/intel_pm.c +1 -0
+						     pri_latency,
level);
drivers/gpu/drm/i915/intel_pm.c +1 -0
+		result->fbc_val = ilk_compute_fbc_wm(cstate,
pristate, result->pri_val);
drivers/gpu/drm/i915/intel_pm.c +4 -0
+	}
+
+	if (sprstate)
+		result->spr_val = ilk_compute_spr_wm(cstate,
sprstate, spr_latency);
drivers/gpu/drm/i915/intel_pm.c +3 -0
+
+	if (curstate)
+		result->cur_val = ilk_compute_cur_wm(cstate,
curstate, cur_latency);
+
result->enable = true;
}
drivers/gpu/drm/i915/intel_pm.c +0 -1
@@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
  	const struct drm_i915_private *dev_priv = dev->dev_private;
  	struct intel_crtc_state *cstate = NULL;
  	struct intel_plane *intel_plane;
-	struct drm_plane_state *ps;
  	struct intel_plane_state *pristate = NULL;
  	struct intel_plane_state *sprstate = NULL;
struct intel_plane_state *curstate = NULL;
drivers/gpu/drm/i915/intel_pm.c +0 -0
@@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
  	memset(pipe_wm, 0, sizeof(*pipe_wm));
for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
drivers/gpu/drm/i915/intel_pm.c +4 -4
-		ps = drm_atomic_get_plane_state(state,
-						&intel_plane->base);
-		if (IS_ERR(ps))
-			return PTR_ERR(ps);
+		struct intel_plane_state *ps;
+
+		ps = intel_atomic_get_existing_plane_state(state,
+							   intel_pla
ne);

drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc, · line 2320

Quoted 2 lines

drivers/gpu/drm/i915/intel_pm.c +2 -0
+		if (!ps)
+			continue;

Ok, let me see if I understood: if some of these planes is not part of the atomic commit, you're not going to include it in the calculations since its value is not going to change. This would allow us lock less planes, which is the main advantage. Is this correct?

If yes, how much do we really gain by saving some plane locking?

So by not assigning values to result->x_val at ilk_compute_wm_level() when NULL is passed you're going to preserve whatever correct values were already set earlier, so you don't need to recompute them. Is this correct?

If the answer to the above is "yes", then don't we need to remove the memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise, nothing will be preserved.

There's also the zero-initialization of "config" to consider.

Or maybe instead of all of the above, we're working under the assumption that if some of the planes is not part of the atomic commit, then all its relevant WM values will be zero?

Quoted 19 lines

if (intel_plane->base.type ==
DRM_PLANE_TYPE_PRIMARY)
drivers/gpu/drm/i915/intel_pm.c +0 -2
-			pristate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type ==
DRM_PLANE_TYPE_OVERLAY)
drivers/gpu/drm/i915/intel_pm.c +0 -2
-			sprstate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type ==
DRM_PLANE_TYPE_CURSOR)
drivers/gpu/drm/i915/intel_pm.c +2 -1
-			curstate = to_intel_plane_state(ps);
+			pristate = ps;
+		else if (intel_plane->base.type ==
DRM_PLANE_TYPE_OVERLAY) {
drivers/gpu/drm/i915/intel_pm.c +4 -0
+			sprstate = ps;
+
+			if (ps) {
+				pipe_wm->sprites_enabled = sprstate-
Quoted 1 line
visible;
drivers/gpu/drm/i915/intel_pm.c +1 -0
+				pipe_wm->sprites_scaled = sprstate-

Quoted 1 line

Quoted 1 line
visible &&

You're setting these values here...

Quoted 7 lines

drivers/gpu/drm/i915/intel_pm.c +1 -0
+					(drm_rect_width(&sprstate-
Quoted 1 line
dst) != drm_rect_width(&sprstate->src) >> 16 ||
drivers/gpu/drm/i915/intel_pm.c +1 -0
+					drm_rect_height(&sprstate-
Quoted 1 line
dst) != drm_rect_height(&sprstate->src) >> 16);
drivers/gpu/drm/i915/intel_pm.c +2 -0
+			}
+		} else if (intel_plane->base.type ==
DRM_PLANE_TYPE_CURSOR)

(missing brackets here, but if you follow my suggestion below you won't need them)

Quoted 11 lines

drivers/gpu/drm/i915/intel_pm.c +1 -0
+			curstate = ps;
}
drivers/gpu/drm/i915/intel_pm.c +0 -3
-	config.sprites_enabled = sprstate->visible;
-	config.sprites_scaled = sprstate->visible &&
-		(drm_rect_width(&sprstate->dst) !=
drm_rect_width(&sprstate->src) >> 16 ||
drivers/gpu/drm/i915/intel_pm.c +0 -1
-		drm_rect_height(&sprstate->dst) !=
drm_rect_height(&sprstate->src) >> 16);
drivers/gpu/drm/i915/intel_pm.c +2 -0
+	config.sprites_enabled = pipe_wm->sprites_enabled;
+	config.sprites_scaled = pipe_wm->sprites_scaled;

Quoted 4 lines

pipe_wm->pipe_enabled = cstate->base.active; pipe_wm->sprites_enabled = config.sprites_enabled; pipe_wm->sprites_scaled = config.sprites_scaled;

...but then we re-set them here.

Either you could remove these assignments here, or you could move the "if (ps)" from the loop to outside it, becoming "if (sprstate)", making the post-patch code similar to the pre-patch code. Since both pipe_wm and config are zero-initialized you don't even need to think about the !sprstate case.

Quoted 30 lines

/* ILK/SNB: LP2+ watermarks only w/o sprites */
drivers/gpu/drm/i915/intel_pm.c +1 -1
-	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
+	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
max_level = 1;

/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
drivers/gpu/drm/i915/intel_pm.c +0 -0
@@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
  	ilk_compute_wm_reg_maximums(dev, 1, &max);
for (level = 1; level <= max_level; level++) {
drivers/gpu/drm/i915/intel_pm.c +1 -1
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
drivers/gpu/drm/i915/intel_pm.c +0 -1
-				     pristate, sprstate, curstate,
&wm);
drivers/gpu/drm/i915/intel_pm.c +1 -0
+				     pristate, sprstate, curstate,
wm);

/*
* Disable any watermark level that exceeds the
* register maximums since such watermarks are
* always invalid.
*/
drivers/gpu/drm/i915/intel_pm.c +1 -1
-		if (!ilk_validate_wm_level(level, &max, &wm))
+		if (!ilk_validate_wm_level(level, &max, wm))
break;

drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc, · line 2368

Quoted 2 lines

drivers/gpu/drm/i915/intel_pm.c +0 -2
-
-		pipe_wm->wm[level] = wm;

The change in the chunk above is that we're now leaving with non-zero wm->{pri,spr,cur}_val if some level is invalid. Given the current complexity of the code, it's not trivial verify whether nothing later is assuming that invalid levels are all-zero, but it looks like we're safe. Anyway, could you please move this to a separate patch that comes before the other changes? It seems this change would be safe alone, and we're seeing problems with WMs these days, so having nice bisectability is a huge plus.

Thanks,
Paulo

Quoted 3 lines

}

return 0;

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-03-01 10:12 UTC #27 lore raw

Re: [PATCH v4 7/8] drm/i915: Do not compute watermarks on a noop.

Op 18-02-16 om 21:51 schreef Zanoni, Paulo R:

Quoted 212 lines

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 4 lines
No atomic state should be included after all validation when nothing has
changed. During modeset all active planes will be added to the state, while disabled planes won't change their state.
As someone who is also not super familiar with the new watermarks code, I really really wish I had a more detailed commit message to allow me to understand your train of thought. I'll ask some questions below to validate my understanding.
Quoted 25 lines
Signed-off-by Maarten Lankhorst <[email protected]>
Cc Matt Roper <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h     | 13 ++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 61 +++++++++++++++++++++-----
----------
 3 files changed, 51 insertions(+), 26 deletions(-)
drivers/gpu/drm/i915/intel_display.c +2 -1
index 00cb261c6787..6bb1f5dbc7a0 100644
@@ -11910,7 +11910,8 @@ static int intel_crtc_atomic_check(struct
 drm_crtc *crtc,
 	}
 
 	ret = 0;
-	if (dev_priv->display.compute_pipe_wm) {
+	if (dev_priv->display.compute_pipe_wm &&
+	    (mode_changed || pipe_config->update_pipe || crtc_state-
 > planes_changed)) {
 		ret = dev_priv->display.compute_pipe_wm(intel_crtc,
state);
drivers/gpu/drm/i915/intel_display.c static int intel_crtc_atomic_check(struct drm_crtc *crtc, · line 11917
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +0 -0
 		if (ret)
 			return ret;
Can't this chunk be on its own separate commit? I'm not sure why the rest of the commit is related to this change.
It seems the rest of the commit is aimed reducing the number of planes we have to lock, not about not computing WMs if nothing in the pipe has changed.
Quoted 15 lines
drivers/gpu/drm/i915/intel_drv.h +4 -0
index 8effb9ece21e..144597ac74e3 100644
@@ -1583,6 +1583,19 @@ intel_atomic_get_crtc_state(struct
 drm_atomic_state *state,
 
 	return to_intel_crtc_state(crtc_state);
 }
+
+static inline struct intel_plane_state *
+intel_atomic_get_existing_plane_state(struct drm_atomic_state
 *state,
+				      struct intel_plane *plane)
drivers/gpu/drm/i915/intel_drv.h intel_atomic_get_crtc_state(struct · line 1601
Quoted 10 lines
drivers/gpu/drm/i915/intel_drv.h +9 -0
+{
+	struct drm_plane_state *plane_state;
+
+	plane_state = drm_atomic_get_existing_plane_state(state,
 &plane->base);
+
+	return to_intel_plane_state(plane_state);
+}
+
+
Two newlines above.
It seems you'll be able to simplify a lot of stuff with this new function. I'm looking forward to review that patch :)
Quoted 56 lines
int intel_atomic_setup_scalers(struct drm_device *dev,
drivers/gpu/drm/i915/intel_drv.h +0 -0
 	struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state);
drivers/gpu/drm/i915/intel_pm.c +0 -5
index 379eabe093cb..8fb8c6891ae6 100644
@@ -2010,11 +2010,18 @@ static void ilk_compute_wm_level(const struct
 drm_i915_private *dev_priv,
 		cur_latency *= 5;
 	}
 
-	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
-					     pri_latency, level);
-	result->spr_val = ilk_compute_spr_wm(cstate, sprstate,
 spr_latency);
-	result->cur_val = ilk_compute_cur_wm(cstate, curstate,
 cur_latency);
-	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate,
result->pri_val);
drivers/gpu/drm/i915/intel_pm.c +2 -0
+	if (pristate) {
+		result->pri_val = ilk_compute_pri_wm(cstate,
pristate,
drivers/gpu/drm/i915/intel_pm.c +1 -0
+						     pri_latency,
level);
drivers/gpu/drm/i915/intel_pm.c +1 -0
+		result->fbc_val = ilk_compute_fbc_wm(cstate,
pristate, result->pri_val);
drivers/gpu/drm/i915/intel_pm.c +4 -0
+	}
+
+	if (sprstate)
+		result->spr_val = ilk_compute_spr_wm(cstate,
sprstate, spr_latency);
drivers/gpu/drm/i915/intel_pm.c +3 -0
+
+	if (curstate)
+		result->cur_val = ilk_compute_cur_wm(cstate,
curstate, cur_latency);
drivers/gpu/drm/i915/intel_pm.c +1 -1
+
 	result->enable = true;
 }
@@ -2287,7 +2294,6 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
 	const struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_state *cstate = NULL;
 	struct intel_plane *intel_plane;
-	struct drm_plane_state *ps;
 	struct intel_plane_state *pristate = NULL;
 	struct intel_plane_state *sprstate = NULL;
drivers/gpu/drm/i915/intel_pm.c +0 -1
 	struct intel_plane_state *curstate = NULL;
@@ -2306,30 +2312,37 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
 	memset(pipe_wm, 0, sizeof(*pipe_wm));
 
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		ps = drm_atomic_get_plane_state(state,
drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct · line 2322
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +6 -3
-						&intel_plane->base);
-		if (IS_ERR(ps))
-			return PTR_ERR(ps);
+		struct intel_plane_state *ps;
+
+		ps = intel_atomic_get_existing_plane_state(state,
+							   intel_pla
 ne);
+		if (!ps)
+			continue;
Ok, let me see if I understood: if some of these planes is not part of the atomic commit, you're not going to include it in the calculations since its value is not going to change. This would allow us lock less planes, which is the main advantage. Is this correct?
If yes, how much do we really gain by saving some plane locking?
So by not assigning values to result->x_val at ilk_compute_wm_level() when NULL is passed you're going to preserve whatever correct values were already set earlier, so you don't need to recompute them. Is this correct?
If the answer to the above is "yes", then don't we need to remove the memset(pipe_wm, 0) at the beginning of ilk_compute_wm_level? Otherwise, nothing will be preserved.
There's also the zero-initialization of "config" to consider.
Or maybe instead of all of the above, we're working under the assumption that if some of the planes is not part of the atomic commit, then all its relevant WM values will be zero?
Quoted 19 lines
drivers/gpu/drm/i915/intel_pm.c +7 -5
 		if (intel_plane->base.type ==
 DRM_PLANE_TYPE_PRIMARY)
-			pristate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type ==
 DRM_PLANE_TYPE_OVERLAY)
-			sprstate = to_intel_plane_state(ps);
-		else if (intel_plane->base.type ==
 DRM_PLANE_TYPE_CURSOR)
-			curstate = to_intel_plane_state(ps);
+			pristate = ps;
+		else if (intel_plane->base.type ==
 DRM_PLANE_TYPE_OVERLAY) {
+			sprstate = ps;
+
+			if (ps) {
+				pipe_wm->sprites_enabled = sprstate-
 > visible;
+				pipe_wm->sprites_scaled = sprstate-
Quoted 1 line
Quoted 1 line
visible &&
You're setting these values here...
Quoted 7 lines
drivers/gpu/drm/i915/intel_pm.c +1 -0
+					(drm_rect_width(&sprstate-
Quoted 1 line
dst) != drm_rect_width(&sprstate->src) >> 16 ||
drivers/gpu/drm/i915/intel_pm.c +1 -0
+					drm_rect_height(&sprstate-
Quoted 1 line
dst) != drm_rect_height(&sprstate->src) >> 16);
drivers/gpu/drm/i915/intel_pm.c +2 -0
+			}
+		} else if (intel_plane->base.type ==
DRM_PLANE_TYPE_CURSOR)
(missing brackets here, but if you follow my suggestion below you won't need them)
Quoted 9 lines
drivers/gpu/drm/i915/intel_pm.c +1 -3
+			curstate = ps;
 	}
 
-	config.sprites_enabled = sprstate->visible;
-	config.sprites_scaled = sprstate->visible &&
-		(drm_rect_width(&sprstate->dst) !=
drm_rect_width(&sprstate->src) >> 16 ||
drivers/gpu/drm/i915/intel_pm.c +0 -1
-		drm_rect_height(&sprstate->dst) !=
drm_rect_height(&sprstate->src) >> 16);
drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc, · line 2342
Quoted 6 lines
drivers/gpu/drm/i915/intel_pm.c +2 -0
+	config.sprites_enabled = pipe_wm->sprites_enabled;
+	config.sprites_scaled = pipe_wm->sprites_scaled;
 
 	pipe_wm->pipe_enabled = cstate->base.active;
 	pipe_wm->sprites_enabled = config.sprites_enabled;
 	pipe_wm->sprites_scaled = config.sprites_scaled;
...but then we re-set them here.

Quoted 5 lines

Either you could remove these assignments here, or you could move the "if (ps)" from the loop to outside it, becoming "if (sprstate)", making the post-patch code similar to the pre-patch code. Since both pipe_wm and config are zero-initialized you don't even need to think about the !sprstate case.

Makes sense, will do so.

Quoted 32 lines

Quoted 22 lines

drivers/gpu/drm/i915/intel_pm.c +3 -3

 	/* ILK/SNB: LP2+ watermarks only w/o sprites */
-	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
+	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
 		max_level = 1;
 
 	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */

@@ -2352,20 +2365,18 @@ static int ilk_compute_pipe_wm(struct
 intel_crtc *intel_crtc,
 	ilk_compute_wm_reg_maximums(dev, 1, &max);
 
 	for (level = 1; level <= max_level; level++) {
-		struct intel_wm_level wm = {};
+		struct intel_wm_level *wm = &pipe_wm->wm[level];
 
 		ilk_compute_wm_level(dev_priv, intel_crtc, level,
 cstate,
-				     pristate, sprstate, curstate,
 &wm);
+				     pristate, sprstate, curstate,
 wm);

drivers/gpu/drm/i915/intel_pm.c static int ilk_compute_pipe_wm(struct · line 2368

Quoted 10 lines

drivers/gpu/drm/i915/intel_pm.c +1 -3

 		/*
 		 * Disable any watermark level that exceeds the
 		 * register maximums since such watermarks are
 		 * always invalid.
 		 */
-		if (!ilk_validate_wm_level(level, &max, &wm))
+		if (!ilk_validate_wm_level(level, &max, wm))
 			break;
-
-		pipe_wm->wm[level] = wm;

Quoted 9 lines

The change in the chunk above is that we're now leaving with non-zero wm->{pri,spr,cur}_val if some level is invalid. Given the current complexity of the code, it's not trivial verify whether nothing later is assuming that invalid levels are all-zero, but it looks like we're safe. Anyway, could you please move this to a separate patch that comes before the other changes? It seems this change would be safe alone, and we're seeing problems with WMs these days, so having nice bisectability is a huge plus.

Well caught, I think we need to calculate even invalid values, but set ->enable = false in that case. That is the only way we can ensure that the wm levels are calculated correctly.
I've sent those as separate patches, so I get a full CI run from them.

[PATCH 1/2] drm/i915: Allow preservation of watermarks.
[PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-10 12:49 UTC #28 lore raw

PATCH v4 8/8 drm/i915: Remove vblank wait from hsw_enable_ips.

In reply to Maarten Lankhorst

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/gpu/drm/i915/intel_display.c +0 -3

index 6bb1f5dbc7a0..19a8d376d63e 100644
@@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	if (!crtc->config->ips_enabled)
 		return;
 
-	/* We can only enable IPS after we enable a plane and wait for a vblank */
-	intel_wait_for_vblank(dev, crtc->pipe);
-
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {
 		mutex_lock(&dev_priv->rps.hw_lock);

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

2016-02-12 12:06 UTC #29 lore raw

Re: [PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.

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

Quoted 18 lines

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.
Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 6bb1f5dbc7a0..19a8d376d63e 100644
@@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
  	if (!crtc->config->ips_enabled)
  		return;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	/* We can only enable IPS after we enable a plane and wait

Quoted 1 line

for a vblank */

Perhaps we can keep the comment, but change it into something like:

/* We can only enable IPS after we enable a plane and wait for a vblank, but we don't need the wait here because of XYZ. */

Quoted 5 lines

drivers/gpu/drm/i915/intel_display.c +0 -2
-	intel_wait_for_vblank(dev, crtc->pipe);
-
assert_plane_enabled(dev_priv, crtc->plane); if (IS_BROADWELL(dev)) { mutex_lock(&dev_priv->rps.hw_lock);

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-02-16 10:32 UTC #30 lore raw

Re: [PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.

Hey,

Op 12-02-16 om 13:06 schreef Zanoni, Paulo R:

Quoted 18 lines

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
Quoted 17 lines
intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.
Signed-off-by Maarten Lankhorst <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/gpu/drm/i915/intel_display.c +0 -0
index 6bb1f5dbc7a0..19a8d376d63e 100644
@@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	if (!crtc->config->ips_enabled)
 		return;
 

Quoted 7 lines

Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +0 -1
-	/* We can only enable IPS after we enable a plane and wait
for a vblank */
Perhaps we can keep the comment, but change it into something like:
/* We can only enable IPS after we enable a plane and wait for a vblank, but we don't need the wait here because of XYZ. */

post_plane_update only gets run after a vblank wait now, so I think adding a comment here is optional. But yeah I'll make it more clear in the commit.

~Maarten
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst

author

2016-03-23 13:33 UTC #31 lore raw

PATCH v5 8/8 drm/i915: Remove vblank wait from hsw_enable_ips, v2.

In reply to Zanoni, Paulo R

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.

Changes since v1:
- Add comment explaining why vblank wait is performed. (Paulo)

Signed-off-by Maarten Lankhorst <[email protected]>

---
Noticed this one still in my branch, does this look ok?

~Maarten

drivers/gpu/drm/i915/intel_display.c +5 -2

index dc920cfc3fd7..6dcc159ce6ac 100644
@@ -4454,8 +4454,11 @@ void hsw_enable_ips(struct intel_crtc *crtc)
 	if (!crtc->config->ips_enabled)
 		return;
 
-	/* We can only enable IPS after we enable a plane and wait for a vblank */
-	intel_wait_for_vblank(dev, crtc->pipe);
+	/*
+	 * We can only enable IPS after we enable a plane and wait for a vblank
+	 * This function is called from post_plane_update, which is run after
+	 * a vblank wait.
+	 */
 
 	assert_plane_enabled(dev_priv, crtc->plane);
 	if (IS_BROADWELL(dev)) {

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Zanoni, Paulo R

Reviewed-by on v5 8/8

2016-03-23 14:43 UTC #32 lore raw

Re: [PATCH v5 8/8] drm/i915: Remove vblank wait from hsw_enable_ips, v2.

Em Qua, 2016-03-23 às 14:33 +0100, Maarten Lankhorst escreveu:

Quoted 29 lines

intel_post_plane_update did an extra vblank wait that's no longer needed when enabling ips.
Changes since v1:
- Add comment explaining why vblank wait is performed. (Paulo)
Signed-off-by Maarten Lankhorst <[email protected]>
---
Noticed this one still in my branch, does this look ok?
~Maarten
drivers/gpu/drm/i915/intel_display.c +0 -0
index dc920cfc3fd7..6dcc159ce6ac 100644
@@ -4454,8 +4454,11 @@ void hsw_enable_ips(struct intel_crtc *crtc)
  	if (!crtc->config->ips_enabled)
  		return;
drivers/gpu/drm/i915/intel_display.c +0 -1
-	/* We can only enable IPS after we enable a plane and wait
for a vblank */
drivers/gpu/drm/i915/intel_display.c +2 -1
-	intel_wait_for_vblank(dev, crtc->pipe);
+	/*
+	 * We can only enable IPS after we enable a plane and wait
for a vblank
drivers/gpu/drm/i915/intel_display.c +1 -0
+	 * This function is called from post_plane_update, which is
run after

drivers/gpu/drm/i915/intel_display.c void hsw_enable_ips(struct intel_crtc *crtc) · line 4461

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +2 -0
+	 * a vblank wait.
+	 */

There are actually other callers, such as intel_dp.c, but I suppose they are correct since they can only be called when the planes are already enabled, so:

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 4 lines

assert_plane_enabled(dev_priv, crtc->plane); if (IS_BROADWELL(dev)) {

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2016-02-15 13:35 UTC #33 lore raw

✗ Fi.CI.BAT: failure for Kill off intel_crtc->atomic! (rev8)

In reply to Maarten Lankhorst

== Summary ==

Series 77v8 Kill off intel_crtc->atomic!
http://patchwork.freedesktop.org/api/1.0/series/77/revisions/8/mbox/

Test kms_flip:

        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (skl-i5k-2)
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (skl-i5k-2)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> FAIL       (hsw-brixbox)
                fail       -> PASS       (bdw-nuci7)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)
                pass       -> FAIL       (bdw-ultra)

bdw-nuci7        total:162  pass:152  dwarn:0   dfail:0   fail:0   skip:10 
bdw-ultra        total:165  pass:151  dwarn:0   dfail:0   fail:1   skip:13 
bsw-nuc-2        total:165  pass:136  dwarn:0   dfail:0   fail:0   skip:29 
byt-nuc          total:165  pass:141  dwarn:0   dfail:0   fail:0   skip:24 
hsw-brixbox      total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
hsw-gt2          total:165  pass:154  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:165  pass:115  dwarn:1   dfail:0   fail:1   skip:48 
ivb-t430s        total:165  pass:150  dwarn:0   dfail:0   fail:1   skip:14 
skl-i5k-2        total:165  pass:147  dwarn:3   dfail:0   fail:0   skip:15 
snb-dellxps      total:165  pass:142  dwarn:0   dfail:0   fail:1   skip:22 
snb-x220t        total:165  pass:142  dwarn:0   dfail:0   fail:2   skip:21

Results at /archive/results/CI_IGT_test/Patchwork_1389/

f2110d8eac120416f8f5669f2aa561d9ab330a77 drm-intel-nightly: 2016y-02m-15d-09h-53m-04s UTC integration manifest 6c3070b739e39258955200bc894f404624443512 drm/i915: Remove vblank wait from hsw_enable_ips. a2efbd40f02b67ccfad95fe1aaa15ee16d064587 drm/i915: Do not compute watermarks on a noop. d4bd3c52cb02bc40d649a9f6a894ea9cea39cd5f drm/i915: Nuke fbc members from intel_crtc->atomic, v2. e18153d768dc4e148e64593a6426c7c17f189299 drm/i915: Remove some post-commit members from intel_crtc->atomic, v2. bbea33f7e7ddb289c194963cf8ebd2e6ff2dc800 drm/i915: Remove update_sprite_watermarks. e593cced664d88417e24324260b9458904268c3d drm/i915: Kill off intel_crtc->atomic.wait_vblank, v4. 3b74695aaa8dd202baec985c3338b36083648c17 drm/i915: Unify power domain handling. a1af80d9af7f7bbd18b60c0e633436ff2bc53c92 drm/i915: Pass crtc state to modeset_get_crtc_power_domains.

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx