[PATCH v2 00/27] Convert to atomic, part 3.

[PATCH v2 00/27] Convert to atomic, part 3.

[Maarten Lankhorst]

This patch requires the following patch from airlied/drm-next:
"[PATCH] drm/atomic: Clear crtc_state->active in drm_atomic_helper_set_config."

Now that suspend/restore is atomic it's time to clean up some
remaining issues. First I clean up the suspend code some more now
that it's atomic.

After that I try to make a better separation between checking and
modeset. This meant that intel_modeset_pipe_config should no longer
touch mode_changed, because it will only be called when a modeset
will already happen to prevent excessive rechecking.

I also clean up the scaler stuff slightly, and move some of it to the
crtc state. Because of feedback on my previous series I'll always add
the plane, instead of handling switching from/to hq mode by waiting
poking scaler 0.

After that it's time to clean up the plane functions, and making sure
the atomic commit runs correctly. This has gone through many
iterations, and I think the way here is the most clean way to deal
with it.

Disabling all planes before a modeset, and committing after, even
if the crtc is in a disabled state provided the most flexibility.
The last callers of the transitional plane helpers are killed, and
intel_set_mode becomes the replacement intel_atomic_commit.

Changes since previous version:
 - Disable all unknown planes correctly by forcing it with a quirk,
  even if sprites were previously enabled it should work without
  requiring a full modeset.
 - Add a plane in the intel_atomic_setup_scalers function again,
   not adding a plane was too tricky.
 - Reorder plane cleanups, 'atomic plane updates in a nutshell'
   is even less scary now.
 - Disable skylake scalers during vblank evasion.

Maarten Lankhorst (27):
  drm/i915: Always reset in intel_crtc_restore_mode
  drm/i915: Use crtc state in intel_modeset_pipe_config
  drm/i915: clean up intel_sanitize_crtc, v2
  drm/i915: Update power domains only on affected crtc's.
  drm/i915: add fastboot checks for has_audio and has_infoframe
  drm/i915: Clean up intel_atomic_setup_scalers slightly.
  drm/i915: Add a simple atomic crtc check function, v2.
  drm/i915: Move scaler setup to check crtc function, v2.
  drm/i915: Assign a new pll from the crtc check function.
  drm/i915: Do not run most checks when there's no modeset.
  drm/i915: Split skl_update_scaler, v2.
  drm/i915: Split plane updates of crtc->atomic into a helper, v2.
  drm/i915: clean up plane commit functions
  drm/i915: clean up atomic plane check functions
  drm/i915: remove force argument from disable_plane
  drm/i915: move detaching scalers to begin_crtc_commit, v2.
  drm/i915: Move crtc commit updates to separate functions.
  drm/i915: Handle disabling planes better.
  drm/i915: atomic plane updates in a nutshell
  drm/i915: Update less state during modeset.
  drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip
  drm/i915: Make setting color key atomic.
  drm/i915: Remove transitional references from
    intel_plane_atomic_check.
  drm/i915: Use full atomic modeset.
  drm/i915: Call plane update functions directly from
    intel_atomic_commit.
  drm/i915: always disable irqs in intel_pipe_update_start
  drm/i915: Only commit planes on crtc's that have changed planes.

 drivers/gpu/drm/i915/i915_debugfs.c       |    8 +-
 drivers/gpu/drm/i915/i915_drv.c           |    3 -
 drivers/gpu/drm/i915/intel_atomic.c       |  159 +--
 drivers/gpu/drm/i915/intel_atomic_plane.c |   44 +-
 drivers/gpu/drm/i915/intel_display.c      | 1815 +++++++++++++----------------
 drivers/gpu/drm/i915/intel_dp.c           |    2 +-
 drivers/gpu/drm/i915/intel_drv.h          |   41 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  191 ++-
 8 files changed, 937 insertions(+), 1326 deletions(-)

-- 
2.1.0

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

[PATCH v2 01/27] drm/i915: Always reset in intel_crtc_restore_mode

[Maarten Lankhorst]

And get rid of things that are no longer true. This function is only
used for forcing a modeset when encoder properties are changed.

All the existing state is fine in this case, only setting mode_changed
will force a full recalculation here, and take all the state needed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 62 +++++++++++-------------------------
 1 file changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb9f07b1e5ca..a1f99a32bddb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12753,66 +12753,40 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
-	struct intel_crtc *intel_crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	struct drm_connector_state *connector_state;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	int ret;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
-		DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
+		DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
 			      crtc->base.id);
 		return;
 	}
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
 
-	/* The force restore path in the HW readout code relies on the staged
-	 * config still keeping the user requested config while the actual
-	 * state has been overwritten by the configuration read from HW. We
-	 * need to copy the staged config to the atomic state, otherwise the
-	 * mode set will just reapply the state the HW is already in. */
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->base.crtc != crtc)
-			continue;
-
-		for_each_intel_connector(dev, connector) {
-			if (connector->base.state->best_encoder != &encoder->base)
-				continue;
-
-			connector_state = drm_atomic_get_connector_state(state, &connector->base);
-			if (IS_ERR(connector_state)) {
-				DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
-					      connector->base.base.id,
-					      connector->base.name,
-					      PTR_ERR(connector_state));
-				continue;
-			}
+retry:
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	ret = PTR_ERR_OR_ZERO(crtc_state);
+	if (!ret) {
+		if (!crtc_state->active)
+			goto out;
 
-			connector_state->crtc = crtc;
-		}
+		crtc_state->mode_changed = true;
+		ret = intel_modeset_compute_config(state);
 	}
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state)) {
-			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
-				      intel_crtc->base.base.id,
-				      PTR_ERR(crtc_state));
-			continue;
-		}
+	if (!ret)
+		ret = intel_set_mode_checked(state);
 
-		if (&intel_crtc->base == crtc)
-			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(state->acquire_ctx);
+		goto retry;
 	}
 
-	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
-					crtc->primary->fb, crtc->x, crtc->y);
-
-	ret = intel_set_mode(state);
 	if (ret)
+out:
 		drm_atomic_state_free(state);
 }
 
-- 
2.1.0

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

[PATCH v2 02/27] drm/i915: Use crtc state in intel_modeset_pipe_config

[Maarten Lankhorst]

Grabbing crtc state from atomic state is a lot more involved,
and make sure connectors are added before calling this function.

Move check_digital_port_conflicts to intel_modeset_checks,
it's only useful to check it on a modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++----------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a1f99a32bddb..0f031658a297 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11748,10 +11748,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 
 static int
 intel_modeset_pipe_config(struct drm_crtc *crtc,
-			  struct drm_atomic_state *state)
+			  struct intel_crtc_state *pipe_config)
 {
-	struct drm_crtc_state *crtc_state;
-	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state = pipe_config->base.state;
 	struct intel_encoder *encoder;
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
@@ -11764,26 +11763,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	if (!check_digital_port_conflicts(state)) {
-		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
-		return -EINVAL;
-	}
-
-	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-	if (WARN_ON(!crtc_state))
-		return -EINVAL;
-
-	pipe_config = to_intel_crtc_state(crtc_state);
-
-	/*
-	 * XXX: Add all connectors to make the crtc state match the encoders.
-	 */
-	if (!needs_modeset(&pipe_config->base)) {
-		ret = drm_atomic_add_affected_connectors(state, crtc);
-		if (ret)
-			return ret;
-	}
-
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =
@@ -12614,6 +12593,11 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	int ret;
 
+	if (!check_digital_port_conflicts(state)) {
+		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -12656,7 +12640,14 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		if (!crtc_state->enable)
 			continue;
 
-		ret = intel_modeset_pipe_config(crtc, state);
+		if (!needs_modeset(crtc_state)) {
+			ret = drm_atomic_add_affected_connectors(state, crtc);
+			if (ret)
+				return ret;
+		}
+
+		ret = intel_modeset_pipe_config(crtc,
+					to_intel_crtc_state(crtc_state));
 		if (ret)
 			return ret;
 
-- 
2.1.0

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

[PATCH v2 03/27] drm/i915: clean up intel_sanitize_crtc, v2

[Maarten Lankhorst]

Instead of breaking all connections manually, kill them with
atomic modeset, if it happens. Now that the everything's atomic
some code becomes obsolete.

Forcing a atomic modeset with null mode and all connectors gone
updates the state too, in a cleaner way.

Because load detection is atomic too there can be no mismatch between
crtc->active and crtc->state->active

Changes since v1:
- Reword commit message.
- Set null mode and break all connectors when force disabling crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |   8 +--
 drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 3 files changed, 53 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 51fe0fbcd00c..db23916c1875 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3637,7 +3637,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 		bool active = pipe_config->base.active;
 
 		if (active) {
-			intel_crtc_control(&crtc->base, false);
+			intel_crtc_control(&crtc->base, false, false);
 			pipe_config = to_intel_crtc_state(crtc->base.state);
 		}
 
@@ -3647,7 +3647,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
 
 		if (active)
-			intel_crtc_control(&crtc->base, true);
+			intel_crtc_control(&crtc->base, true, false);
 	}
 	drm_modeset_unlock_all(dev);
 }
@@ -3671,7 +3671,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 		bool active = pipe_config->base.active;
 
 		if (active) {
-			intel_crtc_control(&crtc->base, false);
+			intel_crtc_control(&crtc->base, false, false);
 			pipe_config = to_intel_crtc_state(crtc->base.state);
 		}
 
@@ -3681,7 +3681,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
 					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
 
 		if (active)
-			intel_crtc_control(&crtc->base, true);
+			intel_crtc_control(&crtc->base, true, false);
 	}
 	drm_modeset_unlock_all(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f031658a297..1936a28a57c1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6192,7 +6192,7 @@ free:
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
-int intel_crtc_control(struct drm_crtc *crtc, bool enable)
+int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_mode_config *config = &dev->mode_config;
@@ -6202,10 +6202,13 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	struct drm_atomic_state *state;
 	int ret;
 
-	if (enable == intel_crtc->active)
+	if (WARN_ON(active && force))
+		return -EINVAL;
+
+	if (active == intel_crtc->active)
 		return 0;
 
-	if (enable && !crtc->state->enable)
+	if (active && !crtc->state->enable)
 		return 0;
 
 	/* this function should be called with drm_modeset_lock_all for now */
@@ -6225,7 +6228,30 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
 		ret = PTR_ERR(pipe_config);
 		goto err;
 	}
-	pipe_config->base.active = enable;
+	pipe_config->base.active = active;
+	if (force) {
+		struct drm_connector *connector;
+
+		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
+		if (ret)
+			goto err;
+
+		list_for_each_entry(connector, &config->connector_list, head) {
+			struct drm_connector_state *conn_state;
+
+			if (connector->state->crtc != crtc)
+				continue;
+
+			conn_state = drm_atomic_get_connector_state(state, connector);
+			if (IS_ERR(conn_state)) {
+				ret = PTR_ERR(conn_state);
+				goto err;
+			}
+			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+			if (ret)
+				goto err;
+		}
+	}
 
 	ret = intel_set_mode(state);
 	if (!ret)
@@ -6249,7 +6275,7 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
 		enable |= intel_encoder->connectors_active;
 
-	intel_crtc_control(crtc, enable);
+	intel_crtc_control(crtc, enable, false);
 }
 
 void intel_encoder_destroy(struct drm_encoder *encoder)
@@ -14812,11 +14838,14 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
 	return true;
 }
 
-static void intel_sanitize_crtc(struct intel_crtc *crtc)
+static void intel_sanitize_crtc(struct intel_crtc *crtc,
+				struct intel_crtc_state *pipe_config)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder;
 	u32 reg;
+	bool enable;
 
 	/* Clear any frame start delays used for debugging left by the BIOS */
 	reg = PIPECONF(crtc->config->cpu_transcoder);
@@ -14833,7 +14862,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	 * disable the crtc (and hence change the state) if it is wrong. Note
 	 * that gen4+ has a fixed plane -> pipe mapping.  */
 	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
-		struct intel_connector *connector;
 		bool plane;
 
 		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
@@ -14846,33 +14874,12 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		to_intel_plane_state(crtc->base.primary->state)->visible = true;
 		crtc->base.primary->crtc = &crtc->base;
 		crtc->plane = !plane;
-		intel_crtc_control(&crtc->base, false);
+		intel_crtc_control(&crtc->base, false, true);
 		crtc->plane = plane;
-
-		/* ... and break all links. */
-		for_each_intel_connector(dev, connector) {
-			if (connector->encoder->base.crtc != &crtc->base)
-				continue;
-
-			connector->base.dpms = DRM_MODE_DPMS_OFF;
-			connector->base.encoder = NULL;
-		}
-		/* multiple connectors may have the same encoder:
-		 *  handle them and break crtc link separately */
-		for_each_intel_connector(dev, connector)
-			if (connector->encoder->base.crtc == &crtc->base) {
-				connector->encoder->base.crtc = NULL;
-				connector->encoder->connectors_active = false;
-			}
-
-		WARN_ON(crtc->active);
-		crtc->base.state->enable = false;
-		crtc->base.state->active = false;
-		crtc->base.enabled = false;
 	}
 
 	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
-	    crtc->pipe == PIPE_A && !crtc->active) {
+	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
 		/* BIOS forgot to enable pipe A, this mostly happens after
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
@@ -14881,36 +14888,16 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	}
 
 	/* Adjust the state of the output pipe according to whether we
-	 * have active connectors/encoders. */
-	intel_crtc_update_dpms(&crtc->base);
-
-	if (crtc->active != crtc->base.state->active) {
-		struct intel_encoder *encoder;
+	 * have active connectors/encoders */
+	enable = false;
+	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
+		enable |= intel_encoder->connectors_active;
 
-		/* This can happen either due to bugs in the get_hw_state
-		 * functions or because the pipe is force-enabled due to the
-		 * pipe A quirk. */
-		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
-			      crtc->base.base.id,
-			      crtc->base.state->enable ? "enabled" : "disabled",
-			      crtc->active ? "enabled" : "disabled");
-
-		crtc->base.state->enable = crtc->active;
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
-
-		/* Because we only establish the connector -> encoder ->
-		 * crtc links if something is active, this means the
-		 * crtc is now deactivated. Break the links. connector
-		 * -> encoder links are only establish when things are
-		 *  actually up, hence no need to break them. */
-		WARN_ON(crtc->active);
-
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
-			WARN_ON(encoder->connectors_active);
-			encoder->base.crtc = NULL;
-		}
-	}
+	/* only turn off separately if configuration's not restored,
+	 * if it's restored it will change mode or turn off anyway.
+	 */
+	if (!enable && crtc->base.state->active && !pipe_config)
+		intel_crtc_control(&crtc->base, false, true);
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
 		/*
@@ -15278,7 +15265,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 				      &crtc->state->adjusted_mode);
 		}
 
-		intel_sanitize_crtc(intel_crtc);
+		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
+				    to_intel_crtc_state(crtc_state));
 
 		/*
 		 * sanitize_crtc may have forced an update of crtc->state,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 807e001e4cee..ec721e31eae8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -993,7 +993,7 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 int intel_display_suspend(struct drm_device *dev);
-int intel_crtc_control(struct drm_crtc *crtc, bool enable);
+int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
-- 
2.1.0

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

[PATCH v2 04/27] drm/i915: Update power domains only on affected crtc's.

[Maarten Lankhorst]

Use for_each_crtc_state to only touch affected crtc's.
In order to make sure that the initial power is still set
correctly we make sure modeset_update_crtc_power_domains is called
during the initial modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  3 ---
 drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d3632c56fdf7..78ef0bb53c36 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 	intel_display_suspend(dev);
 	drm_modeset_unlock_all(dev);
 
-	/* suspending displays will unsets init power */
-	intel_display_set_init_power(dev_priv, true);
-
 	intel_dp_mst_suspend(dev);
 
 	intel_runtime_pm_disable_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1936a28a57c1..64c84fb94b4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5188,42 +5188,50 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	return mask;
 }
 
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
+static void modeset_update_crtc_power_domains(struct drm_atomic_state *state,
+					      bool power_only)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
-	struct intel_crtc *crtc;
+	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }, domains;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
 	/*
 	 * First get all needed power domains, then put all unneeded, to avoid
 	 * any unnecessary toggling of the power wells.
 	 */
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		enum intel_display_power_domain domain;
+		enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
-		if (!crtc->base.state->enable)
+		if (!crtc->state->active)
 			continue;
 
-		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
+		domains = pipe_domains[pipe] = get_crtc_power_domains(crtc);
+		domains &= ~to_intel_crtc(crtc)->enabled_power_domains;
 
-		for_each_power_domain(domain, pipe_domains[crtc->pipe])
+		for_each_power_domain(domain, domains)
 			intel_display_power_get(dev_priv, domain);
 	}
 
-	if (dev_priv->display.modeset_global_resources)
+	if (!power_only && dev_priv->display.modeset_global_resources)
 		dev_priv->display.modeset_global_resources(state);
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		enum pipe pipe = intel_crtc->pipe;
 		enum intel_display_power_domain domain;
 
-		for_each_power_domain(domain, crtc->enabled_power_domains)
-			intel_display_power_put(dev_priv, domain);
+		domains = intel_crtc->enabled_power_domains;
+		domains &= ~pipe_domains[pipe];
 
-		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
-	}
+		intel_crtc->enabled_power_domains = pipe_domains[pipe];
 
-	intel_display_set_init_power(dev_priv, false);
+		for_each_power_domain(domain, domains)
+			intel_display_power_put(dev_priv, domain);
+	}
 }
 
 void broxton_set_cdclk(struct drm_device *dev, int frequency)
@@ -12719,7 +12727,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	/* The state has been swaped above, so state actually contains the
 	 * old state now. */
 
-	modeset_update_crtc_power_domains(state);
+	modeset_update_crtc_power_domains(state, false);
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -15235,14 +15243,16 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		return;
 	}
 
+	/* swap sw/hw state */
 	drm_atomic_helper_swap_state(dev, state);
-
-	/* swap sw/hw dpll state */
 	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
 	intel_shared_dpll_commit(state);
 	memcpy(to_intel_atomic_state(state)->shared_dpll,
 	       shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
 
+	/* update power state to match hw state */
+	modeset_update_crtc_power_domains(state, true);
+
 	/* HW state is read out, now we need to sanitize this mess. */
 	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
@@ -15310,6 +15320,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		drm_atomic_state_free(state);
 	}
 
+	intel_display_set_init_power(dev_priv, false);
+
 	intel_modeset_check_state(dev);
 }
 
-- 
2.1.0

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

[PATCH v2 05/27] drm/i915: add fastboot checks for has_audio and has_infoframe

[Maarten Lankhorst]

The original commit 206645910b97
"drm/i915: check for audio and infoframe changes across mode sets v2"
added checking when has_audio and has_infoframe were changed.

It seems the original commit added both checks for the fastboot case.
By setting crtc_state->mode_changed the code will disable the crtc,
and re-enable it.

This doesn't work on boot, so there we just turn off the crtc.
Later on it should re-enable the crtc anyway, with the correct
settings.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++----------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 64c84fb94b4d..65b7e7406c78 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11888,19 +11888,6 @@ encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-	/* Check if we need to force a modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio) {
-		pipe_config->base.mode_changed = true;
-		ret = drm_atomic_add_affected_planes(state, crtc);
-	}
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
 fail:
 	return ret;
 }
@@ -12667,19 +12654,9 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->enable &&
-		    WARN_ON(crtc_state->active))
-			crtc_state->active = false;
-
-		if (!crtc_state->enable)
+		if (!needs_modeset(crtc_state) || !crtc_state->enable)
 			continue;
 
-		if (!needs_modeset(crtc_state)) {
-			ret = drm_atomic_add_affected_connectors(state, crtc);
-			if (ret)
-				return ret;
-		}
-
 		ret = intel_modeset_pipe_config(crtc,
 					to_intel_crtc_state(crtc_state));
 		if (ret)
@@ -15266,18 +15243,31 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		crtc_state->mode_changed =
 		crtc_state->planes_changed = false;
 
-		if (crtc->enabled) {
+		if (crtc->state->enable) {
 			intel_mode_from_pipe_config(&crtc->state->mode,
 				to_intel_crtc_state(crtc->state));
 
 			drm_mode_copy(&crtc->mode, &crtc->state->mode);
 			drm_mode_copy(&crtc->hwmode,
 				      &crtc->state->adjusted_mode);
+
+			/* Check if we need to force a modeset */
+			if (to_intel_crtc_state(crtc_state)->has_audio !=
+			    to_intel_crtc_state(crtc->state)->has_audio)
+				crtc_state->mode_changed = true;
+
+			if (to_intel_crtc_state(crtc_state)->has_infoframe !=
+			    to_intel_crtc_state(crtc->state)->has_infoframe)
+				crtc_state->mode_changed = true;
 		}
 
 		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
 				    to_intel_crtc_state(crtc_state));
 
+		/* turn CRTC off if a modeset is requested. */
+		if (crtc_state->mode_changed && !force_restore)
+			intel_crtc_control(crtc, false, true);
+
 		/*
 		 * sanitize_crtc may have forced an update of crtc->state,
 		 * so reload in intel_dump_pipe_config
-- 
2.1.0

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

[PATCH v2 06/27] drm/i915: Clean up intel_atomic_setup_scalers slightly.

[Maarten Lankhorst]

Get rid of a whole lot of ternary operators and assign the index
in scaler_id, instead of the id. They're the same thing.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 21 +++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |  2 --
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index e47e00e5b130..63507e8a308a 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -309,15 +309,23 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 	/* walkthrough scaler_users bits and start assigning scalers */
 	for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) {
 		int *scaler_id;
+		const char *name;
+		int idx;
 
 		/* skip if scaler not required */
 		if (!(scaler_state->scaler_users & (1 << i)))
 			continue;
 
 		if (i == SKL_CRTC_INDEX) {
+			name = "CRTC";
+			idx = intel_crtc->base.base.id;
+
 			/* panel fitter case: assign as a crtc scaler */
 			scaler_id = &scaler_state->scaler_id;
 		} else {
+			name = "PLANE";
+			idx = plane->base.id;
+
 			if (!drm_state)
 				continue;
 
@@ -356,23 +364,16 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 			for (j = 0; j < intel_crtc->num_scalers; j++) {
 				if (!scaler_state->scalers[j].in_use) {
 					scaler_state->scalers[j].in_use = 1;
-					*scaler_id = scaler_state->scalers[j].id;
+					*scaler_id = j;
 					DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
-						intel_crtc->pipe,
-						i == SKL_CRTC_INDEX ? scaler_state->scaler_id :
-							plane_state->scaler_id,
-						i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
-						i == SKL_CRTC_INDEX ?  intel_crtc->base.base.id :
-						plane->base.id);
+						intel_crtc->pipe, *scaler_id, name, idx);
 					break;
 				}
 			}
 		}
 
 		if (WARN_ON(*scaler_id < 0)) {
-			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n",
-				i == SKL_CRTC_INDEX ? "CRTC" : "PLANE",
-				i == SKL_CRTC_INDEX ? intel_crtc->base.base.id:plane->base.id);
+			DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
 			continue;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 65b7e7406c78..3da31beee8d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13736,8 +13736,6 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
 	for (i = 0; i < intel_crtc->num_scalers; i++) {
 		intel_scaler = &scaler_state->scalers[i];
 		intel_scaler->in_use = 0;
-		intel_scaler->id = i;
-
 		intel_scaler->mode = PS_SCALER_MODE_DYN;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ec721e31eae8..1d9feb8727ea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -282,7 +282,6 @@ struct intel_initial_plane_config {
 #define SKL_MAX_DST_H 4096
 
 struct intel_scaler {
-	int id;
 	int in_use;
 	uint32_t mode;
 };
-- 
2.1.0

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

[PATCH v2 07/27] drm/i915: Add a simple atomic crtc check function, v2.

[Maarten Lankhorst]

Move the check for encoder cloning here.

Changes since v1:
- Remove was/is crtc_disabled. (mattrope)
- Rename function to intel_crtc_atomic_check. (mattrope)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |   5 +-
 drivers/gpu/drm/i915/intel_display.c | 126 ++++++++++++++++++++---------------
 2 files changed, 75 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 63507e8a308a..f11849c0a8ac 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -100,7 +100,10 @@ int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	/* FIXME: move to crtc atomic check function once it is ready */
+	/*
+	 * FIXME: move to crtc atomic check function once this is
+	 * more atomic friendly.
+	 */
 	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3da31beee8d0..d5a67dbe01bb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11426,11 +11426,82 @@ out_hang:
 	return ret;
 }
 
+static bool encoders_cloneable(const struct intel_encoder *a,
+			       const struct intel_encoder *b)
+{
+	/* masks could be asymmetric, so check both ways */
+	return a == b || (a->cloneable & (1 << b->type) &&
+			  b->cloneable & (1 << a->type));
+}
+
+static bool check_single_encoder_cloning(struct drm_atomic_state *state,
+					 struct intel_crtc *crtc,
+					 struct intel_encoder *encoder)
+{
+	struct intel_encoder *source_encoder;
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != &crtc->base)
+			continue;
+
+		source_encoder =
+			to_intel_encoder(connector_state->best_encoder);
+		if (!encoders_cloneable(encoder, source_encoder))
+			return false;
+	}
+
+	return true;
+}
+
+static bool check_encoder_cloning(struct drm_atomic_state *state,
+				  struct intel_crtc *crtc)
+{
+	struct intel_encoder *encoder;
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		if (connector_state->crtc != &crtc->base)
+			continue;
+
+		encoder = to_intel_encoder(connector_state->best_encoder);
+		if (!check_single_encoder_cloning(state, crtc, encoder))
+			return false;
+	}
+
+	return true;
+}
+
+static int intel_crtc_atomic_check(struct drm_crtc *crtc,
+				   struct drm_crtc_state *crtc_state)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_atomic_state *state = crtc_state->state;
+	int idx = crtc->base.id;
+	bool mode_changed = needs_modeset(crtc_state);
+
+	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
+		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
+		return -EINVAL;
+	}
+
+	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
+		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
+		idx, crtc->state->active, intel_crtc->active);
+
+	return 0;
+}
+
 static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
 	.atomic_begin = intel_begin_crtc_commit,
 	.atomic_flush = intel_finish_crtc_commit,
+	.atomic_check = intel_crtc_atomic_check,
 };
 
 /* Transitional helper to copy current connector/encoder state to
@@ -11656,56 +11727,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	}
 }
 
-static bool encoders_cloneable(const struct intel_encoder *a,
-			       const struct intel_encoder *b)
-{
-	/* masks could be asymmetric, so check both ways */
-	return a == b || (a->cloneable & (1 << b->type) &&
-			  b->cloneable & (1 << a->type));
-}
-
-static bool check_single_encoder_cloning(struct drm_atomic_state *state,
-					 struct intel_crtc *crtc,
-					 struct intel_encoder *encoder)
-{
-	struct intel_encoder *source_encoder;
-	struct drm_connector *connector;
-	struct drm_connector_state *connector_state;
-	int i;
-
-	for_each_connector_in_state(state, connector, connector_state, i) {
-		if (connector_state->crtc != &crtc->base)
-			continue;
-
-		source_encoder =
-			to_intel_encoder(connector_state->best_encoder);
-		if (!encoders_cloneable(encoder, source_encoder))
-			return false;
-	}
-
-	return true;
-}
-
-static bool check_encoder_cloning(struct drm_atomic_state *state,
-				  struct intel_crtc *crtc)
-{
-	struct intel_encoder *encoder;
-	struct drm_connector *connector;
-	struct drm_connector_state *connector_state;
-	int i;
-
-	for_each_connector_in_state(state, connector, connector_state, i) {
-		if (connector_state->crtc != &crtc->base)
-			continue;
-
-		encoder = to_intel_encoder(connector_state->best_encoder);
-		if (!check_single_encoder_cloning(state, crtc, encoder))
-			return false;
-	}
-
-	return true;
-}
-
 static bool check_digital_port_conflicts(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -11792,11 +11813,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 	int i;
 	bool retry = true;
 
-	if (!check_encoder_cloning(state, to_intel_crtc(crtc))) {
-		DRM_DEBUG_KMS("rejecting invalid cloning configuration\n");
-		return -EINVAL;
-	}
-
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =
-- 
2.1.0

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

[PATCH v2 08/27] drm/i915: Move scaler setup to check crtc function, v2.

[Maarten Lankhorst]

The scaler setup may add planes, but since they're unchanged we only
have to wait for primary flips. Also set planes_changed to indicate
at least 1 plane is modified.

Changes since v1:
- Instead of removing planes, do minimal validation needed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 17 +++++++++--------
 drivers/gpu/drm/i915/intel_display.c | 15 +++++----------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index f11849c0a8ac..7ca225194b7c 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -100,14 +100,6 @@ int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	/*
-	 * FIXME: move to crtc atomic check function once this is
-	 * more atomic friendly.
-	 */
-	ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state);
-	if (ret)
-		return ret;
-
 	return ret;
 }
 
@@ -349,6 +341,15 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 						plane->base.id);
 					return PTR_ERR(state);
 				}
+
+				/*
+				 * the plane is added after plane checks are run,
+				 * but since this plane is unchanged just do the
+				 * minimum required validation.
+				 */
+				if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+					intel_crtc->atomic.wait_for_flips = true;
+				crtc_state->base.planes_changed = true;
 			}
 
 			intel_plane = to_intel_plane(plane);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d5a67dbe01bb..41e0f50b3eae 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6552,7 +6552,6 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
-	int ret;
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
@@ -6599,14 +6598,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	if (pipe_config->has_pch_encoder)
 		return ironlake_fdi_compute_config(crtc, pipe_config);
 
-	/* FIXME: remove below call once atomic mode set is place and all crtc
-	 * related checks called from atomic_crtc_check function */
-	ret = 0;
-	DRM_DEBUG_KMS("intel_crtc = %p drm_state (pipe_config->base.state) = %p\n",
-		crtc, pipe_config->base.state);
-	ret = intel_atomic_setup_scalers(dev, crtc, pipe_config);
-
-	return ret;
+	return 0;
 }
 
 static int skylake_get_display_clock_speed(struct drm_device *dev)
@@ -11479,7 +11471,10 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
+	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *pipe_config =
+		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
 	int idx = crtc->base.id;
 	bool mode_changed = needs_modeset(crtc_state);
@@ -11493,7 +11488,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
 		idx, crtc->state->active, intel_crtc->active);
 
-	return 0;
+	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
 }
 
 static const struct drm_crtc_helper_funcs intel_helper_funcs = {
-- 
2.1.0

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

[PATCH v2 09/27] drm/i915: Assign a new pll from the crtc check function.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41e0f50b3eae..5ce78fec2575 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11472,11 +11472,12 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
-	int idx = crtc->base.id;
+	int ret, idx = crtc->base.id;
 	bool mode_changed = needs_modeset(crtc_state);
 
 	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
@@ -11488,6 +11489,15 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
 		idx, crtc->state->active, intel_crtc->active);
 
+	if (mode_changed && crtc_state->enable &&
+	    dev_priv->display.crtc_compute_clock &&
+	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
+		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
+							   pipe_config);
+		if (ret)
+			return ret;
+	}
+
 	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
 }
 
@@ -12505,53 +12515,37 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
-static int intel_modeset_setup_plls(struct drm_atomic_state *state)
+static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	unsigned clear_pipes = 0;
+	struct intel_shared_dpll_config *shared_dpll = NULL;
 	struct intel_crtc *intel_crtc;
 	struct intel_crtc_state *intel_crtc_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	int ret = 0;
 	int i;
 
 	if (!dev_priv->display.crtc_compute_clock)
-		return 0;
+		return;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		int dpll;
+
 		intel_crtc = to_intel_crtc(crtc);
 		intel_crtc_state = to_intel_crtc_state(crtc_state);
+		dpll = intel_crtc_state->shared_dpll;
 
-		if (needs_modeset(crtc_state)) {
-			clear_pipes |= 1 << intel_crtc->pipe;
-			intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
-		}
-	}
-
-	if (clear_pipes) {
-		struct intel_shared_dpll_config *shared_dpll =
-			intel_atomic_get_shared_dpll_state(state);
-
-		for (i = 0; i < dev_priv->num_shared_dpll; i++)
-			shared_dpll[i].crtc_mask &= ~clear_pipes;
-	}
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc_state) || !crtc_state->enable)
+		if (!needs_modeset(crtc_state) || dpll == DPLL_ID_PRIVATE)
 			continue;
 
-		intel_crtc = to_intel_crtc(crtc);
-		intel_crtc_state = to_intel_crtc_state(crtc_state);
+		intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
 
-		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
-							   intel_crtc_state);
-		if (ret)
-			return ret;
-	}
+		if (!shared_dpll)
+			shared_dpll = intel_atomic_get_shared_dpll_state(state);
 
-	return ret;
+		shared_dpll[dpll].crtc_mask &= ~(1 << intel_crtc->pipe);
+	}
 }
 
 /*
@@ -12643,14 +12637,12 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 			return ret;
 	}
 
-	ret = intel_modeset_setup_plls(state);
-	if (ret)
-		return ret;
+	intel_modeset_clear_plls(state);
 
 	if (IS_HASWELL(dev))
-		ret = haswell_mode_set_planes_workaround(state);
+		return haswell_mode_set_planes_workaround(state);
 
-	return ret;
+	return 0;
 }
 
 static int
-- 
2.1.0

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

[PATCH v2 10/27] drm/i915: Do not run most checks when there's no modeset.

[Maarten Lankhorst]

The only reason to check anything without modeset was for fastboot
audio_changed/infoframe_changed changes.

Now that's handled during hw readout parameters shouldn't change
any more when all things stay identical.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ce78fec2575..967398cc03cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12651,13 +12651,18 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int ret, i;
+	bool any_ms = false;
 
 	ret = drm_atomic_helper_check_modeset(state->dev, state);
 	if (ret)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc_state) || !crtc_state->enable)
+		if (!needs_modeset(crtc_state))
+			continue;
+
+		any_ms = true;
+		if (!crtc_state->enable)
 			continue;
 
 		ret = intel_modeset_pipe_config(crtc,
@@ -12670,9 +12675,12 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 				       "[modeset]");
 	}
 
-	ret = intel_modeset_checks(state);
-	if (ret)
-		return ret;
+	if (any_ms) {
+		ret = intel_modeset_checks(state);
+
+		if (ret)
+			return ret;
+	}
 
 	return drm_atomic_helper_check_planes(state->dev, state);
 }
@@ -12685,6 +12693,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;
 	int i;
+	bool any_ms = false;
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
@@ -12693,7 +12702,11 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	drm_atomic_helper_swap_state(dev, state);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc->state) || !crtc_state->active)
+		if (!needs_modeset(crtc->state))
+			continue;
+
+		any_ms = true;
+		if (!crtc_state->active)
 			continue;
 
 		intel_crtc_disable_planes(crtc);
@@ -12706,8 +12719,8 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	/* The state has been swaped above, so state actually contains the
 	 * old state now. */
-
-	modeset_update_crtc_power_domains(state, false);
+	if (any_ms)
+		modeset_update_crtc_power_domains(state, false);
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-- 
2.1.0

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

[PATCH v2 11/27] drm/i915: Split skl_update_scaler, v2.

[Maarten Lankhorst]

It's easier to read separate functions for crtc and plane scaler state.

Changes since v1:
 - Update documentation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 200 ++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_dp.c      |   2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  12 ++-
 drivers/gpu/drm/i915/intel_sprite.c  |   3 +-
 4 files changed, 113 insertions(+), 104 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 967398cc03cb..310b98226d82 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4299,62 +4299,17 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 	}
 }
 
-/**
- * skl_update_scaler_users - Stages update to crtc's scaler state
- * @intel_crtc: crtc
- * @crtc_state: crtc_state
- * @plane: plane (NULL indicates crtc is requesting update)
- * @plane_state: plane's state
- * @force_detach: request unconditional detachment of scaler
- *
- * This function updates scaler state for requested plane or crtc.
- * To request scaler usage update for a plane, caller shall pass plane pointer.
- * To request scaler usage update for crtc, caller shall pass plane pointer
- * as NULL.
- *
- * Return
- *     0 - scaler_usage updated successfully
- *    error - requested scaling cannot be supported or other error condition
- */
-int
-skl_update_scaler_users(
-	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
-	struct intel_plane *intel_plane, struct intel_plane_state *plane_state,
-	int force_detach)
+static int
+skl_update_scaler(const char *name, int i, unsigned idx,
+		  struct intel_crtc_state *crtc_state, bool force_detach,
+		  int *scaler_id, unsigned int rotation,
+		  int src_w, int src_h, int dst_w, int dst_h)
 {
+	struct intel_crtc_scaler_state *scaler_state =
+		&crtc_state->scaler_state;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(crtc_state->base.crtc);
 	int need_scaling;
-	int idx;
-	int src_w, src_h, dst_w, dst_h;
-	int *scaler_id;
-	struct drm_framebuffer *fb;
-	struct intel_crtc_scaler_state *scaler_state;
-	unsigned int rotation;
-
-	if (!intel_crtc || !crtc_state)
-		return 0;
-
-	scaler_state = &crtc_state->scaler_state;
-
-	idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX;
-	fb = intel_plane ? plane_state->base.fb : NULL;
-
-	if (intel_plane) {
-		src_w = drm_rect_width(&plane_state->src) >> 16;
-		src_h = drm_rect_height(&plane_state->src) >> 16;
-		dst_w = drm_rect_width(&plane_state->dst);
-		dst_h = drm_rect_height(&plane_state->dst);
-		scaler_id = &plane_state->scaler_id;
-		rotation = plane_state->base.rotation;
-	} else {
-		struct drm_display_mode *adjusted_mode =
-			&crtc_state->base.adjusted_mode;
-		src_w = crtc_state->pipe_src_w;
-		src_h = crtc_state->pipe_src_h;
-		dst_w = adjusted_mode->hdisplay;
-		dst_h = adjusted_mode->vdisplay;
-		scaler_id = &scaler_state->scaler_id;
-		rotation = DRM_ROTATE_0;
-	}
 
 	need_scaling = intel_rotation_90_or_270(rotation) ?
 		(src_h != dst_w || src_w != dst_h):
@@ -4370,18 +4325,15 @@ skl_update_scaler_users(
 	 * update to free the scaler is done in plane/panel-fit programming.
 	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
 	 */
-	if (force_detach || !need_scaling || (intel_plane &&
-		(!fb || !plane_state->visible))) {
+	if (force_detach || !need_scaling) {
 		if (*scaler_id >= 0) {
 			scaler_state->scaler_users &= ~(1 << idx);
 			scaler_state->scalers[*scaler_id].in_use = 0;
 
 			DRM_DEBUG_KMS("Staged freeing scaler id %d.%d from %s:%d "
 				"crtc_state = %p scaler_users = 0x%x\n",
-				intel_crtc->pipe, *scaler_id, intel_plane ? "PLANE" : "CRTC",
-				intel_plane ? intel_plane->base.base.id :
-				intel_crtc->base.base.id, crtc_state,
-				scaler_state->scaler_users);
+				intel_crtc->pipe, *scaler_id, name, i,
+				crtc_state, scaler_state->scaler_users);
 			*scaler_id = -1;
 		}
 		return 0;
@@ -4395,49 +4347,104 @@ skl_update_scaler_users(
 		dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H) {
 		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u "
 			"size is out of scaler range\n",
-			intel_plane ? "PLANE" : "CRTC",
-			intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
-			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
+			name, i, intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
 		return -EINVAL;
 	}
 
+	/* mark this plane as a scaler user in crtc_state */
+	scaler_state->scaler_users |= (1 << idx);
+	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
+		"crtc_state = %p scaler_users = 0x%x\n",
+		name, i, src_w, src_h, dst_w, dst_h,
+		crtc_state, scaler_state->scaler_users);
+
+	return 0;
+}
+
+/**
+ * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
+ *
+ * @state: crtc's scaler state
+ * @force_detach: whether to forcibly disable scaler
+ *
+ * Return
+ *     0 - scaler_usage updated successfully
+ *    error - requested scaling cannot be supported or other error condition
+ */
+int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
+{
+	struct drm_crtc *crtc = state->base.crtc;
+	struct drm_display_mode *adjusted_mode =
+		&state->base.adjusted_mode;
+
+	return skl_update_scaler("CRTC", crtc->base.id,
+		SKL_CRTC_INDEX, state, force_detach,
+		&state->scaler_state.scaler_id, DRM_ROTATE_0,
+		state->pipe_src_w, state->pipe_src_h,
+		adjusted_mode->hdisplay, adjusted_mode->hdisplay);
+}
+
+/**
+ * skl_update_scaler_plane - Stages update to scaler state for a given plane.
+ *
+ * @state: crtc's scaler state
+ * @intel_plane: affected plane
+ * @plane_state: atomic plane state to update
+ *
+ * Return
+ *     0 - scaler_usage updated successfully
+ *    error - requested scaling cannot be supported or other error condition
+ */
+int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
+			    struct intel_plane *intel_plane,
+			    struct intel_plane_state *plane_state)
+{
+
+	struct drm_framebuffer *fb = plane_state->base.fb;
+	int ret;
+
+	bool force_detach = !fb || !plane_state->visible;
+
+	ret = skl_update_scaler("PLANE", intel_plane->base.base.id,
+				drm_plane_index(&intel_plane->base),
+				crtc_state, force_detach,
+				&plane_state->scaler_id,
+				plane_state->base.rotation,
+				drm_rect_width(&plane_state->src) >> 16,
+				drm_rect_height(&plane_state->src) >> 16,
+				drm_rect_width(&plane_state->dst),
+				drm_rect_height(&plane_state->dst));
+
+	if (ret || plane_state->scaler_id < 0)
+		return ret;
+
 	/* check colorkey */
-	if (WARN_ON(intel_plane &&
-		intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
-		DRM_DEBUG_KMS("PLANE:%d scaling %ux%u->%ux%u not allowed with colorkey",
-			intel_plane->base.base.id, src_w, src_h, dst_w, dst_h);
+	if (WARN_ON(intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
+		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
+			intel_plane->base.base.id);
 		return -EINVAL;
 	}
 
 	/* Check src format */
-	if (intel_plane) {
-		switch (fb->pixel_format) {
-		case DRM_FORMAT_RGB565:
-		case DRM_FORMAT_XBGR8888:
-		case DRM_FORMAT_XRGB8888:
-		case DRM_FORMAT_ABGR8888:
-		case DRM_FORMAT_ARGB8888:
-		case DRM_FORMAT_XRGB2101010:
-		case DRM_FORMAT_XBGR2101010:
-		case DRM_FORMAT_YUYV:
-		case DRM_FORMAT_YVYU:
-		case DRM_FORMAT_UYVY:
-		case DRM_FORMAT_VYUY:
-			break;
-		default:
-			DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
-				intel_plane->base.base.id, fb->base.id, fb->pixel_format);
-			return -EINVAL;
-		}
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_RGB565:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ABGR8888:
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
+		break;
+	default:
+		DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
+			intel_plane->base.base.id, fb->base.id, fb->pixel_format);
+		return -EINVAL;
 	}
 
-	/* mark this plane as a scaler user in crtc_state */
-	scaler_state->scaler_users |= (1 << idx);
-	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
-		"crtc_state = %p scaler_users = 0x%x\n",
-		intel_plane ? "PLANE" : "CRTC",
-		intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
-		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users);
 	return 0;
 }
 
@@ -4452,7 +4459,7 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
 	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
 
 	/* To update pfit, first update scaler state */
-	skl_update_scaler_users(crtc, crtc->config, NULL, NULL, !enable);
+	skl_update_scaler_crtc(crtc->config, !enable);
 	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
 	skl_detach_scalers(crtc);
 	if (!enable)
@@ -13339,8 +13346,9 @@ intel_check_primary_plane(struct drm_plane *plane,
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_users(intel_crtc, crtc_state,
-			to_intel_plane(plane), state, 0);
+		ret = skl_update_scaler_plane(crtc_state,
+					      to_intel_plane(plane),
+					      state);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 15e88922a984..6bfb3eb62e9f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1376,7 +1376,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 		if (INTEL_INFO(dev)->gen >= 9) {
 			int ret;
-			ret = skl_update_scaler_users(intel_crtc, pipe_config, NULL, NULL, 0);
+			ret = skl_update_scaler_crtc(pipe_config, 0);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d9feb8727ea..e116a5f6346e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -252,7 +252,7 @@ struct intel_plane_state {
 	 * plane requiring a scaler:
 	 *   - During check_plane, its bit is set in
 	 *     crtc_state->scaler_state.scaler_users by calling helper function
-	 *     update_scaler_users.
+	 *     update_scaler_plane.
 	 *   - scaler_id indicates the scaler it got assigned.
 	 *
 	 * plane doesn't require a scaler:
@@ -260,7 +260,7 @@ struct intel_plane_state {
 	 *     got disabled.
 	 *   - During check_plane, corresponding bit is reset in
 	 *     crtc_state->scaler_state.scaler_users by calling helper function
-	 *     update_scaler_users.
+	 *     update_scaler_plane.
 	 */
 	int scaler_id;
 };
@@ -1136,9 +1136,11 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 void skl_detach_scalers(struct intel_crtc *intel_crtc);
-int skl_update_scaler_users(struct intel_crtc *intel_crtc,
-	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
-	struct intel_plane_state *plane_state, int force_detach);
+int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
+			    struct intel_plane *intel_plane,
+			    struct intel_plane_state *plane_state);
+
+int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
 
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8193a35388d7..fe95f25f019a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -941,8 +941,7 @@ finish:
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
-			state, 0);
+		ret = skl_update_scaler_plane(crtc_state, intel_plane, state);
 		if (ret)
 			return ret;
 	}
-- 
2.1.0

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

[PATCH v2 12/27] drm/i915: Split plane updates of crtc->atomic into a helper, v2.

[Maarten Lankhorst]

This makes it easier to verify that no changes are done when
calling this from crtc instead.

Changes since v1:
 - Make intel_wm_need_update static and always check it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  21 +--
 drivers/gpu/drm/i915/intel_display.c      | 275 ++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h          |   8 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  32 +---
 4 files changed, 176 insertions(+), 160 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 86ba4b2c3a65..aa2128369a0a 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -114,6 +114,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	struct intel_crtc_state *crtc_state;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
@@ -160,20 +161,6 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	intel_state->clip.y2 =
 		crtc_state->base.active ? crtc_state->pipe_src_h : 0;
 
-	/*
-	 * Disabling a plane is always okay; we just need to update
-	 * fb tracking in a special way since cleanup_fb() won't
-	 * get called by the plane helpers.
-	 */
-	if (state->fb == NULL && plane->state->fb != NULL) {
-		/*
-		 * 'prepare' is never called when plane is being disabled, so
-		 * we need to handle frontbuffer tracking as a special case
-		 */
-		intel_crtc->atomic.disabled_planes |=
-			(1 << drm_plane_index(plane));
-	}
-
 	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
 		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
@@ -198,7 +185,11 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		}
 	}
 
-	return intel_plane->check_plane(plane, intel_state);
+	ret = intel_plane->check_plane(plane, intel_state);
+	if (ret || !state->state)
+		return ret;
+
+	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
 }
 
 static void intel_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 310b98226d82..517a1bd302d5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4388,18 +4388,18 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
  * skl_update_scaler_plane - Stages update to scaler state for a given plane.
  *
  * @state: crtc's scaler state
- * @intel_plane: affected plane
  * @plane_state: atomic plane state to update
  *
  * Return
  *     0 - scaler_usage updated successfully
  *    error - requested scaling cannot be supported or other error condition
  */
-int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
-			    struct intel_plane *intel_plane,
-			    struct intel_plane_state *plane_state)
+static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
+				   struct intel_plane_state *plane_state)
 {
 
+	struct intel_plane *intel_plane =
+		to_intel_plane(plane_state->base.plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	int ret;
 
@@ -11425,6 +11425,161 @@ out_hang:
 	return ret;
 }
 
+
+/**
+ * intel_wm_need_update - Check whether watermarks need updating
+ * @plane: drm plane
+ * @state: new plane state
+ *
+ * Check current plane state versus the new one to determine whether
+ * watermarks need to be recalculated.
+ *
+ * Returns true or false.
+ */
+static bool intel_wm_need_update(struct drm_plane *plane,
+				 struct drm_plane_state *state)
+{
+	/* Update watermarks on tiling changes. */
+	if (!plane->state->fb || !state->fb ||
+	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
+	    plane->state->rotation != state->rotation)
+		return true;
+
+	if (plane->state->crtc_w != state->crtc_w)
+		return true;
+
+	return false;
+}
+
+int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
+				    struct drm_plane_state *plane_state)
+{
+	struct drm_crtc *crtc = crtc_state->crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_plane *plane = plane_state->plane;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	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;
+
+	bool turn_off, turn_on, visible, was_visible;
+	struct drm_framebuffer *fb = plane_state->fb;
+
+	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
+	    plane->type != DRM_PLANE_TYPE_CURSOR) {
+		ret = skl_update_scaler_plane(
+			to_intel_crtc_state(crtc_state),
+			to_intel_plane_state(plane_state));
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Disabling a plane is always okay; we just need to update
+	 * fb tracking in a special way since cleanup_fb() won't
+	 * get called by the plane helpers.
+	 */
+	if (old_plane_state->base.fb && !fb)
+		intel_crtc->atomic.disabled_planes |= 1 << i;
+
+	/* don't run rest during modeset yet */
+	if (!intel_crtc->active || mode_changed)
+		return 0;
+
+	was_visible = old_plane_state->visible;
+	visible = to_intel_plane_state(plane_state)->visible;
+
+	if (!was_crtc_enabled && WARN_ON(was_visible))
+		was_visible = false;
+
+	if (!is_crtc_enabled && WARN_ON(visible))
+		visible = false;
+
+	if (!was_visible && !visible)
+		return 0;
+
+	turn_off = was_visible && (!visible || mode_changed);
+	turn_on = visible && (!was_visible || mode_changed);
+
+	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
+			 plane->base.id, fb ? fb->base.id : -1);
+
+	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
+			 plane->base.id, was_visible, visible,
+			 turn_off, turn_on, mode_changed);
+
+	if (intel_wm_need_update(plane, plane_state))
+		intel_crtc->atomic.update_wm = true;
+
+	switch (plane->type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		if (visible)
+			intel_crtc->atomic.fb_bits |=
+			    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+
+		intel_crtc->atomic.wait_for_flips = true;
+		intel_crtc->atomic.pre_disable_primary = turn_off;
+		intel_crtc->atomic.post_enable_primary = turn_on;
+
+		if (turn_off)
+			intel_crtc->atomic.disable_fbc = true;
+
+		/*
+		 * FBC does not work on some platforms for rotated
+		 * planes, so disable it when rotation is not 0 and
+		 * update it when rotation is set back to 0.
+		 *
+		 * FIXME: This is redundant with the fbc update done in
+		 * the primary plane enable function except that that
+		 * one is done too late. We eventually need to unify
+		 * this.
+		 */
+
+		if (visible &&
+		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
+		    dev_priv->fbc.crtc == intel_crtc &&
+		    plane_state->rotation != BIT(DRM_ROTATE_0))
+			intel_crtc->atomic.disable_fbc = true;
+
+		/*
+		 * BDW signals flip done immediately if the plane
+		 * is disabled, even if the plane enable is already
+		 * armed to occur at the next vblank :(
+		 */
+		if (turn_on && IS_BROADWELL(dev))
+			intel_crtc->atomic.wait_vblank = true;
+
+		intel_crtc->atomic.update_fbc |= visible || mode_changed;
+		break;
+	case DRM_PLANE_TYPE_CURSOR:
+		if (visible)
+			intel_crtc->atomic.fb_bits |=
+			    INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+		break;
+	case DRM_PLANE_TYPE_OVERLAY:
+		/*
+		 * 'prepare' is never called when plane is being disabled, so
+		 * we need to handle frontbuffer tracking as a special case
+		 */
+		if (visible)
+			intel_crtc->atomic.fb_bits |=
+			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
+
+		if (turn_off && is_crtc_enabled) {
+			intel_crtc->atomic.wait_vblank = true;
+			intel_crtc->atomic.update_sprite_watermarks |=
+				1 << i;
+		}
+		break;
+	}
+	return 0;
+}
+
 static bool encoders_cloneable(const struct intel_encoder *a,
 			       const struct intel_encoder *b)
 {
@@ -13118,28 +13273,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
 }
 
 /**
- * intel_wm_need_update - Check whether watermarks need updating
- * @plane: drm plane
- * @state: new plane state
- *
- * Check current plane state versus the new one to determine whether
- * watermarks need to be recalculated.
- *
- * Returns true or false.
- */
-bool intel_wm_need_update(struct drm_plane *plane,
-			  struct drm_plane_state *state)
-{
-	/* Update watermarks on tiling changes. */
-	if (!plane->state->fb || !state->fb ||
-	    plane->state->fb->modifier[0] != state->fb->modifier[0] ||
-	    plane->state->rotation != state->rotation)
-		return true;
-
-	return false;
-}
-
-/**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
  * @fb: framebuffer to prepare for presentation
@@ -13260,7 +13393,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = state->base.crtc;
 	struct intel_crtc *intel_crtc;
 	struct intel_crtc_state *crtc_state;
@@ -13271,7 +13403,6 @@ intel_check_primary_plane(struct drm_plane *plane,
 	bool can_position = false;
 	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
-	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
@@ -13287,73 +13418,11 @@ intel_check_primary_plane(struct drm_plane *plane,
 		can_position = true;
 	}
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    src, dest, clip,
-					    min_scale,
-					    max_scale,
-					    can_position, true,
-					    &state->visible);
-	if (ret)
-		return ret;
-
-	if (intel_crtc->active) {
-		struct intel_plane_state *old_state =
-			to_intel_plane_state(plane->state);
-
-		intel_crtc->atomic.wait_for_flips = true;
-
-		/*
-		 * FBC does not work on some platforms for rotated
-		 * planes, so disable it when rotation is not 0 and
-		 * update it when rotation is set back to 0.
-		 *
-		 * FIXME: This is redundant with the fbc update done in
-		 * the primary plane enable function except that that
-		 * one is done too late. We eventually need to unify
-		 * this.
-		 */
-		if (state->visible &&
-		    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-		    dev_priv->fbc.crtc == intel_crtc &&
-		    state->base.rotation != BIT(DRM_ROTATE_0)) {
-			intel_crtc->atomic.disable_fbc = true;
-		}
-
-		if (state->visible && !old_state->visible) {
-			/*
-			 * BDW signals flip done immediately if the plane
-			 * is disabled, even if the plane enable is already
-			 * armed to occur at the next vblank :(
-			 */
-			if (IS_BROADWELL(dev))
-				intel_crtc->atomic.wait_vblank = true;
-
-			if (crtc_state && !needs_modeset(&crtc_state->base))
-				intel_crtc->atomic.post_enable_primary = true;
-		}
-
-		if (!state->visible && old_state->visible &&
-		    crtc_state && !needs_modeset(&crtc_state->base))
-			intel_crtc->atomic.pre_disable_primary = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
-
-		intel_crtc->atomic.update_fbc = true;
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-	}
-
-	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_plane(crtc_state,
-					      to_intel_plane(plane),
-					      state);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     src, dest, clip,
+					     min_scale, max_scale,
+					     can_position, true,
+					     &state->visible);
 }
 
 static void
@@ -13613,10 +13682,9 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-
 	/* if we want to turn off the cursor ignore width and height */
 	if (!obj)
-		goto finish;
+		return 0;
 
 	/* Check for which cursor types we support */
 	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
@@ -13633,19 +13701,10 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
-		ret = -EINVAL;
-	}
-
-finish:
-	if (intel_crtc->active) {
-		if (plane->state->crtc_w != state->base.crtc_w)
-			intel_crtc->atomic.update_wm = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e116a5f6346e..802f3e99c849 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1055,6 +1055,8 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
 				    struct drm_plane_state *state,
 				    struct drm_property *property,
 				    uint64_t val);
+int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
+				    struct drm_plane_state *plane_state);
 
 unsigned int
 intel_tile_height(struct drm_device *dev, uint32_t pixel_format,
@@ -1069,9 +1071,6 @@ intel_rotation_90_or_270(unsigned int rotation)
 void intel_create_rotation_property(struct drm_device *dev,
 					struct intel_plane *plane);
 
-bool intel_wm_need_update(struct drm_plane *plane,
-			  struct drm_plane_state *state);
-
 /* shared dpll functions */
 struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
@@ -1136,9 +1135,6 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 void skl_detach_scalers(struct intel_crtc *intel_crtc);
-int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
-			    struct intel_plane *intel_plane,
-			    struct intel_plane_state *plane_state);
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fe95f25f019a..f5921b652b90 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -756,7 +756,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	int max_scale, min_scale;
 	bool can_scale;
 	int pixel_size;
-	int ret;
 
 	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
 	crtc_state = state->base.state ?
@@ -764,7 +763,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 	if (!fb) {
 		state->visible = false;
-		goto finish;
+		return 0;
 	}
 
 	/* Don't modify another pipe's plane */
@@ -917,35 +916,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	dst->y1 = crtc_y;
 	dst->y2 = crtc_y + crtc_h;
 
-finish:
-	/*
-	 * If the sprite is completely covering the primary plane,
-	 * we can disable the primary and save power.
-	 */
-	if (intel_crtc->active) {
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
-
-		if (intel_wm_need_update(plane, &state->base))
-			intel_crtc->atomic.update_wm = true;
-
-		if (!state->visible) {
-			/*
-			 * Avoid underruns when disabling the sprite.
-			 * FIXME remove once watermark updates are done properly.
-			 */
-			intel_crtc->atomic.wait_vblank = true;
-			intel_crtc->atomic.update_sprite_watermarks |=
-				(1 << drm_plane_index(plane));
-		}
-	}
-
-	if (INTEL_INFO(dev)->gen >= 9) {
-		ret = skl_update_scaler_plane(crtc_state, intel_plane, state);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
-- 
2.1.0

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

[PATCH v2 13/27] drm/i915: clean up plane commit functions

[Maarten Lankhorst]

No point in hiding behind big ifs. This will be true most of the time.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_sprite.c  | 33 ++++++++++++++-------------------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 517a1bd302d5..857c00881eff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13443,14 +13443,14 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	if (intel_crtc->active) {
-		if (state->visible)
-			/* FIXME: kill this fastboot hack */
-			intel_update_pipe_size(intel_crtc);
+	if (!intel_crtc->active)
+		return;
 
-		dev_priv->display.update_primary_plane(crtc, plane->fb,
-						       crtc->x, crtc->y);
-	}
+	if (state->visible)
+		/* FIXME: kill this fastboot hack */
+		intel_update_pipe_size(intel_crtc);
+
+	dev_priv->display.update_primary_plane(crtc, fb, crtc->x, crtc->y);
 }
 
 static void
@@ -13752,8 +13752,8 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 
 	intel_crtc->cursor_addr = addr;
 	intel_crtc->cursor_bo = obj;
-update:
 
+update:
 	if (intel_crtc->active)
 		intel_crtc_update_cursor(crtc, state->visible);
 }
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f5921b652b90..c909b8b8ce85 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -927,31 +927,26 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
-	int crtc_x, crtc_y;
-	unsigned int crtc_w, crtc_h;
-	uint32_t src_x, src_y, src_w, src_h;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = fb;
 
-	if (intel_crtc->active) {
-		if (state->visible) {
-			crtc_x = state->dst.x1;
-			crtc_y = state->dst.y1;
-			crtc_w = drm_rect_width(&state->dst);
-			crtc_h = drm_rect_height(&state->dst);
-			src_x = state->src.x1 >> 16;
-			src_y = state->src.y1 >> 16;
-			src_w = drm_rect_width(&state->src) >> 16;
-			src_h = drm_rect_height(&state->src) >> 16;
-			intel_plane->update_plane(plane, crtc, fb,
-						  crtc_x, crtc_y, crtc_w, crtc_h,
-						  src_x, src_y, src_w, src_h);
-		} else {
-			intel_plane->disable_plane(plane, crtc, false);
-		}
+	if (!intel_crtc->active)
+		return;
+
+	if (state->visible) {
+		intel_plane->update_plane(plane, crtc, fb,
+					  state->dst.x1, state->dst.y1,
+					  drm_rect_width(&state->dst),
+					  drm_rect_height(&state->dst),
+					  state->src.x1 >> 16,
+					  state->src.y1 >> 16,
+					  drm_rect_width(&state->src) >> 16,
+					  drm_rect_height(&state->src) >> 16);
+	} else {
+		intel_plane->disable_plane(plane, crtc, false);
 	}
 }
 
-- 
2.1.0

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

[PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Maarten Lankhorst]

By passing crtc_state to the check_plane functions a lot of duplicated
code can be removed. And now that the transitional helpers are gone the
crtc_state can be reliably obtained.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
 drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h          |  1 +
 drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
 4 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index aa2128369a0a..4d8cacbca777 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	intel_state->visible = false;
+
 	/*
 	 * Both crtc and plane->crtc could be NULL if we're updating a
 	 * property while the plane is disabled.  We don't actually have
@@ -185,7 +187,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 		}
 	}
 
-	ret = intel_plane->check_plane(plane, intel_state);
+	ret = intel_plane->check_plane(plane, crtc_state, intel_state);
 	if (ret || !state->state)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 857c00881eff..8841cf5e8f1b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13390,36 +13390,25 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 
 static int
 intel_check_primary_plane(struct drm_plane *plane,
+			  struct intel_crtc_state *crtc_state,
 			  struct intel_plane_state *state)
 {
-	struct drm_device *dev = plane->dev;
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
 	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_rect *dest = &state->dst;
-	struct drm_rect *src = &state->src;
-	const struct drm_rect *clip = &state->clip;
-	bool can_position = false;
-	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
 	int min_scale = DRM_PLANE_HELPER_NO_SCALING;
+	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
+	bool can_position = false;
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
-
-	if (INTEL_INFO(dev)->gen >= 9) {
-		/* use scaler when colorkey is not required */
-		if (to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
-			min_scale = 1;
-			max_scale = skl_max_scale(intel_crtc, crtc_state);
-		}
+	/* use scaler when colorkey is not required */
+	if (INTEL_INFO(plane->dev)->gen >= 9 &&
+	    to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
+		min_scale = 1;
+		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
 		can_position = true;
 	}
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
-					     src, dest, clip,
+	return drm_plane_helper_check_update(plane, crtc, fb, &state->src,
+					     &state->dst, &state->clip,
 					     min_scale, max_scale,
 					     can_position, true,
 					     &state->visible);
@@ -13658,24 +13647,17 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *
 
 static int
 intel_check_cursor_plane(struct drm_plane *plane,
+			 struct intel_crtc_state *crtc_state,
 			 struct intel_plane_state *state)
 {
-	struct drm_crtc *crtc = state->base.crtc;
-	struct drm_device *dev = plane->dev;
+	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct drm_framebuffer *fb = state->base.fb;
-	struct drm_rect *dest = &state->dst;
-	struct drm_rect *src = &state->src;
-	const struct drm_rect *clip = &state->clip;
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct intel_crtc *intel_crtc;
 	unsigned stride;
 	int ret;
 
-	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
-
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    src, dest, clip,
+	ret = drm_plane_helper_check_update(plane, crtc, fb, &state->src,
+					    &state->dst, &state->clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    true, true, &state->visible);
@@ -13687,7 +13669,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		return 0;
 
 	/* Check for which cursor types we support */
-	if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) {
+	if (!cursor_size_ok(plane->dev, state->base.crtc_w, state->base.crtc_h)) {
 		DRM_DEBUG("Cursor dimension %dx%d not supported\n",
 			  state->base.crtc_w, state->base.crtc_h);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 802f3e99c849..96e365e74b04 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -601,6 +601,7 @@ struct intel_plane {
 	void (*disable_plane)(struct drm_plane *plane,
 			      struct drm_crtc *crtc, bool force);
 	int (*check_plane)(struct drm_plane *plane,
+			   struct intel_crtc_state *crtc_state,
 			   struct intel_plane_state *state);
 	void (*commit_plane)(struct drm_plane *plane,
 			     struct intel_plane_state *state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c909b8b8ce85..c15d54b60105 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -739,11 +739,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc, bool force)
 
 static int
 intel_check_sprite_plane(struct drm_plane *plane,
+			 struct intel_crtc_state *crtc_state,
 			 struct intel_plane_state *state)
 {
 	struct drm_device *dev = plane->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc *crtc = state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 	int crtc_x, crtc_y;
@@ -757,14 +758,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	bool can_scale;
 	int pixel_size;
 
-	intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
-	crtc_state = state->base.state ?
-		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
-
-	if (!fb) {
-		state->visible = false;
+	if (!fb)
 		return 0;
-	}
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
-- 
2.1.0

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

[PATCH v2 15/27] drm/i915: remove force argument from disable_plane

[Maarten Lankhorst]

The idea was good, but planes can have a fb even though
they're disabled. This makes the force argument useless
and always true, because only the commit function updates
state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 10 +++++-----
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8841cf5e8f1b..f5f074524603 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4784,7 +4784,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 			struct drm_crtc *from = intel_plane->base.crtc;
 
 			intel_plane->disable_plane(&intel_plane->base,
-						   from ?: crtc, true);
+						   from ?: crtc);
 		}
 	}
 
@@ -13444,8 +13444,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 
 static void
 intel_disable_primary_plane(struct drm_plane *plane,
-			    struct drm_crtc *crtc,
-			    bool force)
+			    struct drm_crtc *crtc)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -13691,17 +13690,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
 
 static void
 intel_disable_cursor_plane(struct drm_plane *plane,
-			   struct drm_crtc *crtc,
-			   bool force)
+			   struct drm_crtc *crtc)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	if (!force) {
-		plane->fb = NULL;
-		intel_crtc->cursor_bo = NULL;
-		intel_crtc->cursor_addr = 0;
-	}
-
 	intel_crtc_update_cursor(crtc, false);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 96e365e74b04..0dae94631094 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -599,7 +599,7 @@ struct intel_plane {
 			     uint32_t x, uint32_t y,
 			     uint32_t src_w, uint32_t src_h);
 	void (*disable_plane)(struct drm_plane *plane,
-			      struct drm_crtc *crtc, bool force);
+			      struct drm_crtc *crtc);
 	int (*check_plane)(struct drm_plane *plane,
 			   struct intel_crtc_state *crtc_state,
 			   struct intel_plane_state *state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c15d54b60105..2a6cfac210cb 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -272,7 +272,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 }
 
 static void
-skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
+skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -455,7 +455,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 }
 
 static void
-vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc, bool force)
+vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -595,7 +595,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 static void
-ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc, bool force)
+ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -722,7 +722,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 static void
-ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc, bool force)
+ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -941,7 +941,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 					  drm_rect_width(&state->src) >> 16,
 					  drm_rect_height(&state->src) >> 16);
 	} else {
-		intel_plane->disable_plane(plane, crtc, false);
+		intel_plane->disable_plane(plane, crtc);
 	}
 }
 
-- 
2.1.0

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

[PATCH v2 16/27] drm/i915: move detaching scalers to begin_crtc_commit, v2.

[Maarten Lankhorst]

This is probably intended to be be done during vblank evasion.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 3 ---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 drivers/gpu/drm/i915/intel_drv.h     | 1 -
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 7ca225194b7c..8447a1fef332 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -144,9 +144,6 @@ int intel_atomic_commit(struct drm_device *dev,
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
 
-		if (INTEL_INFO(dev)->gen >= 9)
-			skl_detach_scalers(to_intel_crtc(crtc));
-
 		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f5f074524603..2ba3d6abbd0c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2911,16 +2911,13 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 /*
  * This function detaches (aka. unbinds) unused scalers in hardware
  */
-void skl_detach_scalers(struct intel_crtc *intel_crtc)
+static void skl_detach_scalers(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev;
 	struct drm_i915_private *dev_priv;
 	struct intel_crtc_scaler_state *scaler_state;
 	int i;
 
-	if (!intel_crtc || !intel_crtc->config)
-		return;
-
 	dev = intel_crtc->base.dev;
 	dev_priv = dev->dev_private;
 	scaler_state = &intel_crtc->config->scaler_state;
@@ -13505,6 +13502,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
+
+	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
+		skl_detach_scalers(intel_crtc);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0dae94631094..d96c9a24b94b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1135,7 +1135,6 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
-void skl_detach_scalers(struct intel_crtc *intel_crtc);
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
-- 
2.1.0

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

[PATCH v2 17/27] drm/i915: Move crtc commit updates to separate functions.

[Maarten Lankhorst]

To allow them to be used in intel_set_mode.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 127 +++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ba3d6abbd0c..b72724121f57 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4744,6 +4744,72 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	hsw_disable_ips(intel_crtc);
 }
 
+static void intel_post_plane_update(struct intel_crtc *crtc)
+{
+	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_plane *plane;
+
+	if (atomic->wait_vblank)
+		intel_wait_for_vblank(dev, crtc->pipe);
+
+	intel_frontbuffer_flip(dev, atomic->fb_bits);
+
+	if (atomic->update_fbc) {
+		mutex_lock(&dev->struct_mutex);
+		intel_fbc_update(dev);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	if (atomic->post_enable_primary)
+		intel_post_enable_primary(&crtc->base);
+
+	drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
+		intel_update_sprite_watermarks(plane, &crtc->base,
+					       0, 0, 0, false, false);
+
+	memset(atomic, 0, sizeof(*atomic));
+}
+
+static void intel_pre_plane_update(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+	struct drm_plane *p;
+
+	/* Track fb's for any planes being disabled */
+
+	drm_for_each_plane_mask(p, dev, atomic->disabled_planes) {
+		struct intel_plane *plane = to_intel_plane(p);
+		unsigned fb_bits = 0;
+
+		switch (p->type) {
+		case DRM_PLANE_TYPE_PRIMARY:
+			fb_bits = INTEL_FRONTBUFFER_PRIMARY(plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_CURSOR:
+			fb_bits = INTEL_FRONTBUFFER_CURSOR(plane->pipe);
+			break;
+		case DRM_PLANE_TYPE_OVERLAY:
+			fb_bits = INTEL_FRONTBUFFER_SPRITE(plane->pipe);
+			break;
+		}
+
+		mutex_lock(&dev->struct_mutex);
+		i915_gem_track_fb(intel_fb_obj(plane->base.fb), NULL, fb_bits);
+		mutex_unlock(&dev->struct_mutex);
+	}
+
+	if (atomic->wait_for_flips)
+		intel_crtc_wait_for_pending_flips(&crtc->base);
+
+	if (atomic->disable_fbc)
+		intel_fbc_disable(dev);
+
+	if (atomic->pre_disable_primary)
+		intel_pre_disable_primary(&crtc->base);
+}
+
 static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -13454,43 +13520,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_crtc_state *crtc_state = intel_crtc->base.state;
-	struct intel_plane *intel_plane;
-	struct drm_plane *p;
-	unsigned fb_bits = 0;
-
-	/* Track fb's for any planes being disabled */
-	list_for_each_entry(p, &dev->mode_config.plane_list, head) {
-		intel_plane = to_intel_plane(p);
-
-		if (intel_crtc->atomic.disabled_planes &
-		    (1 << drm_plane_index(p))) {
-			switch (p->type) {
-			case DRM_PLANE_TYPE_PRIMARY:
-				fb_bits = INTEL_FRONTBUFFER_PRIMARY(intel_plane->pipe);
-				break;
-			case DRM_PLANE_TYPE_CURSOR:
-				fb_bits = INTEL_FRONTBUFFER_CURSOR(intel_plane->pipe);
-				break;
-			case DRM_PLANE_TYPE_OVERLAY:
-				fb_bits = INTEL_FRONTBUFFER_SPRITE(intel_plane->pipe);
-				break;
-			}
-
-			mutex_lock(&dev->struct_mutex);
-			i915_gem_track_fb(intel_fb_obj(p->fb), NULL, fb_bits);
-			mutex_unlock(&dev->struct_mutex);
-		}
-	}
-
-	if (intel_crtc->atomic.wait_for_flips)
-		intel_crtc_wait_for_pending_flips(crtc);
-
-	if (intel_crtc->atomic.disable_fbc)
-		intel_fbc_disable(dev);
 
-	if (intel_crtc->atomic.pre_disable_primary)
-		intel_pre_disable_primary(crtc);
+	intel_pre_plane_update(intel_crtc);
 
 	if (intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
@@ -13498,7 +13529,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (crtc_state->active && !needs_modeset(crtc_state))
+	if (crtc->state->active && !needs_modeset(crtc->state))
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
@@ -13512,7 +13543,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_plane *p;
 
 	if (intel_crtc->atomic.evade)
 		intel_pipe_update_end(intel_crtc,
@@ -13520,26 +13550,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 
 	intel_runtime_pm_put(dev_priv);
 
-	if (intel_crtc->atomic.wait_vblank && intel_crtc->active)
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-
-	intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
-
-	if (intel_crtc->atomic.update_fbc) {
-		mutex_lock(&dev->struct_mutex);
-		intel_fbc_update(dev);
-		mutex_unlock(&dev->struct_mutex);
-	}
-
-	if (intel_crtc->atomic.post_enable_primary)
-		intel_post_enable_primary(crtc);
-
-	drm_for_each_legacy_plane(p, &dev->mode_config.plane_list)
-		if (intel_crtc->atomic.update_sprite_watermarks & drm_plane_index(p))
-			intel_update_sprite_watermarks(p, crtc, 0, 0, 0,
-						       false, false);
-
-	memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
+	intel_post_plane_update(intel_crtc);
 }
 
 /**
-- 
2.1.0

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

[PATCH v2 18/27] drm/i915: Handle disabling planes better.

[Maarten Lankhorst]

Read out the initial state, and add a quirk to force disable all plane
that were initially active. Use this to disable planes during modeset
too.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |   7 ++
 drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   7 ++
 3 files changed, 114 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 8447a1fef332..5627df2807b0 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
 		return -EINVAL;
 	}
 
+	if (crtc_state &&
+	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
+		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
+		if (ret)
+			return ret;
+	}
+
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b72724121f57..3b5d23692935 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
 }
 
+static void intel_disable_planes_on_crtc(struct drm_crtc *c)
+{
+	struct intel_crtc *crtc = to_intel_crtc(c);
+	struct drm_plane *plane;
+
+	drm_for_each_plane_mask(plane, crtc->base.dev,
+				crtc->atomic.force_disabled_planes)
+		to_intel_plane(plane)->disable_plane(plane, c);
+}
+
 static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
 	int pipe = intel_crtc->pipe;
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_pre_disable_primary(crtc);
 
 	intel_crtc_dpms_overlay_disable(intel_crtc);
-	for_each_intel_plane(dev, intel_plane) {
-		if (intel_plane->pipe == pipe) {
-			struct drm_crtc *from = intel_plane->base.crtc;
-
-			intel_plane->disable_plane(&intel_plane->base,
-						   from ?: crtc);
-		}
-	}
+	intel_disable_planes_on_crtc(crtc);
 
 	/*
 	 * FIXME: Once we grow proper nuclear flip support out of this we need
@@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (!intel_crtc->active || mode_changed)
 		return 0;
 
+	if (to_intel_crtc_state(crtc_state)->quirks &
+	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
+		if (!plane_state->crtc) {
+			intel_crtc->atomic.force_disabled_planes |=
+				1 << drm_plane_index(plane);
+			crtc_state->plane_mask &=
+				~(1 << drm_plane_index(plane));
+		}
+
+		/* Initial state for sprite planes is unknown,
+		 * no need to update sprite watermarks */
+		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
+			mode_changed = true;
+	}
+
 	was_visible = old_plane_state->visible;
 	visible = to_intel_plane_state(plane_state)->visible;
 
@@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 			intel_crtc->atomic.fb_bits |=
 			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
 
-		if (turn_off && is_crtc_enabled) {
+		if (turn_off && !mode_changed) {
 			intel_crtc->atomic.wait_vblank = true;
 			intel_crtc->atomic.update_sprite_watermarks |=
 				1 << i;
@@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
 		idx, crtc->state->active, intel_crtc->active);
 
+	/* plane mask is fixed up after all initial planes are calculated */
+	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
+	if (mode_changed)
+		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
+
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
 	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
@@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (to_intel_crtc_state(crtc_state)->quirks &
+		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
+			ret = drm_atomic_add_affected_planes(state, crtc);
+			if (ret)
+				return ret;
+		}
+
 		if (!needs_modeset(crtc_state))
 			continue;
 
@@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (crtc->state->active && !needs_modeset(crtc->state))
+	if (crtc->state->active && !needs_modeset(crtc->state)) {
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
 
+		if (intel_crtc->atomic.force_disabled_planes)
+			intel_disable_planes_on_crtc(crtc);
+	}
+
 	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
-		skl_detach_scalers(intel_crtc);
+                skl_detach_scalers(intel_crtc);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
 	i915_redisable_vga_power_on(dev);
 }
 
-static bool primary_get_hw_state(struct intel_crtc *crtc)
+static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
+				      struct intel_plane *intel_plane)
 {
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!crtc->base.enabled)
-		return false;
+	switch (intel_plane->base.type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
+
+	case DRM_PLANE_TYPE_CURSOR:
+		if (IS_845G(dev) || IS_I865G(dev))
+			return I915_READ(_CURACNTR) & CURSOR_ENABLE;
+		else
+			return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
+
+	default:
+		return true;
+	}
+}
+
+static int readout_plane_state(struct drm_atomic_state *state,
+			       struct intel_crtc *crtc,
+			       struct intel_crtc_state *crtc_state)
+{
+	struct intel_plane *p;
+	struct drm_plane_state *drm_plane_state;
+	bool active = crtc_state->base.active;
+
+	if (active) {
+		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
 
-	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
+		/* apply to previous sw state too */
+		to_intel_crtc_state(crtc->base.state)->quirks |=
+			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
+	}
+
+	for_each_intel_plane(state->dev, p) {
+		if (crtc->plane != p->plane)
+			continue;
+
+		drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
+		if (IS_ERR(drm_plane_state))
+			return PTR_ERR(drm_plane_state);
+
+		if (active && intel_read_hw_plane_state(crtc, p)) {
+			to_intel_plane_state(drm_plane_state)->visible = true;
+			crtc_state->base.plane_mask |= 1 << drm_plane_index(&p->base);
+			p->base.crtc = &crtc->base;
+		} else {
+			to_intel_plane_state(drm_plane_state)->visible = false;
+			crtc_state->base.plane_mask &= ~(1 << drm_plane_index(&p->base));
+			p->base.crtc = drm_plane_state->crtc;
+		}
+	}
+
+	return 0;
 }
 
 static int readout_hw_crtc_state(struct drm_atomic_state *state,
@@ -15055,19 +15137,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_crtc_state *crtc_state;
-	struct drm_plane *primary = crtc->base.primary;
-	struct drm_plane_state *drm_plane_state;
-	struct intel_plane_state *plane_state;
-	int ret;
 
 	crtc_state = intel_atomic_get_crtc_state(state, crtc);
 	if (IS_ERR(crtc_state))
 		return PTR_ERR(crtc_state);
 
-	ret = drm_atomic_add_affected_planes(state, &crtc->base);
-	if (ret)
-		return ret;
-
 	memset(crtc_state, 0, sizeof(*crtc_state));
 	crtc_state->base.crtc = &crtc->base;
 	crtc_state->base.state = state;
@@ -15081,24 +15155,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
 	crtc->active = crtc_state->base.active;
 	crtc->config = crtc_state;
 
-	drm_plane_state = drm_atomic_get_plane_state(state, primary);
-	if (IS_ERR(drm_plane_state))
-		return PTR_ERR(drm_plane_state);
-
-	plane_state = to_intel_plane_state(drm_plane_state);
-	plane_state->visible = primary_get_hw_state(crtc);
-
-	if (plane_state->visible) {
-		primary->crtc = &crtc->base;
-		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
-	} else
-		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
-
 	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 		      crtc->base.base.id,
 		      crtc_state->base.active ? "enabled" : "disabled");
 
-	return 0;
+	return readout_plane_state(state, crtc, crtc_state);
 }
 
 static int readout_hw_pll_state(struct drm_atomic_state *state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d96c9a24b94b..7b1a13f63872 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -327,6 +327,7 @@ struct intel_crtc_state {
 	 */
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
+#define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
 	unsigned long quirks;
 
 	/* Pipe source size (ie. panel fitter input size)
@@ -485,8 +486,14 @@ struct intel_crtc_atomic_commit {
 	bool disable_fbc;
 	bool pre_disable_primary;
 	bool update_wm;
+
+	/* planes that will be disabled in a normal way */
 	unsigned disabled_planes;
 
+	/* planes that will be disabled during modeset,
+	 * or initially enabled planes */
+	unsigned force_disabled_planes;
+
 	/* Sleepable operations to perform after commit */
 	unsigned fb_bits;
 	bool wait_vblank;
-- 
2.1.0

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

[PATCH v2 19/27] drm/i915: atomic plane updates in a nutshell

[Maarten Lankhorst]

Now that all planes are added during a modeset we can use the
calculated changes before disabling a plane, and then either commit
or force disable a plane before disabling the crtc.

The code is shared with atomic_begin/flush, except watermark updating
and vblank evasion are not used.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 128 +++++++----------------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   4 +-
 2 files changed, 27 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b5d23692935..d4ddcad7eaf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,8 +109,6 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
 	struct intel_crtc_state *crtc_state);
 static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
 			   int num_connectors);
-static void intel_crtc_enable_planes(struct drm_crtc *crtc);
-static void intel_crtc_disable_planes(struct drm_crtc *crtc);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -2215,28 +2213,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc)
 		intel_wait_for_pipe_off(crtc);
 }
 
-/**
- * intel_enable_primary_hw_plane - enable the primary plane on a given pipe
- * @plane:  plane to be enabled
- * @crtc: crtc for the plane
- *
- * Enable @plane on @crtc, making sure that the pipe is running first.
- */
-static void intel_enable_primary_hw_plane(struct drm_plane *plane,
-					  struct drm_crtc *crtc)
-{
-	struct drm_device *dev = plane->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* If the pipe isn't enabled, we can't pump pixels and may hang */
-	assert_pipe_enabled(dev_priv, intel_crtc->pipe);
-	to_intel_plane_state(plane->state)->visible = true;
-
-	dev_priv->display.update_primary_plane(crtc, plane->fb,
-					       crtc->x, crtc->y);
-}
-
 static bool need_vtd_wa(struct drm_device *dev)
 {
 #ifdef CONFIG_INTEL_IOMMU
@@ -4501,20 +4477,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc)
 	}
 }
 
-static void intel_enable_sprite_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	enum pipe pipe = to_intel_crtc(crtc)->pipe;
-	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
-
-	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		intel_plane = to_intel_plane(plane);
-		if (intel_plane->pipe == pipe)
-			intel_plane_restore(&intel_plane->base);
-	}
-}
-
 void hsw_enable_ips(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -4810,26 +4772,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 		intel_pre_disable_primary(&crtc->base);
 }
 
-static void intel_crtc_enable_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
-
-	intel_enable_primary_hw_plane(crtc->primary, crtc);
-	intel_enable_sprite_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	intel_post_enable_primary(crtc);
-
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
-}
-
 static void intel_disable_planes_on_crtc(struct drm_crtc *c)
 {
 	struct intel_crtc *crtc = to_intel_crtc(c);
@@ -4840,27 +4782,6 @@ static void intel_disable_planes_on_crtc(struct drm_crtc *c)
 		to_intel_plane(plane)->disable_plane(plane, c);
 }
 
-static void intel_crtc_disable_planes(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
-
-	intel_crtc_wait_for_pending_flips(crtc);
-
-	intel_pre_disable_primary(crtc);
-
-	intel_crtc_dpms_overlay_disable(intel_crtc);
-	intel_disable_planes_on_crtc(crtc);
-
-	/*
-	 * FIXME: Once we grow proper nuclear flip support out of this we need
-	 * to compute the mask of flip planes precisely. For the time being
-	 * consider this a flip to a NULL plane.
-	 */
-	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
-}
-
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -11552,10 +11473,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	if (old_plane_state->base.fb && !fb)
 		intel_crtc->atomic.disabled_planes |= 1 << i;
 
-	/* don't run rest during modeset yet */
-	if (!intel_crtc->active || mode_changed)
-		return 0;
-
 	if (to_intel_crtc_state(crtc_state)->quirks &
 	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
 		if (!plane_state->crtc) {
@@ -12956,15 +12873,23 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	drm_atomic_helper_swap_state(dev, state);
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
 		if (!needs_modeset(crtc->state))
 			continue;
 
 		any_ms = true;
-		if (!crtc_state->active)
-			continue;
+		intel_pre_plane_update(intel_crtc);
+
+		if (crtc_state->active) {
+			intel_crtc_dpms_overlay_disable(intel_crtc);
+			intel_disable_planes_on_crtc(crtc);
+
+			/* XXX: Calculate mask here? */
+			intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(intel_crtc->pipe));
 
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+			dev_priv->display.crtc_disable(crtc);
+		}
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -12978,15 +12903,12 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
-
-		if (!needs_modeset(crtc->state) || !crtc->state->active)
-			continue;
-
-		update_scanline_offset(to_intel_crtc(crtc));
+		if (needs_modeset(crtc->state) && crtc->state->active) {
+			update_scanline_offset(to_intel_crtc(crtc));
+			dev_priv->display.crtc_enable(crtc);
+		}
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
+		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 	}
 
 	/* FIXME: add subpixel order */
@@ -13524,7 +13446,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 	crtc->x = src->x1 >> 16;
 	crtc->y = src->y1 >> 16;
 
-	if (!intel_crtc->active)
+	if (!crtc->state->active)
 		return;
 
 	if (state->visible)
@@ -13550,7 +13472,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	intel_pre_plane_update(intel_crtc);
+	if (!needs_modeset(crtc->state))
+		intel_pre_plane_update(intel_crtc);
 
 	if (intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
@@ -13558,17 +13481,18 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (crtc->state->active && !needs_modeset(crtc->state)) {
+	if (crtc->state->active)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
 
+	if (!needs_modeset(crtc->state)) {
+		if (INTEL_INFO(dev)->gen >= 9)
+			skl_detach_scalers(intel_crtc);
+
 		if (intel_crtc->atomic.force_disabled_planes)
 			intel_disable_planes_on_crtc(crtc);
 	}
-
-	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
-                skl_detach_scalers(intel_crtc);
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
@@ -13770,7 +13694,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_bo = obj;
 
 update:
-	if (intel_crtc->active)
+	if (crtc->state->active)
 		intel_crtc_update_cursor(crtc, state->visible);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2a6cfac210cb..49d867a8b073 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -919,16 +919,14 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 			  struct intel_plane_state *state)
 {
 	struct drm_crtc *crtc = state->base.crtc;
-	struct intel_crtc *intel_crtc;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *fb = state->base.fb;
 
 	crtc = crtc ? crtc : plane->crtc;
-	intel_crtc = to_intel_crtc(crtc);
 
 	plane->fb = fb;
 
-	if (!intel_crtc->active)
+	if (!crtc->state->active)
 		return;
 
 	if (state->visible) {
-- 
2.1.0

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

[PATCH v2 20/27] drm/i915: Update less state during modeset.

[Maarten Lankhorst]

No need to repeatedly call update_watermarks, or update_fbc.
Down to a single call to update_watermarks in .crtc_enable

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 65 +++++++++---------------------------
 1 file changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d4ddcad7eaf1..fcd7e6783901 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1945,10 +1945,10 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 
 	/* PCH only available on ILK+ */
 	BUG_ON(INTEL_INFO(dev)->gen < 5);
-	if (WARN_ON(pll == NULL))
+	if (pll == NULL)
 	       return;
 
-	if (WARN_ON(pll->config.crtc_mask == 0))
+	if (WARN_ON(!(pll->config.crtc_mask & (1 << drm_crtc_index(&crtc->base)))))
 		return;
 
 	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
@@ -4634,10 +4634,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
 	 */
 	hsw_enable_ips(intel_crtc);
 
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * Gen2 reports pipe underruns whenever all planes are disabled.
 	 * So don't enable underrun reporting before at least some planes
@@ -4692,11 +4688,6 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		intel_set_memory_cxsr(dev_priv, false);
 
-	mutex_lock(&dev->struct_mutex);
-	if (dev_priv->fbc.crtc == intel_crtc)
-		intel_fbc_disable(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	/*
 	 * FIXME IPS should be fine as long as one plane is
 	 * enabled, but in practice it seems to have problems
@@ -4736,6 +4727,7 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
 static void intel_pre_plane_update(struct intel_crtc *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 drm_plane *p;
 
@@ -4765,8 +4757,13 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 	if (atomic->wait_for_flips)
 		intel_crtc_wait_for_pending_flips(&crtc->base);
 
-	if (atomic->disable_fbc)
-		intel_fbc_disable(dev);
+	if (atomic->disable_fbc &&
+	    dev_priv->fbc.crtc == crtc) {
+		mutex_lock(&dev->struct_mutex);
+		if (dev_priv->fbc.crtc == crtc)
+			intel_fbc_disable(dev);
+		mutex_unlock(&dev->struct_mutex);
+	}
 
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
@@ -4973,9 +4970,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (WARN_ON(!intel_crtc->active))
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
@@ -5014,18 +5008,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 			I915_WRITE(PCH_DPLL_SEL, temp);
 		}
 
-		/* disable PCH DPLL */
-		intel_disable_shared_dpll(intel_crtc);
-
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static void haswell_crtc_disable(struct drm_crtc *crtc)
@@ -5036,9 +5020,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-	if (WARN_ON(!intel_crtc->active))
-		return;
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
@@ -5074,16 +5055,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
-
-	if (intel_crtc_to_shared_dpll(intel_crtc))
-		intel_disable_shared_dpll(intel_crtc);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
@@ -6089,9 +6060,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (WARN_ON(!intel_crtc->active))
-		return;
-
 	/*
 	 * On gen2 planes are double buffered but the pipe isn't, so we must
 	 * wait for planes to fully turn off before disabling the pipe.
@@ -6125,13 +6093,6 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
-
-	intel_crtc->active = false;
-	intel_update_watermarks(crtc);
-
-	mutex_lock(&dev->struct_mutex);
-	intel_fbc_update(dev);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 /*
@@ -11650,8 +11611,10 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 
 	/* plane mask is fixed up after all initial planes are calculated */
 	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-	if (mode_changed)
+	if (mode_changed) {
 		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
+		intel_crtc->atomic.update_wm = !crtc_state->active;
+	}
 
 	if (mode_changed && crtc_state->enable &&
 	    dev_priv->display.crtc_compute_clock &&
@@ -12889,6 +12852,8 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 			intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(intel_crtc->pipe));
 
 			dev_priv->display.crtc_disable(crtc);
+			intel_crtc->active = false;
+			intel_disable_shared_dpll(intel_crtc);
 		}
 	}
 
-- 
2.1.0

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

[PATCH v2 21/27] drm/i915: get rid of intel_plane_restore in intel_crtc_page_flip

[Maarten Lankhorst]

Use a full atomic call instead. intel_crtc_page_flip will still
have to live until async updates are allowed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fcd7e6783901..b228db21e9c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11361,8 +11361,32 @@ free_work:
 	kfree(work);
 
 	if (ret == -EIO) {
+		struct drm_atomic_state *state;
+		struct drm_plane_state *plane_state;
+
 out_hang:
-		ret = intel_plane_restore(primary);
+		state = drm_atomic_state_alloc(dev);
+		if (!state)
+			return -ENOMEM;
+		state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+
+		plane_state = drm_atomic_get_plane_state(state, primary);
+		ret = PTR_ERR_OR_ZERO(plane_state);
+		if (!ret) {
+			drm_atomic_set_fb_for_plane(plane_state, fb);
+
+			ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
+			if (!ret)
+				ret = drm_atomic_commit(state);
+		}
+
+		/* shouldn't happen because of legacy_acquire_ctx */
+		if (WARN_ON(ret == -EDEADLK))
+			ret = -EIO;
+
+		if (ret)
+			drm_atomic_state_free(state);
+
 		if (ret == 0 && event) {
 			spin_lock_irq(&dev->event_lock);
 			drm_send_vblank_event(dev, pipe, event);
-- 
2.1.0

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

[PATCH v2 22/27] drm/i915: Make setting color key atomic.

[Maarten Lankhorst]

By making color key atomic there are no more transitional helpers.
The plane check function will reject the color key when a scaler is
active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c |  1 +
 drivers/gpu/drm/i915/intel_display.c      |  5 +-
 drivers/gpu/drm/i915/intel_drv.h          |  6 +--
 drivers/gpu/drm/i915/intel_sprite.c       | 85 +++++++++++++++----------------
 4 files changed, 45 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 4d8cacbca777..9af1053ae8aa 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -56,6 +56,7 @@ intel_create_plane_state(struct drm_plane *plane)
 
 	state->base.plane = plane;
 	state->base.rotation = BIT(DRM_ROTATE_0);
+	state->ckey.flags = I915_SET_COLORKEY_NONE;
 
 	return state;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b228db21e9c0..d2a1bf4f31dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4392,7 +4392,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 		return ret;
 
 	/* check colorkey */
-	if (WARN_ON(intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
+	if (plane_state->ckey.flags != I915_SET_COLORKEY_NONE) {
 		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
 			intel_plane->base.base.id);
 		return -EINVAL;
@@ -13404,7 +13404,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 	/* use scaler when colorkey is not required */
 	if (INTEL_INFO(plane->dev)->gen >= 9 &&
-	    to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
+	    state->ckey.flags == I915_SET_COLORKEY_NONE) {
 		min_scale = 1;
 		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
 		can_position = true;
@@ -13555,7 +13555,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	primary->check_plane = intel_check_primary_plane;
 	primary->commit_plane = intel_commit_primary_plane;
 	primary->disable_plane = intel_disable_primary_plane;
-	primary->ckey.flags = I915_SET_COLORKEY_NONE;
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b1a13f63872..506a7e320fbd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -263,6 +263,8 @@ struct intel_plane_state {
 	 *     update_scaler_plane.
 	 */
 	int scaler_id;
+
+	struct drm_intel_sprite_colorkey ckey;
 };
 
 struct intel_initial_plane_config {
@@ -582,9 +584,6 @@ struct intel_plane {
 	bool can_scale;
 	int max_downscale;
 
-	/* FIXME convert to properties */
-	struct drm_intel_sprite_colorkey ckey;
-
 	/* Since we need to change the watermarks before/after
 	 * enabling/disabling the planes, we need to store the parameters here
 	 * as the other pieces of the struct may not reflect the values we want
@@ -1385,7 +1384,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 
 /* intel_sprite.c */
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
-int intel_plane_restore(struct drm_plane *plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 bool intel_pipe_update_start(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 49d867a8b073..994d9ac9078c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -182,7 +182,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl, stride_div, stride;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
+	const struct drm_intel_sprite_colorkey *key =
+		&to_intel_plane_state(drm_plane->state)->ckey;
 	unsigned long surf_addr;
 	u32 tile_height, plane_offset, plane_size;
 	unsigned int rotation;
@@ -344,7 +345,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
+	const struct drm_intel_sprite_colorkey *key =
+		&to_intel_plane_state(dplane->state)->ckey;
 
 	sprctl = SP_ENABLE;
 
@@ -487,7 +489,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
+	const struct drm_intel_sprite_colorkey *key =
+		&to_intel_plane_state(plane->state)->ckey;
 
 	sprctl = SPRITE_ENABLE;
 
@@ -627,7 +630,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
-	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
+	const struct drm_intel_sprite_colorkey *key =
+		&to_intel_plane_state(plane->state)->ckey;
 
 	dvscntr = DVS_ENABLE;
 
@@ -776,7 +780,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	/* setup can_scale, min_scale, max_scale */
 	if (INTEL_INFO(dev)->gen >= 9) {
 		/* use scaler when colorkey is not required */
-		if (intel_plane->ckey.flags == I915_SET_COLORKEY_NONE) {
+		if (state->ckey.flags == I915_SET_COLORKEY_NONE) {
 			can_scale = 1;
 			min_scale = 1;
 			max_scale = skl_max_scale(intel_crtc, crtc_state);
@@ -796,7 +800,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	 * coordinates and sizes. We probably need some way to decide whether
 	 * more strict checking should be done instead.
 	 */
-
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
 			state->base.rotation);
 
@@ -806,7 +809,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
 	BUG_ON(vscale < 0);
 
-	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+	state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
 
 	crtc_x = dst->x1;
 	crtc_y = dst->y1;
@@ -948,7 +951,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 {
 	struct drm_intel_sprite_colorkey *set = data;
 	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
+	struct drm_plane_state *plane_state;
+	struct drm_atomic_state *state;
+	struct drm_modeset_acquire_ctx ctx;
 	int ret = 0;
 
 	/* Make sure we don't try to enable both src & dest simultaneously */
@@ -959,50 +964,41 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 	    set->flags & I915_SET_COLORKEY_DESTINATION)
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
-
 	plane = drm_plane_find(dev, set->plane_id);
-	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY) {
-		ret = -ENOENT;
-		goto out_unlock;
-	}
+	if (!plane || plane->type != DRM_PLANE_TYPE_OVERLAY)
+		return -ENOENT;
 
-	intel_plane = to_intel_plane(plane);
+	drm_modeset_acquire_init(&ctx, 0);
 
-	if (INTEL_INFO(dev)->gen >= 9) {
-		/* plane scaling and colorkey are mutually exclusive */
-		if (to_intel_plane_state(plane->state)->scaler_id >= 0) {
-			DRM_ERROR("colorkey not allowed with scaler\n");
-			ret = -EINVAL;
-			goto out_unlock;
-		}
+	state = drm_atomic_state_alloc(plane->dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
+	state->acquire_ctx = &ctx;
+
+	while (1) {
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		ret = PTR_ERR_OR_ZERO(plane_state);
+		if (!ret) {
+			to_intel_plane_state(plane_state)->ckey = *set;
+			ret = drm_atomic_commit(state);
+		}
 
-	intel_plane->ckey = *set;
-
-	/*
-	 * The only way this could fail would be due to
-	 * the current plane state being unsupportable already,
-	 * and we dont't consider that an error for the
-	 * colorkey ioctl. So just ignore any error.
-	 */
-	intel_plane_restore(plane);
+		if (ret != -EDEADLK)
+			break;
 
-out_unlock:
-	drm_modeset_unlock_all(dev);
-	return ret;
-}
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(&ctx);
+	}
 
-int intel_plane_restore(struct drm_plane *plane)
-{
-	if (!plane->crtc || !plane->state->fb)
-		return 0;
+	if (ret)
+		drm_atomic_state_free(state);
 
-	return drm_plane_helper_update(plane, plane->crtc, plane->state->fb,
-				       plane->state->crtc_x, plane->state->crtc_y,
-				       plane->state->crtc_w, plane->state->crtc_h,
-				       plane->state->src_x, plane->state->src_y,
-				       plane->state->src_w, plane->state->src_h);
+out:
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	return ret;
 }
 
 static const uint32_t ilk_plane_formats[] = {
@@ -1131,7 +1127,6 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->plane = plane;
 	intel_plane->check_plane = intel_check_sprite_plane;
 	intel_plane->commit_plane = intel_commit_sprite_plane;
-	intel_plane->ckey.flags = I915_SET_COLORKEY_NONE;
 	possible_crtcs = (1 << pipe);
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
-- 
2.1.0

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

[PATCH v2 23/27] drm/i915: Remove transitional references from intel_plane_atomic_check.

[Maarten Lankhorst]

All transitional plane helpers are gone, party!

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 9af1053ae8aa..6d5ede8a9bf6 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -131,16 +131,16 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	if (!crtc)
 		return 0;
 
-	/* FIXME: temporary hack necessary while we still use the plane update
-	 * helper. */
-	if (state->state) {
-		crtc_state =
-			intel_atomic_get_crtc_state(state->state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
-	} else {
-		crtc_state = intel_crtc->config;
-	}
+	/*
+	 * Normally we could use get_existing_crtc_state, but the initial hw
+	 * readout path only sets plane->crtc, which is not picked up by the
+	 * atomic core. It's forbidden to set state->crtc without a valid
+	 * state->fb, so getting crtc state here is the best we can do..
+	 */
+	crtc_state = intel_atomic_get_crtc_state(state->state, intel_crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+	crtc_state->base.planes_changed = true;
 
 	/*
 	 * The original src/dest coordinates are stored in state->base, but
@@ -189,7 +189,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	}
 
 	ret = intel_plane->check_plane(plane, crtc_state, intel_state);
-	if (ret || !state->state)
+	if (ret)
 		return ret;
 
 	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
-- 
2.1.0

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

[PATCH v2 24/27] drm/i915: Use full atomic modeset.

[Maarten Lankhorst]

Huzzah! \o/

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 128 ----------------
 drivers/gpu/drm/i915/intel_display.c | 280 ++++++-----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |   5 -
 3 files changed, 42 insertions(+), 371 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 5627df2807b0..90c5f93d9b13 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -35,134 +35,6 @@
 #include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 
-
-/**
- * intel_atomic_check - validate state object
- * @dev: drm device
- * @state: state to validate
- */
-int intel_atomic_check(struct drm_device *dev,
-		       struct drm_atomic_state *state)
-{
-	int nplanes = dev->mode_config.num_total_plane;
-	int ncrtcs = dev->mode_config.num_crtc;
-	int nconnectors = dev->mode_config.num_connector;
-	enum pipe nuclear_pipe = INVALID_PIPE;
-	struct intel_crtc *nuclear_crtc = NULL;
-	struct intel_crtc_state *crtc_state = NULL;
-	int ret;
-	int i;
-	bool not_nuclear = false;
-
-	/*
-	 * FIXME:  At the moment, we only support "nuclear pageflip" on a
-	 * single CRTC.  Cross-crtc updates will be added later.
-	 */
-	for (i = 0; i < nplanes; i++) {
-		struct intel_plane *plane = to_intel_plane(state->planes[i]);
-		if (!plane)
-			continue;
-
-		if (nuclear_pipe == INVALID_PIPE) {
-			nuclear_pipe = plane->pipe;
-		} else if (nuclear_pipe != plane->pipe) {
-			DRM_DEBUG_KMS("i915 only support atomic plane operations on a single CRTC at the moment\n");
-			return -EINVAL;
-		}
-	}
-
-	/*
-	 * FIXME:  We only handle planes for now; make sure there are no CRTC's
-	 * or connectors involved.
-	 */
-	state->allow_modeset = false;
-	for (i = 0; i < ncrtcs; i++) {
-		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
-		if (crtc)
-			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
-		if (crtc && crtc->pipe != nuclear_pipe)
-			not_nuclear = true;
-		if (crtc && crtc->pipe == nuclear_pipe) {
-			nuclear_crtc = crtc;
-			crtc_state = to_intel_crtc_state(state->crtc_states[i]);
-		}
-	}
-	for (i = 0; i < nconnectors; i++)
-		if (state->connectors[i] != NULL)
-			not_nuclear = true;
-
-	if (not_nuclear) {
-		DRM_DEBUG_KMS("i915 only supports atomic plane operations at the moment\n");
-		return -EINVAL;
-	}
-
-	if (crtc_state &&
-	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
-		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
-		if (ret)
-			return ret;
-	}
-
-	ret = drm_atomic_helper_check_planes(dev, state);
-	if (ret)
-		return ret;
-
-	return ret;
-}
-
-
-/**
- * intel_atomic_commit - commit validated state object
- * @dev: DRM device
- * @state: the top-level driver state object
- * @async: asynchronous commit
- *
- * This function commits a top-level state object that has been validated
- * with drm_atomic_helper_check().
- *
- * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
- * we can only handle plane-related operations and do not yet support
- * asynchronous commit.
- *
- * RETURNS
- * Zero for success or -errno.
- */
-int intel_atomic_commit(struct drm_device *dev,
-			struct drm_atomic_state *state,
-			bool async)
-{
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc *crtc;
-	int ret;
-	int i;
-
-	if (async) {
-		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
-		return -EINVAL;
-	}
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	/* Point of no return */
-	drm_atomic_helper_swap_state(dev, state);
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
-
-		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
-	}
-
-	/* FIXME: This function should eventually call __intel_set_mode when needed */
-
-	drm_atomic_helper_wait_for_vblanks(dev, state);
-	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
-
-	return 0;
-}
-
 /**
  * intel_connector_atomic_get_property - fetch connector property value
  * @connector: connector to fetch property for
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d2a1bf4f31dc..d5e783c170a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -86,7 +86,6 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config);
 
-static int intel_set_mode(struct drm_atomic_state *state);
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -110,14 +109,6 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
 static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
 			   int num_connectors);
 
-static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
-{
-	if (!connector->mst_port)
-		return connector->encoder;
-	else
-		return &connector->mst_port->mst_encoders[pipe]->base;
-}
-
 typedef struct {
 	int	min, max;
 } intel_range_t;
@@ -6135,7 +6126,7 @@ int intel_display_suspend(struct drm_device *dev)
 	}
 
 	if (crtc_mask) {
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 
 		if (!ret) {
 			for_each_crtc(dev, crtc)
@@ -6215,7 +6206,7 @@ int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force)
 		}
 	}
 
-	ret = intel_set_mode(state);
+	ret = drm_atomic_commit(state);
 	if (!ret)
 		return ret;
 
@@ -10175,7 +10166,7 @@ retry:
 
 	drm_mode_copy(&crtc_state->base.mode, mode);
 
-	if (intel_set_mode(state)) {
+	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -10243,7 +10234,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (ret)
 			goto fail;
 
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 		if (ret)
 			goto fail;
 
@@ -12764,7 +12755,6 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
 	return 0;
 }
 
-/* Code that should eventually be part of atomic_check() */
 static int intel_modeset_checks(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -12796,15 +12786,20 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int
-intel_modeset_compute_config(struct drm_atomic_state *state)
+/**
+ * intel_atomic_check - validate state object
+ * @dev: drm device
+ * @state: state to validate
+ */
+static int intel_atomic_check(struct drm_device *dev,
+			      struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int ret, i;
 	bool any_ms = false;
 
-	ret = drm_atomic_helper_check_modeset(state->dev, state);
+	ret = drm_atomic_helper_check_modeset(dev, state);
 	if (ret)
 		return ret;
 
@@ -12827,7 +12822,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 					to_intel_crtc_state(crtc_state));
 		if (ret)
 			return ret;
-
 		intel_dump_pipe_config(to_intel_crtc(crtc),
 				       to_intel_crtc_state(crtc_state),
 				       "[modeset]");
@@ -12843,9 +12837,26 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	return drm_atomic_helper_check_planes(state->dev, state);
 }
 
-static int __intel_set_mode(struct drm_atomic_state *state)
+/**
+ * intel_atomic_commit - commit validated state object
+ * @dev: DRM device
+ * @state: the top-level driver state object
+ * @async: asynchronous commit
+ *
+ * This function commits a top-level state object that has been validated
+ * with drm_atomic_helper_check().
+ *
+ * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
+ * we can only handle plane-related operations and do not yet support
+ * asynchronous commit.
+ *
+ * RETURNS
+ * Zero for success or -errno.
+ */
+static int intel_atomic_commit(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       bool async)
 {
-	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -12853,6 +12864,11 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	int i;
 	bool any_ms = false;
 
+	if (async) {
+		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
+		return -EINVAL;
+	}
+
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
 		return ret;
@@ -12902,34 +12918,14 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	/* FIXME: add subpixel order */
 
+	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-
 	drm_atomic_state_free(state);
 
-	return 0;
-}
-
-static int intel_set_mode_checked(struct drm_atomic_state *state)
-{
-	struct drm_device *dev = state->dev;
-	int ret;
-
-	ret = __intel_set_mode(state);
-	if (ret == 0)
+	if (any_ms)
 		intel_modeset_check_state(dev);
 
-	return ret;
-}
-
-static int intel_set_mode(struct drm_atomic_state *state)
-{
-	int ret;
-
-	ret = intel_modeset_compute_config(state);
-	if (ret)
-		return ret;
-
-	return intel_set_mode_checked(state);
+	return 0;
 }
 
 void intel_crtc_restore_mode(struct drm_crtc *crtc)
@@ -12956,11 +12952,10 @@ retry:
 			goto out;
 
 		crtc_state->mode_changed = true;
-		ret = intel_modeset_compute_config(state);
 	}
 
 	if (!ret)
-		ret = intel_set_mode_checked(state);
+		ret = drm_atomic_commit(state);
 
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
@@ -12975,200 +12970,9 @@ out:
 
 #undef for_each_intel_crtc_masked
 
-static bool intel_connector_in_mode_set(struct intel_connector *connector,
-					struct drm_mode_set *set)
-{
-	int ro;
-
-	for (ro = 0; ro < set->num_connectors; ro++)
-		if (set->connectors[ro] == &connector->base)
-			return true;
-
-	return false;
-}
-
-static int
-intel_modeset_stage_output_state(struct drm_device *dev,
-				 struct drm_mode_set *set,
-				 struct drm_atomic_state *state)
-{
-	struct intel_connector *connector;
-	struct drm_connector *drm_connector;
-	struct drm_connector_state *connector_state;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int i, ret;
-
-	/* The upper layers ensure that we either disable a crtc or have a list
-	 * of connectors. For paranoia, double-check this. */
-	WARN_ON(!set->fb && (set->num_connectors != 0));
-	WARN_ON(set->fb && (set->num_connectors == 0));
-
-	for_each_intel_connector(dev, connector) {
-		bool in_mode_set = intel_connector_in_mode_set(connector, set);
-
-		if (!in_mode_set && connector->base.state->crtc != set->crtc)
-			continue;
-
-		connector_state =
-			drm_atomic_get_connector_state(state, &connector->base);
-		if (IS_ERR(connector_state))
-			return PTR_ERR(connector_state);
-
-		if (in_mode_set) {
-			int pipe = to_intel_crtc(set->crtc)->pipe;
-			connector_state->best_encoder =
-				&intel_find_encoder(connector, pipe)->base;
-		}
-
-		if (connector->base.state->crtc != set->crtc)
-			continue;
-
-		/* If we disable the crtc, disable all its connectors. Also, if
-		 * the connector is on the changing crtc but not on the new
-		 * connector list, disable it. */
-		if (!set->fb || !in_mode_set) {
-			connector_state->best_encoder = NULL;
-
-			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [NOCRTC]\n",
-				connector->base.base.id,
-				connector->base.name);
-		}
-	}
-	/* connector->new_encoder is now updated for all connectors. */
-
-	for_each_connector_in_state(state, drm_connector, connector_state, i) {
-		connector = to_intel_connector(drm_connector);
-
-		if (!connector_state->best_encoder) {
-			ret = drm_atomic_set_crtc_for_connector(connector_state,
-								NULL);
-			if (ret)
-				return ret;
-
-			continue;
-		}
-
-		if (intel_connector_in_mode_set(connector, set)) {
-			struct drm_crtc *crtc = connector->base.state->crtc;
-
-			/* If this connector was in a previous crtc, add it
-			 * to the state. We might need to disable it. */
-			if (crtc) {
-				crtc_state =
-					drm_atomic_get_crtc_state(state, crtc);
-				if (IS_ERR(crtc_state))
-					return PTR_ERR(crtc_state);
-			}
-
-			ret = drm_atomic_set_crtc_for_connector(connector_state,
-								set->crtc);
-			if (ret)
-				return ret;
-		}
-
-		/* Make sure the new CRTC will work with the encoder */
-		if (!drm_encoder_crtc_ok(connector_state->best_encoder,
-					 connector_state->crtc)) {
-			return -EINVAL;
-		}
-
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
-			connector->base.base.id,
-			connector->base.name,
-			connector_state->crtc->base.id);
-
-		if (connector_state->best_encoder != &connector->encoder->base)
-			connector->encoder =
-				to_intel_encoder(connector_state->best_encoder);
-	}
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		bool has_connectors;
-
-		ret = drm_atomic_add_affected_connectors(state, crtc);
-		if (ret)
-			return ret;
-
-		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
-		if (has_connectors != crtc_state->enable)
-			crtc_state->enable =
-			crtc_state->active = has_connectors;
-	}
-
-	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
-					      set->fb, set->x, set->y);
-	if (ret)
-		return ret;
-
-	crtc_state = drm_atomic_get_crtc_state(state, set->crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	if (set->mode)
-		drm_mode_copy(&crtc_state->mode, set->mode);
-
-	if (set->num_connectors)
-		crtc_state->active = true;
-
-	return 0;
-}
-
-static int intel_crtc_set_config(struct drm_mode_set *set)
-{
-	struct drm_device *dev;
-	struct drm_atomic_state *state = NULL;
-	int ret;
-
-	BUG_ON(!set);
-	BUG_ON(!set->crtc);
-	BUG_ON(!set->crtc->helper_private);
-
-	/* Enforce sane interface api - has been abused by the fb helper. */
-	BUG_ON(!set->mode && set->fb);
-	BUG_ON(set->fb && set->num_connectors == 0);
-
-	if (set->fb) {
-		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-				set->crtc->base.id, set->fb->base.id,
-				(int)set->num_connectors, set->x, set->y);
-	} else {
-		DRM_DEBUG_KMS("[CRTC:%d] [NOFB]\n", set->crtc->base.id);
-	}
-
-	dev = set->crtc->dev;
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	ret = intel_modeset_stage_output_state(dev, set, state);
-	if (ret)
-		goto out;
-
-	ret = intel_modeset_compute_config(state);
-	if (ret)
-		goto out;
-
-	intel_update_pipe_size(to_intel_crtc(set->crtc));
-
-	ret = intel_set_mode_checked(state);
-	if (ret) {
-		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
-			      set->crtc->base.id, ret);
-	}
-
-out:
-	if (ret)
-		drm_atomic_state_free(state);
-	return ret;
-}
-
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.gamma_set = intel_crtc_gamma_set,
-	.set_config = intel_crtc_set_config,
+	.set_config = drm_atomic_helper_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
@@ -15338,7 +15142,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 		i915_redisable_vga(dev);
 
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 		if (ret) {
 			DRM_ERROR("Failed to restore previous mode\n");
 			drm_atomic_state_free(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 506a7e320fbd..894f7df105ca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1394,11 +1394,6 @@ void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
 void intel_tv_init(struct drm_device *dev);
 
 /* intel_atomic.c */
-int intel_atomic_check(struct drm_device *dev,
-		       struct drm_atomic_state *state);
-int intel_atomic_commit(struct drm_device *dev,
-			struct drm_atomic_state *state,
-			bool async);
 int intel_connector_atomic_get_property(struct drm_connector *connector,
 					const struct drm_connector_state *state,
 					struct drm_property *property,
-- 
2.1.0

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

[PATCH v2 25/27] drm/i915: Call plane update functions directly from intel_atomic_commit.

[Maarten Lankhorst]

Now that there's only a single path for all atomic updates we can call
intel_(pre/post)_plane_update from intel_atomic_commit directly. This
makes the intention more clear.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d5e783c170a7..6778b381d14c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12908,12 +12908,17 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state) && crtc->state->active) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		if (!needs_modeset(crtc->state)) {
+			intel_pre_plane_update(intel_crtc);
+		} else if (crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
 		}
 
 		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+		intel_post_plane_update(intel_crtc);
 	}
 
 	/* FIXME: add subpixel order */
@@ -13265,9 +13270,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	if (!needs_modeset(crtc->state))
-		intel_pre_plane_update(intel_crtc);
-
 	if (intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
 
@@ -13299,8 +13301,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 				      intel_crtc->atomic.start_vbl_count);
 
 	intel_runtime_pm_put(dev_priv);
-
-	intel_post_plane_update(intel_crtc);
 }
 
 /**
-- 
2.1.0

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

[PATCH v2 26/27] drm/i915: always disable irqs in intel_pipe_update_start

[Maarten Lankhorst]

This can only fail because of a bug in the code.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++----------
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6778b381d14c..728a3b9a7343 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11033,12 +11033,11 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
 static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
-	bool atomic_update;
 	u32 start_vbl_count;
 
 	intel_mark_page_flip_active(intel_crtc);
 
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+	intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	if (INTEL_INFO(dev)->gen >= 9)
 		skl_do_mmio_flip(intel_crtc);
@@ -11046,8 +11045,7 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 		/* use_mmio_flip() retricts MMIO flips to ilk+ */
 		ilk_do_mmio_flip(intel_crtc);
 
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
+	intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void intel_mmio_flip_work_func(struct work_struct *work)
@@ -13277,9 +13275,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 
 	/* Perform vblank evasion around commit operation */
 	if (crtc->state->active)
-		intel_crtc->atomic.evade =
-			intel_pipe_update_start(intel_crtc,
-						&intel_crtc->atomic.start_vbl_count);
+		intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
 
 	if (!needs_modeset(crtc->state)) {
 		if (INTEL_INFO(dev)->gen >= 9)
@@ -13296,9 +13292,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	if (intel_crtc->atomic.evade)
-		intel_pipe_update_end(intel_crtc,
-				      intel_crtc->atomic.start_vbl_count);
+	if (crtc->state->active)
+		intel_pipe_update_end(intel_crtc, intel_crtc->atomic.start_vbl_count);
 
 	intel_runtime_pm_put(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 894f7df105ca..fcc6735a1427 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1386,7 +1386,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
-bool intel_pipe_update_start(struct intel_crtc *crtc,
+void intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 994d9ac9078c..907b64714a60 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -75,10 +75,8 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
  * until a subsequent call to intel_pipe_update_end(). That is done to
  * avoid random delays. The value written to @start_vbl_count should be
  * supplied to intel_pipe_update_end() for error checking.
- *
- * Return: true if the call was successful
  */
-bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+void intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
@@ -96,13 +94,14 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 	min = vblank_start - usecs_to_scanlines(mode, 100);
 	max = vblank_start - 1;
 
+	local_irq_disable();
+	*start_vbl_count = 0;
+
 	if (min <= 0 || max <= 0)
-		return false;
+		return;
 
 	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
-		return false;
-
-	local_irq_disable();
+		return;
 
 	trace_i915_pipe_update_start(crtc, min, max);
 
@@ -138,8 +137,6 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
 	trace_i915_pipe_update_vblank_evaded(crtc, min, max, *start_vbl_count);
-
-	return true;
 }
 
 /**
@@ -161,7 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 
 	local_irq_enable();
 
-	if (start_vbl_count != end_vbl_count)
+	if (start_vbl_count && start_vbl_count != end_vbl_count)
 		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
 			  pipe_name(pipe), start_vbl_count, end_vbl_count);
 }
-- 
2.1.0

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

[PATCH v2 27/27] drm/i915: Only commit planes on crtc's that have changed planes.

[Maarten Lankhorst]

No point in applying vblank evasion if there's nothing to evade.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 728a3b9a7343..7a141e672fcd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12915,7 +12915,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 			dev_priv->display.crtc_enable(crtc);
 		}
 
-		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+		if (crtc->state->planes_changed)
+			drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+
 		intel_post_plane_update(intel_crtc);
 	}
 
-- 
2.1.0

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

Re: [PATCH v2 09/27] drm/i915: Assign a new pll from the crtc check function.

[Maarten Lankhorst]

Hey,

Op 04-06-15 om 14:47 schreef Maarten Lankhorst:
> <snip>
> @@ -12505,53 +12515,37 @@ static void update_scanline_offset(struct intel_crtc *crtc)
>  		crtc->scanline_offset = 1;
>  }
>  
> -static int intel_modeset_setup_plls(struct drm_atomic_state *state)
> +static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	unsigned clear_pipes = 0;
> +	struct intel_shared_dpll_config *shared_dpll = NULL;
>  	struct intel_crtc *intel_crtc;
>  	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
>  	int i;
>  
>  	if (!dev_priv->display.crtc_compute_clock)
> -		return 0;
> +		return;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		int dpll;
> +
>  		intel_crtc = to_intel_crtc(crtc);
>  		intel_crtc_state = to_intel_crtc_state(crtc_state);
> +		dpll = intel_crtc_state->shared_dpll;
>
Oops, this is slightly incorrect. This needs to be:

+		dpll = to_intel_crtc_state(crtc->state)->shared_dpll;

Or we could clear the crtc_mask on the wrong pll, which would mess up our tracking.

~Maarten

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

Re: [PATCH v2 03/27] drm/i915: clean up intel_sanitize_crtc, v2

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:33PM +0200, Maarten Lankhorst wrote:
> Instead of breaking all connections manually, kill them with
> atomic modeset, if it happens. Now that the everything's atomic
> some code becomes obsolete.
> 
> Forcing a atomic modeset with null mode and all connectors gone
> updates the state too, in a cleaner way.
> 
> Because load detection is atomic too there can be no mismatch between
> crtc->active and crtc->state->active
> 
> Changes since v1:
> - Reword commit message.
> - Set null mode and break all connectors when force disabling crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |   8 +--
>  drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +-
>  3 files changed, 53 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 51fe0fbcd00c..db23916c1875 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3637,7 +3637,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>  		bool active = pipe_config->base.active;
>  
>  		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> +			intel_crtc_control(&crtc->base, false, false);
>  			pipe_config = to_intel_crtc_state(crtc->base.state);
>  		}
>  
> @@ -3647,7 +3647,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>  					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
>  
>  		if (active)
> -			intel_crtc_control(&crtc->base, true);
> +			intel_crtc_control(&crtc->base, true, false);
>  	}
>  	drm_modeset_unlock_all(dev);
>  }
> @@ -3671,7 +3671,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>  		bool active = pipe_config->base.active;
>  
>  		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> +			intel_crtc_control(&crtc->base, false, false);
>  			pipe_config = to_intel_crtc_state(crtc->base.state);
>  		}
>  
> @@ -3681,7 +3681,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>  					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
>  
>  		if (active)
> -			intel_crtc_control(&crtc->base, true);
> +			intel_crtc_control(&crtc->base, true, false);
>  	}
>  	drm_modeset_unlock_all(dev);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0f031658a297..1936a28a57c1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6192,7 +6192,7 @@ free:
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> -int intel_crtc_control(struct drm_crtc *crtc, bool enable)
> +int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force)

I realize intel_display.c hasn't been fully kerneldoc-ized yet, but this
function seems like an important one to have a description of what it's
doing and what the parameters mean (the term 'force' is a bit
vague/generic and it may not be immediately obvious to a reader that
what you're doing here is going to wind up detaching the links.


Matt


>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
> @@ -6202,10 +6202,13 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  	struct drm_atomic_state *state;
>  	int ret;
>  
> -	if (enable == intel_crtc->active)
> +	if (WARN_ON(active && force))
> +		return -EINVAL;
> +
> +	if (active == intel_crtc->active)
>  		return 0;
>  
> -	if (enable && !crtc->state->enable)
> +	if (active && !crtc->state->enable)
>  		return 0;
>  
>  	/* this function should be called with drm_modeset_lock_all for now */
> @@ -6225,7 +6228,30 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  		ret = PTR_ERR(pipe_config);
>  		goto err;
>  	}
> -	pipe_config->base.active = enable;
> +	pipe_config->base.active = active;
> +	if (force) {
> +		struct drm_connector *connector;
> +
> +		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
> +		if (ret)
> +			goto err;
> +
> +		list_for_each_entry(connector, &config->connector_list, head) {
> +			struct drm_connector_state *conn_state;
> +
> +			if (connector->state->crtc != crtc)
> +				continue;
> +
> +			conn_state = drm_atomic_get_connector_state(state, connector);
> +			if (IS_ERR(conn_state)) {
> +				ret = PTR_ERR(conn_state);
> +				goto err;
> +			}
> +			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +			if (ret)
> +				goto err;
> +		}
> +	}
>  
>  	ret = intel_set_mode(state);
>  	if (!ret)
> @@ -6249,7 +6275,7 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
>  		enable |= intel_encoder->connectors_active;
>  
> -	intel_crtc_control(crtc, enable);
> +	intel_crtc_control(crtc, enable, false);
>  }
>  
>  void intel_encoder_destroy(struct drm_encoder *encoder)
> @@ -14812,11 +14838,14 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  	return true;
>  }
>  
> -static void intel_sanitize_crtc(struct intel_crtc *crtc)
> +static void intel_sanitize_crtc(struct intel_crtc *crtc,
> +				struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *intel_encoder;
>  	u32 reg;
> +	bool enable;
>  
>  	/* Clear any frame start delays used for debugging left by the BIOS */
>  	reg = PIPECONF(crtc->config->cpu_transcoder);
> @@ -14833,7 +14862,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * disable the crtc (and hence change the state) if it is wrong. Note
>  	 * that gen4+ has a fixed plane -> pipe mapping.  */
>  	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
> -		struct intel_connector *connector;
>  		bool plane;
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
> @@ -14846,33 +14874,12 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		to_intel_plane_state(crtc->base.primary->state)->visible = true;
>  		crtc->base.primary->crtc = &crtc->base;
>  		crtc->plane = !plane;
> -		intel_crtc_control(&crtc->base, false);
> +		intel_crtc_control(&crtc->base, false, true);
>  		crtc->plane = plane;
> -
> -		/* ... and break all links. */
> -		for_each_intel_connector(dev, connector) {
> -			if (connector->encoder->base.crtc != &crtc->base)
> -				continue;
> -
> -			connector->base.dpms = DRM_MODE_DPMS_OFF;
> -			connector->base.encoder = NULL;
> -		}
> -		/* multiple connectors may have the same encoder:
> -		 *  handle them and break crtc link separately */
> -		for_each_intel_connector(dev, connector)
> -			if (connector->encoder->base.crtc == &crtc->base) {
> -				connector->encoder->base.crtc = NULL;
> -				connector->encoder->connectors_active = false;
> -			}
> -
> -		WARN_ON(crtc->active);
> -		crtc->base.state->enable = false;
> -		crtc->base.state->active = false;
> -		crtc->base.enabled = false;
>  	}
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> -	    crtc->pipe == PIPE_A && !crtc->active) {
> +	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
>  		/* BIOS forgot to enable pipe A, this mostly happens after
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
> @@ -14881,36 +14888,16 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> -	 * have active connectors/encoders. */
> -	intel_crtc_update_dpms(&crtc->base);
> -
> -	if (crtc->active != crtc->base.state->active) {
> -		struct intel_encoder *encoder;
> +	 * have active connectors/encoders */
> +	enable = false;
> +	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> +		enable |= intel_encoder->connectors_active;
>  
> -		/* This can happen either due to bugs in the get_hw_state
> -		 * functions or because the pipe is force-enabled due to the
> -		 * pipe A quirk. */
> -		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
> -			      crtc->base.base.id,
> -			      crtc->base.state->enable ? "enabled" : "disabled",
> -			      crtc->active ? "enabled" : "disabled");
> -
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.state->active = crtc->active;
> -		crtc->base.enabled = crtc->active;
> -
> -		/* Because we only establish the connector -> encoder ->
> -		 * crtc links if something is active, this means the
> -		 * crtc is now deactivated. Break the links. connector
> -		 * -> encoder links are only establish when things are
> -		 *  actually up, hence no need to break them. */
> -		WARN_ON(crtc->active);
> -
> -		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> -			WARN_ON(encoder->connectors_active);
> -			encoder->base.crtc = NULL;
> -		}
> -	}
> +	/* only turn off separately if configuration's not restored,
> +	 * if it's restored it will change mode or turn off anyway.
> +	 */
> +	if (!enable && crtc->base.state->active && !pipe_config)
> +		intel_crtc_control(&crtc->base, false, true);
>  
>  	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
>  		/*
> @@ -15278,7 +15265,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  				      &crtc->state->adjusted_mode);
>  		}
>  
> -		intel_sanitize_crtc(intel_crtc);
> +		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
> +				    to_intel_crtc_state(crtc_state));
>  
>  		/*
>  		 * sanitize_crtc may have forced an update of crtc->state,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 807e001e4cee..ec721e31eae8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -993,7 +993,7 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>  int intel_display_suspend(struct drm_device *dev);
> -int intel_crtc_control(struct drm_crtc *crtc, bool enable);
> +int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 03/27] drm/i915: clean up intel_sanitize_crtc, v2

[Maarten Lankhorst]

Op 10-06-15 om 03:58 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:33PM +0200, Maarten Lankhorst wrote:
>> Instead of breaking all connections manually, kill them with
>> atomic modeset, if it happens. Now that the everything's atomic
>> some code becomes obsolete.
>>
>> Forcing a atomic modeset with null mode and all connectors gone
>> updates the state too, in a cleaner way.
>>
>> Because load detection is atomic too there can be no mismatch between
>> crtc->active and crtc->state->active
>>
>> Changes since v1:
>> - Reword commit message.
>> - Set null mode and break all connectors when force disabling crtc.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  |   8 +--
>>  drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++-------------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   2 +-
>>  3 files changed, 53 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 51fe0fbcd00c..db23916c1875 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3637,7 +3637,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>>  		bool active = pipe_config->base.active;
>>  
>>  		if (active) {
>> -			intel_crtc_control(&crtc->base, false);
>> +			intel_crtc_control(&crtc->base, false, false);
>>  			pipe_config = to_intel_crtc_state(crtc->base.state);
>>  		}
>>  
>> @@ -3647,7 +3647,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>>  					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
>>  
>>  		if (active)
>> -			intel_crtc_control(&crtc->base, true);
>> +			intel_crtc_control(&crtc->base, true, false);
>>  	}
>>  	drm_modeset_unlock_all(dev);
>>  }
>> @@ -3671,7 +3671,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>>  		bool active = pipe_config->base.active;
>>  
>>  		if (active) {
>> -			intel_crtc_control(&crtc->base, false);
>> +			intel_crtc_control(&crtc->base, false, false);
>>  			pipe_config = to_intel_crtc_state(crtc->base.state);
>>  		}
>>  
>> @@ -3681,7 +3681,7 @@ static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
>>  					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
>>  
>>  		if (active)
>> -			intel_crtc_control(&crtc->base, true);
>> +			intel_crtc_control(&crtc->base, true, false);
>>  	}
>>  	drm_modeset_unlock_all(dev);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0f031658a297..1936a28a57c1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6192,7 +6192,7 @@ free:
>>  }
>>  
>>  /* Master function to enable/disable CRTC and corresponding power wells */
>> -int intel_crtc_control(struct drm_crtc *crtc, bool enable)
>> +int intel_crtc_control(struct drm_crtc *crtc, bool active, bool force)
> I realize intel_display.c hasn't been fully kerneldoc-ized yet, but this
> function seems like an important one to have a description of what it's
> doing and what the parameters mean (the term 'force' is a bit
> vague/generic and it may not be immediately obvious to a reader that
> what you're doing here is going to wind up detaching the links.
Yeah no worries, this patch can be dropped. I think we should strive to perform a single modeset the hw readout state.

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

Re: [PATCH v2 11/27] drm/i915: Split skl_update_scaler, v2.

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:41PM +0200, Maarten Lankhorst wrote:
> It's easier to read separate functions for crtc and plane scaler state.
> 
> Changes since v1:
>  - Update documentation.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 200 ++++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_dp.c      |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  12 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  |   3 +-
>  4 files changed, 113 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 967398cc03cb..310b98226d82 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4299,62 +4299,17 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  	}
>  }
>  
> -/**
> - * skl_update_scaler_users - Stages update to crtc's scaler state
> - * @intel_crtc: crtc
> - * @crtc_state: crtc_state
> - * @plane: plane (NULL indicates crtc is requesting update)
> - * @plane_state: plane's state
> - * @force_detach: request unconditional detachment of scaler
> - *
> - * This function updates scaler state for requested plane or crtc.
> - * To request scaler usage update for a plane, caller shall pass plane pointer.
> - * To request scaler usage update for crtc, caller shall pass plane pointer
> - * as NULL.
> - *
> - * Return
> - *     0 - scaler_usage updated successfully
> - *    error - requested scaling cannot be supported or other error condition
> - */
> -int
> -skl_update_scaler_users(
> -	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
> -	struct intel_plane *intel_plane, struct intel_plane_state *plane_state,
> -	int force_detach)
> +static int
> +skl_update_scaler(const char *name, int i, unsigned idx,

This is a pretty confusing function signature with 'i,' 'idx,' and
'scaler_id' all as parameters.  Even though this is a static function, I
think keeping kerneldoc (and possibly renaming these to be more clear
about what they represent an index or ID of) would be a good idea.

Actually, a bunch of the parameters we pass here have no use except for
very verbose DRM_DEBUG_KMS() messages.  Could we just have a single
detailed message at the callsite and eliminate all of the scaler ID's,
object ID's, src/dest rectangles, etc. from the messages in this
function?  That might simplify things a bit.


Matt

> +		  struct intel_crtc_state *crtc_state, bool force_detach,
> +		  int *scaler_id, unsigned int rotation,
> +		  int src_w, int src_h, int dst_w, int dst_h)
>  {
> +	struct intel_crtc_scaler_state *scaler_state =
> +		&crtc_state->scaler_state;
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(crtc_state->base.crtc);
>  	int need_scaling;
> -	int idx;
> -	int src_w, src_h, dst_w, dst_h;
> -	int *scaler_id;
> -	struct drm_framebuffer *fb;
> -	struct intel_crtc_scaler_state *scaler_state;
> -	unsigned int rotation;
> -
> -	if (!intel_crtc || !crtc_state)
> -		return 0;
> -
> -	scaler_state = &crtc_state->scaler_state;
> -
> -	idx = intel_plane ? drm_plane_index(&intel_plane->base) : SKL_CRTC_INDEX;
> -	fb = intel_plane ? plane_state->base.fb : NULL;
> -
> -	if (intel_plane) {
> -		src_w = drm_rect_width(&plane_state->src) >> 16;
> -		src_h = drm_rect_height(&plane_state->src) >> 16;
> -		dst_w = drm_rect_width(&plane_state->dst);
> -		dst_h = drm_rect_height(&plane_state->dst);
> -		scaler_id = &plane_state->scaler_id;
> -		rotation = plane_state->base.rotation;
> -	} else {
> -		struct drm_display_mode *adjusted_mode =
> -			&crtc_state->base.adjusted_mode;
> -		src_w = crtc_state->pipe_src_w;
> -		src_h = crtc_state->pipe_src_h;
> -		dst_w = adjusted_mode->hdisplay;
> -		dst_h = adjusted_mode->vdisplay;
> -		scaler_id = &scaler_state->scaler_id;
> -		rotation = DRM_ROTATE_0;
> -	}
>  
>  	need_scaling = intel_rotation_90_or_270(rotation) ?
>  		(src_h != dst_w || src_w != dst_h):
> @@ -4370,18 +4325,15 @@ skl_update_scaler_users(
>  	 * update to free the scaler is done in plane/panel-fit programming.
>  	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
>  	 */
> -	if (force_detach || !need_scaling || (intel_plane &&
> -		(!fb || !plane_state->visible))) {
> +	if (force_detach || !need_scaling) {
>  		if (*scaler_id >= 0) {
>  			scaler_state->scaler_users &= ~(1 << idx);
>  			scaler_state->scalers[*scaler_id].in_use = 0;
>  
>  			DRM_DEBUG_KMS("Staged freeing scaler id %d.%d from %s:%d "
>  				"crtc_state = %p scaler_users = 0x%x\n",
> -				intel_crtc->pipe, *scaler_id, intel_plane ? "PLANE" : "CRTC",
> -				intel_plane ? intel_plane->base.base.id :
> -				intel_crtc->base.base.id, crtc_state,
> -				scaler_state->scaler_users);
> +				intel_crtc->pipe, *scaler_id, name, i,
> +				crtc_state, scaler_state->scaler_users);
>  			*scaler_id = -1;
>  		}
>  		return 0;
> @@ -4395,49 +4347,104 @@ skl_update_scaler_users(
>  		dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H) {
>  		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u dst %ux%u "
>  			"size is out of scaler range\n",
> -			intel_plane ? "PLANE" : "CRTC",
> -			intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> -			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
> +			name, i, intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
>  		return -EINVAL;
>  	}
>  
> +	/* mark this plane as a scaler user in crtc_state */
> +	scaler_state->scaler_users |= (1 << idx);
> +	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
> +		"crtc_state = %p scaler_users = 0x%x\n",
> +		name, i, src_w, src_h, dst_w, dst_h,
> +		crtc_state, scaler_state->scaler_users);
> +
> +	return 0;
> +}
> +
> +/**
> + * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
> + *
> + * @state: crtc's scaler state
> + * @force_detach: whether to forcibly disable scaler
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
> +{
> +	struct drm_crtc *crtc = state->base.crtc;
> +	struct drm_display_mode *adjusted_mode =
> +		&state->base.adjusted_mode;
> +
> +	return skl_update_scaler("CRTC", crtc->base.id,
> +		SKL_CRTC_INDEX, state, force_detach,
> +		&state->scaler_state.scaler_id, DRM_ROTATE_0,
> +		state->pipe_src_w, state->pipe_src_h,
> +		adjusted_mode->hdisplay, adjusted_mode->hdisplay);
> +}
> +
> +/**
> + * skl_update_scaler_plane - Stages update to scaler state for a given plane.
> + *
> + * @state: crtc's scaler state
> + * @intel_plane: affected plane
> + * @plane_state: atomic plane state to update
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
> +			    struct intel_plane *intel_plane,
> +			    struct intel_plane_state *plane_state)
> +{
> +
> +	struct drm_framebuffer *fb = plane_state->base.fb;
> +	int ret;
> +
> +	bool force_detach = !fb || !plane_state->visible;
> +
> +	ret = skl_update_scaler("PLANE", intel_plane->base.base.id,
> +				drm_plane_index(&intel_plane->base),
> +				crtc_state, force_detach,
> +				&plane_state->scaler_id,
> +				plane_state->base.rotation,
> +				drm_rect_width(&plane_state->src) >> 16,
> +				drm_rect_height(&plane_state->src) >> 16,
> +				drm_rect_width(&plane_state->dst),
> +				drm_rect_height(&plane_state->dst));
> +
> +	if (ret || plane_state->scaler_id < 0)
> +		return ret;
> +
>  	/* check colorkey */
> -	if (WARN_ON(intel_plane &&
> -		intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> -		DRM_DEBUG_KMS("PLANE:%d scaling %ux%u->%ux%u not allowed with colorkey",
> -			intel_plane->base.base.id, src_w, src_h, dst_w, dst_h);
> +	if (WARN_ON(intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> +		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
> +			intel_plane->base.base.id);
>  		return -EINVAL;
>  	}
>  
>  	/* Check src format */
> -	if (intel_plane) {
> -		switch (fb->pixel_format) {
> -		case DRM_FORMAT_RGB565:
> -		case DRM_FORMAT_XBGR8888:
> -		case DRM_FORMAT_XRGB8888:
> -		case DRM_FORMAT_ABGR8888:
> -		case DRM_FORMAT_ARGB8888:
> -		case DRM_FORMAT_XRGB2101010:
> -		case DRM_FORMAT_XBGR2101010:
> -		case DRM_FORMAT_YUYV:
> -		case DRM_FORMAT_YVYU:
> -		case DRM_FORMAT_UYVY:
> -		case DRM_FORMAT_VYUY:
> -			break;
> -		default:
> -			DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
> -				intel_plane->base.base.id, fb->base.id, fb->pixel_format);
> -			return -EINVAL;
> -		}
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_RGB565:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ABGR8888:
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("PLANE:%d FB:%d unsupported scaling format 0x%x\n",
> +			intel_plane->base.base.id, fb->base.id, fb->pixel_format);
> +		return -EINVAL;
>  	}
>  
> -	/* mark this plane as a scaler user in crtc_state */
> -	scaler_state->scaler_users |= (1 << idx);
> -	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
> -		"crtc_state = %p scaler_users = 0x%x\n",
> -		intel_plane ? "PLANE" : "CRTC",
> -		intel_plane ? intel_plane->base.base.id : intel_crtc->base.base.id,
> -		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state->scaler_users);
>  	return 0;
>  }
>  
> @@ -4452,7 +4459,7 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
>  	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
>  
>  	/* To update pfit, first update scaler state */
> -	skl_update_scaler_users(crtc, crtc->config, NULL, NULL, !enable);
> +	skl_update_scaler_crtc(crtc->config, !enable);
>  	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
>  	skl_detach_scalers(crtc);
>  	if (!enable)
> @@ -13339,8 +13346,9 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state,
> -			to_intel_plane(plane), state, 0);
> +		ret = skl_update_scaler_plane(crtc_state,
> +					      to_intel_plane(plane),
> +					      state);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 15e88922a984..6bfb3eb62e9f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1376,7 +1376,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  
>  		if (INTEL_INFO(dev)->gen >= 9) {
>  			int ret;
> -			ret = skl_update_scaler_users(intel_crtc, pipe_config, NULL, NULL, 0);
> +			ret = skl_update_scaler_crtc(pipe_config, 0);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1d9feb8727ea..e116a5f6346e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -252,7 +252,7 @@ struct intel_plane_state {
>  	 * plane requiring a scaler:
>  	 *   - During check_plane, its bit is set in
>  	 *     crtc_state->scaler_state.scaler_users by calling helper function
> -	 *     update_scaler_users.
> +	 *     update_scaler_plane.
>  	 *   - scaler_id indicates the scaler it got assigned.
>  	 *
>  	 * plane doesn't require a scaler:
> @@ -260,7 +260,7 @@ struct intel_plane_state {
>  	 *     got disabled.
>  	 *   - During check_plane, corresponding bit is reset in
>  	 *     crtc_state->scaler_state.scaler_users by calling helper function
> -	 *     update_scaler_users.
> +	 *     update_scaler_plane.
>  	 */
>  	int scaler_id;
>  };
> @@ -1136,9 +1136,11 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  void skl_detach_scalers(struct intel_crtc *intel_crtc);
> -int skl_update_scaler_users(struct intel_crtc *intel_crtc,
> -	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
> -	struct intel_plane_state *plane_state, int force_detach);
> +int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
> +			    struct intel_plane *intel_plane,
> +			    struct intel_plane_state *plane_state);
> +
> +int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8193a35388d7..fe95f25f019a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -941,8 +941,7 @@ finish:
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
> -			state, 0);
> +		ret = skl_update_scaler_plane(crtc_state, intel_plane, state);
>  		if (ret)
>  			return ret;
>  	}
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/27] drm/i915: Split plane updates of crtc->atomic into a helper, v2.

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:42PM +0200, Maarten Lankhorst wrote:
> This makes it easier to verify that no changes are done when
> calling this from crtc instead.
> 
> Changes since v1:
>  - Make intel_wm_need_update static and always check it.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Do we even need crtc->atomic anymore?  When I first added that, I
expected it to just be a temporary dumping ground until we had
crtc_state's tracked and swapped properly (which we do now).  Can we
just migrate these fields into the state structure instead?


<snip>
> +int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> +				    struct drm_plane_state *plane_state)
> +{
> +	struct drm_crtc *crtc = crtc_state->crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_plane *plane = plane_state->plane;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	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;
> +
> +	bool turn_off, turn_on, visible, was_visible;
> +	struct drm_framebuffer *fb = plane_state->fb;
> +
> +	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> +	    plane->type != DRM_PLANE_TYPE_CURSOR) {
> +		ret = skl_update_scaler_plane(
> +			to_intel_crtc_state(crtc_state),
> +			to_intel_plane_state(plane_state));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Disabling a plane is always okay; we just need to update
> +	 * fb tracking in a special way since cleanup_fb() won't
> +	 * get called by the plane helpers.
> +	 */
> +	if (old_plane_state->base.fb && !fb)
> +		intel_crtc->atomic.disabled_planes |= 1 << i;
> +
> +	/* don't run rest during modeset yet */
> +	if (!intel_crtc->active || mode_changed)
> +		return 0;
> +
> +	was_visible = old_plane_state->visible;
> +	visible = to_intel_plane_state(plane_state)->visible;
> +
> +	if (!was_crtc_enabled && WARN_ON(was_visible))
> +		was_visible = false;
> +
> +	if (!is_crtc_enabled && WARN_ON(visible))
> +		visible = false;
> +
> +	if (!was_visible && !visible)
> +		return 0;
> +
> +	turn_off = was_visible && (!visible || mode_changed);
> +	turn_on = visible && (!was_visible || mode_changed);
> +
> +	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
> +			 plane->base.id, fb ? fb->base.id : -1);
> +
> +	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
> +			 plane->base.id, was_visible, visible,
> +			 turn_off, turn_on, mode_changed);
> +
> +	if (intel_wm_need_update(plane, plane_state))
> +		intel_crtc->atomic.update_wm = true;

Hmm, I realize this is the way the code already worked, but this is only
going to trigger update_watermarks rather than update_sprite_watermarks,
which on some platforms could make us update watermarks with stale
sprite values.  I think the only reason we get away with this today is
because we actually perform sprite watermark updates in the low-level
plane update functions (which is bad since we're under vblank
evasion...).  I had some patches that fixed that oversight as part of my
watermark RFC series, but they haven't gone in; I probably need to
extract the fix from the rest of the RFC.

Not sure if you want to worry about it as part of your work here or not
since this doesn't leave us any worse off than we already are today;
just figured I'd mention it so we don't forget about it.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote:
> By passing crtc_state to the check_plane functions a lot of duplicated
> code can be removed. And now that the transitional helpers are gone the
> crtc_state can be reliably obtained.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
>  4 files changed, 23 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aa2128369a0a..4d8cacbca777 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	crtc = crtc ? crtc : plane->crtc;
>  	intel_crtc = to_intel_crtc(crtc);
>  
> +	intel_state->visible = false;
> +

What do we need this change for?  Primary and cursor check functions
immediately overwrite state->visible, so setting this here has no
effect.  The sprite case where fb==NULL is the only case where this
would matter, but moving the assignment from the sprite check function
to here doesn't seem like it gains us anything.

Here's the only effect I see it having:

> @@ -757,14 +758,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
<snip>
> -	if (!fb) {
> -		state->visible = false;
> +	if (!fb)
>  		return 0;
> -	}
>  
>  	/* Don't modify another pipe's plane */
>  	if (intel_plane->pipe != intel_crtc->pipe) {

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 16/27] drm/i915: move detaching scalers to begin_crtc_commit, v2.

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:46PM +0200, Maarten Lankhorst wrote:
> This is probably intended to be be done during vblank evasion.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 3 ---
>  drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>  drivers/gpu/drm/i915/intel_drv.h     | 1 -
>  3 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 7ca225194b7c..8447a1fef332 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -144,9 +144,6 @@ int intel_atomic_commit(struct drm_device *dev,
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>  
> -		if (INTEL_INFO(dev)->gen >= 9)
> -			skl_detach_scalers(to_intel_crtc(crtc));
> -
>  		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f5f074524603..2ba3d6abbd0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2911,16 +2911,13 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  /*
>   * This function detaches (aka. unbinds) unused scalers in hardware
>   */
> -void skl_detach_scalers(struct intel_crtc *intel_crtc)
> +static void skl_detach_scalers(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_device *dev;
>  	struct drm_i915_private *dev_priv;
>  	struct intel_crtc_scaler_state *scaler_state;
>  	int i;
>  
> -	if (!intel_crtc || !intel_crtc->config)
> -		return;
> -
>  	dev = intel_crtc->base.dev;
>  	dev_priv = dev->dev_private;
>  	scaler_state = &intel_crtc->config->scaler_state;
> @@ -13505,6 +13502,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> +
> +	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> +		skl_detach_scalers(intel_crtc);

I'm not sure I follow where the !needs_modeset test comes from.  Do
these get blown away somewhere else in the modeset sequence that I'm
overlooking?  Detaching scalers here when we are doing a modeset
wouldn't cause actual problems would it?


Matt

>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0dae94631094..d96c9a24b94b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1135,7 +1135,6 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
> -void skl_detach_scalers(struct intel_crtc *intel_crtc);
>  
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 18/27] drm/i915: Handle disabling planes better.

[Matt Roper]

On Thu, Jun 04, 2015 at 02:47:48PM +0200, Maarten Lankhorst wrote:
> Read out the initial state, and add a quirk to force disable all plane
> that were initially active. Use this to disable planes during modeset
> too.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |   7 ++
>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>  3 files changed, 114 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 8447a1fef332..5627df2807b0 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> +	if (crtc_state &&
> +	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
> +		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b72724121f57..3b5d23692935 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>  }
>  
> +static void intel_disable_planes_on_crtc(struct drm_crtc *c)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(c);
> +	struct drm_plane *plane;
> +
> +	drm_for_each_plane_mask(plane, crtc->base.dev,
> +				crtc->atomic.force_disabled_planes)
> +		to_intel_plane(plane)->disable_plane(plane, c);
> +}
> +
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
>  	int pipe = intel_crtc->pipe;
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
> @@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	intel_pre_disable_primary(crtc);
>  
>  	intel_crtc_dpms_overlay_disable(intel_crtc);
> -	for_each_intel_plane(dev, intel_plane) {
> -		if (intel_plane->pipe == pipe) {
> -			struct drm_crtc *from = intel_plane->base.crtc;
> -
> -			intel_plane->disable_plane(&intel_plane->base,
> -						   from ?: crtc);
> -		}
> -	}
> +	intel_disable_planes_on_crtc(crtc);
>  
>  	/*
>  	 * FIXME: Once we grow proper nuclear flip support out of this we need
> @@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	if (!intel_crtc->active || mode_changed)
>  		return 0;
>  
> +	if (to_intel_crtc_state(crtc_state)->quirks &
> +	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> +		if (!plane_state->crtc) {
> +			intel_crtc->atomic.force_disabled_planes |=
> +				1 << drm_plane_index(plane);
> +			crtc_state->plane_mask &=
> +				~(1 << drm_plane_index(plane));
> +		}
> +
> +		/* Initial state for sprite planes is unknown,
> +		 * no need to update sprite watermarks */
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> +			mode_changed = true;
> +	}
> +
>  	was_visible = old_plane_state->visible;
>  	visible = to_intel_plane_state(plane_state)->visible;
>  
> @@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			intel_crtc->atomic.fb_bits |=
>  			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>  
> -		if (turn_off && is_crtc_enabled) {
> +		if (turn_off && !mode_changed) {
>  			intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
> @@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>  		idx, crtc->state->active, intel_crtc->active);
>  
> +	/* plane mask is fixed up after all initial planes are calculated */
> +	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> +	if (mode_changed)
> +		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
> +
>  	if (mode_changed && crtc_state->enable &&
>  	    dev_priv->display.crtc_compute_clock &&
>  	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
> @@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  		return ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (to_intel_crtc_state(crtc_state)->quirks &
> +		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> +			ret = drm_atomic_add_affected_planes(state, crtc);
> +			if (ret)
> +				return ret;
> +		}
> +
>  		if (!needs_modeset(crtc_state))
>  			continue;
>  
> @@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (crtc->state->active && !needs_modeset(crtc->state))
> +	if (crtc->state->active && !needs_modeset(crtc->state)) {
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
>  
> +		if (intel_crtc->atomic.force_disabled_planes)
> +			intel_disable_planes_on_crtc(crtc);
> +	}
> +
>  	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
> -		skl_detach_scalers(intel_crtc);
> +                skl_detach_scalers(intel_crtc);

Unintentional change from tabs to spaces on this line?

>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
> @@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
>  	i915_redisable_vga_power_on(dev);
>  }
>  
> -static bool primary_get_hw_state(struct intel_crtc *crtc)
> +static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
> +				      struct intel_plane *intel_plane)
>  {
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!crtc->base.enabled)
> -		return false;
> +	switch (intel_plane->base.type) {
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> +
> +	case DRM_PLANE_TYPE_CURSOR:
> +		if (IS_845G(dev) || IS_I865G(dev))
> +			return I915_READ(_CURACNTR) & CURSOR_ENABLE;
> +		else
> +			return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
> +

If we're not trying to seamlessly inherit the cursor image, I'm not sure
I understand what the benefit of figuring out whether the cursor is
actually on or not is.  Just assuming true, as we do for sprites, would
be enough to make the cursor always turn off, right?

> +	default:
> +		return true;
> +	}
> +}
> +
> +static int readout_plane_state(struct drm_atomic_state *state,
> +			       struct intel_crtc *crtc,
> +			       struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_plane *p;
> +	struct drm_plane_state *drm_plane_state;
> +	bool active = crtc_state->base.active;
> +
> +	if (active) {
> +		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>  
> -	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> +		/* apply to previous sw state too */
> +		to_intel_crtc_state(crtc->base.state)->quirks |=
> +			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> +	}
> +
> +	for_each_intel_plane(state->dev, p) {
> +		if (crtc->plane != p->plane)
> +			continue;

Was this supposed to be comparing ->pipe rather than ->plane?  For
primary and cursor planes I believe this works out okay since ->plane
and ->pipe are basically the same (except for pre-gen4 FBC swapping),
but for sprite planes, the ->plane pointer indicates whether it's the
first, second, third, etc. sprite of that specific CRTC.

As written, I think you'll only be operating on the first sprite plane
of CRTC 0, the second sprite plane of CRTC 1, etc.


Matt

> +
> +		drm_plane_state = drm_atomic_get_plane_state(state, &p->base);
> +		if (IS_ERR(drm_plane_state))
> +			return PTR_ERR(drm_plane_state);
> +
> +		if (active && intel_read_hw_plane_state(crtc, p)) {
> +			to_intel_plane_state(drm_plane_state)->visible = true;
> +			crtc_state->base.plane_mask |= 1 << drm_plane_index(&p->base);
> +			p->base.crtc = &crtc->base;
> +		} else {
> +			to_intel_plane_state(drm_plane_state)->visible = false;
> +			crtc_state->base.plane_mask &= ~(1 << drm_plane_index(&p->base));
> +			p->base.crtc = drm_plane_state->crtc;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static int readout_hw_crtc_state(struct drm_atomic_state *state,
> @@ -15055,19 +15137,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc_state *crtc_state;
> -	struct drm_plane *primary = crtc->base.primary;
> -	struct drm_plane_state *drm_plane_state;
> -	struct intel_plane_state *plane_state;
> -	int ret;
>  
>  	crtc_state = intel_atomic_get_crtc_state(state, crtc);
>  	if (IS_ERR(crtc_state))
>  		return PTR_ERR(crtc_state);
>  
> -	ret = drm_atomic_add_affected_planes(state, &crtc->base);
> -	if (ret)
> -		return ret;
> -
>  	memset(crtc_state, 0, sizeof(*crtc_state));
>  	crtc_state->base.crtc = &crtc->base;
>  	crtc_state->base.state = state;
> @@ -15081,24 +15155,11 @@ static int readout_hw_crtc_state(struct drm_atomic_state *state,
>  	crtc->active = crtc_state->base.active;
>  	crtc->config = crtc_state;
>  
> -	drm_plane_state = drm_atomic_get_plane_state(state, primary);
> -	if (IS_ERR(drm_plane_state))
> -		return PTR_ERR(drm_plane_state);
> -
> -	plane_state = to_intel_plane_state(drm_plane_state);
> -	plane_state->visible = primary_get_hw_state(crtc);
> -
> -	if (plane_state->visible) {
> -		primary->crtc = &crtc->base;
> -		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
> -	} else
> -		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
> -
>  	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  		      crtc->base.base.id,
>  		      crtc_state->base.active ? "enabled" : "disabled");
>  
> -	return 0;
> +	return readout_plane_state(state, crtc, crtc_state);
>  }
>  
>  static int readout_hw_pll_state(struct drm_atomic_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d96c9a24b94b..7b1a13f63872 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -327,6 +327,7 @@ struct intel_crtc_state {
>  	 */
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
>  #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
> +#define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
>  	unsigned long quirks;
>  
>  	/* Pipe source size (ie. panel fitter input size)
> @@ -485,8 +486,14 @@ struct intel_crtc_atomic_commit {
>  	bool disable_fbc;
>  	bool pre_disable_primary;
>  	bool update_wm;
> +
> +	/* planes that will be disabled in a normal way */
>  	unsigned disabled_planes;
>  
> +	/* planes that will be disabled during modeset,
> +	 * or initially enabled planes */
> +	unsigned force_disabled_planes;
> +
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
>  	bool wait_vblank;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/27] drm/i915: Split plane updates of crtc->atomic into a helper, v2.

[Maarten Lankhorst]

Op 11-06-15 om 03:35 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:42PM +0200, Maarten Lankhorst wrote:
>> This makes it easier to verify that no changes are done when
>> calling this from crtc instead.
>>
>> Changes since v1:
>>  - Make intel_wm_need_update static and always check it.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Do we even need crtc->atomic anymore?  When I first added that, I
> expected it to just be a temporary dumping ground until we had
> crtc_state's tracked and swapped properly (which we do now).  Can we
> just migrate these fields into the state structure instead?
This is temporarily, I need full conversion to atomic to kill off crtc->atomic.

After that I have 2 update functions, pre_plane_update and post_plane_update.

Any required changes could then be calculated from plane_state->visible, old_plane_state->visible and crtc enabled/disabled.
> <snip>
>> +int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>> +				    struct drm_plane_state *plane_state)
>> +{
>> +	struct drm_crtc *crtc = crtc_state->crtc;
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +	struct drm_plane *plane = plane_state->plane;
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	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;
>> +
>> +	bool turn_off, turn_on, visible, was_visible;
>> +	struct drm_framebuffer *fb = plane_state->fb;
>> +
>> +	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
>> +	    plane->type != DRM_PLANE_TYPE_CURSOR) {
>> +		ret = skl_update_scaler_plane(
>> +			to_intel_crtc_state(crtc_state),
>> +			to_intel_plane_state(plane_state));
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	/*
>> +	 * Disabling a plane is always okay; we just need to update
>> +	 * fb tracking in a special way since cleanup_fb() won't
>> +	 * get called by the plane helpers.
>> +	 */
>> +	if (old_plane_state->base.fb && !fb)
>> +		intel_crtc->atomic.disabled_planes |= 1 << i;
>> +
>> +	/* don't run rest during modeset yet */
>> +	if (!intel_crtc->active || mode_changed)
>> +		return 0;
>> +
>> +	was_visible = old_plane_state->visible;
>> +	visible = to_intel_plane_state(plane_state)->visible;
>> +
>> +	if (!was_crtc_enabled && WARN_ON(was_visible))
>> +		was_visible = false;
>> +
>> +	if (!is_crtc_enabled && WARN_ON(visible))
>> +		visible = false;
>> +
>> +	if (!was_visible && !visible)
>> +		return 0;
>> +
>> +	turn_off = was_visible && (!visible || mode_changed);
>> +	turn_on = visible && (!was_visible || mode_changed);
>> +
>> +	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
>> +			 plane->base.id, fb ? fb->base.id : -1);
>> +
>> +	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
>> +			 plane->base.id, was_visible, visible,
>> +			 turn_off, turn_on, mode_changed);
>> +
>> +	if (intel_wm_need_update(plane, plane_state))
>> +		intel_crtc->atomic.update_wm = true;
> Hmm, I realize this is the way the code already worked, but this is only
> going to trigger update_watermarks rather than update_sprite_watermarks,
> which on some platforms could make us update watermarks with stale
> sprite values.  I think the only reason we get away with this today is
> because we actually perform sprite watermark updates in the low-level
> plane update functions (which is bad since we're under vblank
> evasion...).  I had some patches that fixed that oversight as part of my
> watermark RFC series, but they haven't gone in; I probably need to
> extract the fix from the rest of the RFC.
>
> Not sure if you want to worry about it as part of your work here or not
> since this doesn't leave us any worse off than we already are today;
> just figured I'd mention it so we don't forget about it.
>
Yeah, I only wanted to preserve current behavior in this commit, so it becomes more clear why it happens.

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

Re: [PATCH v2 16/27] drm/i915: move detaching scalers to begin_crtc_commit, v2.

[Maarten Lankhorst]

Op 11-06-15 om 03:36 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:46PM +0200, Maarten Lankhorst wrote:
>> This is probably intended to be be done during vblank evasion.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  | 3 ---
>>  drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>>  drivers/gpu/drm/i915/intel_drv.h     | 1 -
>>  3 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 7ca225194b7c..8447a1fef332 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -144,9 +144,6 @@ int intel_atomic_commit(struct drm_device *dev,
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>>  
>> -		if (INTEL_INFO(dev)->gen >= 9)
>> -			skl_detach_scalers(to_intel_crtc(crtc));
>> -
>>  		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f5f074524603..2ba3d6abbd0c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2911,16 +2911,13 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>>  /*
>>   * This function detaches (aka. unbinds) unused scalers in hardware
>>   */
>> -void skl_detach_scalers(struct intel_crtc *intel_crtc)
>> +static void skl_detach_scalers(struct intel_crtc *intel_crtc)
>>  {
>>  	struct drm_device *dev;
>>  	struct drm_i915_private *dev_priv;
>>  	struct intel_crtc_scaler_state *scaler_state;
>>  	int i;
>>  
>> -	if (!intel_crtc || !intel_crtc->config)
>> -		return;
>> -
>>  	dev = intel_crtc->base.dev;
>>  	dev_priv = dev->dev_private;
>>  	scaler_state = &intel_crtc->config->scaler_state;
>> @@ -13505,6 +13502,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>  		intel_crtc->atomic.evade =
>>  			intel_pipe_update_start(intel_crtc,
>>  						&intel_crtc->atomic.start_vbl_count);
>> +
>> +	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> +		skl_detach_scalers(intel_crtc);
> I'm not sure I follow where the !needs_modeset test comes from.  Do
> these get blown away somewhere else in the modeset sequence that I'm
> overlooking?  Detaching scalers here when we are doing a modeset
> wouldn't cause actual problems would it?
No it won't cause direct problems, just unneeded since modeset disable would already disable scalers.

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

Re: [PATCH v2 18/27] drm/i915: Handle disabling planes better.

[Maarten Lankhorst]

Op 11-06-15 om 03:37 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:48PM +0200, Maarten Lankhorst wrote:
>> Read out the initial state, and add a quirk to force disable all plane
>> that were initially active. Use this to disable planes during modeset
>> too.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |   7 ++
>>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>>  3 files changed, 114 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 8447a1fef332..5627df2807b0 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -96,6 +96,13 @@ int intel_atomic_check(struct drm_device *dev,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (crtc_state &&
>> +	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE) {
>> +		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	ret = drm_atomic_helper_check_planes(dev, state);
>>  	if (ret)
>>  		return ret;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b72724121f57..3b5d23692935 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4830,11 +4830,20 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>>  	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));
>>  }
>>  
>> +static void intel_disable_planes_on_crtc(struct drm_crtc *c)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(c);
>> +	struct drm_plane *plane;
>> +
>> +	drm_for_each_plane_mask(plane, crtc->base.dev,
>> +				crtc->atomic.force_disabled_planes)
>> +		to_intel_plane(plane)->disable_plane(plane, c);
>> +}
>> +
>>  static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_plane *intel_plane;
>>  	int pipe = intel_crtc->pipe;
>>  
>>  	intel_crtc_wait_for_pending_flips(crtc);
>> @@ -4842,14 +4851,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>>  	intel_pre_disable_primary(crtc);
>>  
>>  	intel_crtc_dpms_overlay_disable(intel_crtc);
>> -	for_each_intel_plane(dev, intel_plane) {
>> -		if (intel_plane->pipe == pipe) {
>> -			struct drm_crtc *from = intel_plane->base.crtc;
>> -
>> -			intel_plane->disable_plane(&intel_plane->base,
>> -						   from ?: crtc);
>> -		}
>> -	}
>> +	intel_disable_planes_on_crtc(crtc);
>>  
>>  	/*
>>  	 * FIXME: Once we grow proper nuclear flip support out of this we need
>> @@ -11554,6 +11556,21 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  	if (!intel_crtc->active || mode_changed)
>>  		return 0;
>>  
>> +	if (to_intel_crtc_state(crtc_state)->quirks &
>> +	    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> +		if (!plane_state->crtc) {
>> +			intel_crtc->atomic.force_disabled_planes |=
>> +				1 << drm_plane_index(plane);
>> +			crtc_state->plane_mask &=
>> +				~(1 << drm_plane_index(plane));
>> +		}
>> +
>> +		/* Initial state for sprite planes is unknown,
>> +		 * no need to update sprite watermarks */
>> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY)
>> +			mode_changed = true;
>> +	}
>> +
>>  	was_visible = old_plane_state->visible;
>>  	visible = to_intel_plane_state(plane_state)->visible;
>>  
>> @@ -11633,7 +11650,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  			intel_crtc->atomic.fb_bits |=
>>  			    INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe);
>>  
>> -		if (turn_off && is_crtc_enabled) {
>> +		if (turn_off && !mode_changed) {
>>  			intel_crtc->atomic.wait_vblank = true;
>>  			intel_crtc->atomic.update_sprite_watermarks |=
>>  				1 << i;
>> @@ -11714,6 +11731,11 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>>  		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>>  		idx, crtc->state->active, intel_crtc->active);
>>  
>> +	/* plane mask is fixed up after all initial planes are calculated */
>> +	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> +	if (mode_changed)
>> +		intel_crtc->atomic.force_disabled_planes = crtc->state->plane_mask;
>> +
>>  	if (mode_changed && crtc_state->enable &&
>>  	    dev_priv->display.crtc_compute_clock &&
>>  	    !WARN_ON(pipe_config->shared_dpll != DPLL_ID_PRIVATE)) {
>> @@ -12883,6 +12905,13 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>>  		return ret;
>>  
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		if (to_intel_crtc_state(crtc_state)->quirks &
>> +		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>> +			ret = drm_atomic_add_affected_planes(state, crtc);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>>  		if (!needs_modeset(crtc_state))
>>  			continue;
>>  
>> @@ -13529,13 +13558,17 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>  	intel_runtime_pm_get(dev_priv);
>>  
>>  	/* Perform vblank evasion around commit operation */
>> -	if (crtc->state->active && !needs_modeset(crtc->state))
>> +	if (crtc->state->active && !needs_modeset(crtc->state)) {
>>  		intel_crtc->atomic.evade =
>>  			intel_pipe_update_start(intel_crtc,
>>  						&intel_crtc->atomic.start_vbl_count);
>>  
>> +		if (intel_crtc->atomic.force_disabled_planes)
>> +			intel_disable_planes_on_crtc(crtc);
>> +	}
>> +
>>  	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>> -		skl_detach_scalers(intel_crtc);
>> +                skl_detach_scalers(intel_crtc);
> Unintentional change from tabs to spaces on this line?
Oops.

>>  }
>>  
>>  static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>> @@ -15040,14 +15073,63 @@ void i915_redisable_vga(struct drm_device *dev)
>>  	i915_redisable_vga_power_on(dev);
>>  }
>>  
>> -static bool primary_get_hw_state(struct intel_crtc *crtc)
>> +static bool intel_read_hw_plane_state(struct intel_crtc *crtc,
>> +				      struct intel_plane *intel_plane)
>>  {
>> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> -	if (!crtc->base.enabled)
>> -		return false;
>> +	switch (intel_plane->base.type) {
>> +	case DRM_PLANE_TYPE_PRIMARY:
>> +		return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> +
>> +	case DRM_PLANE_TYPE_CURSOR:
>> +		if (IS_845G(dev) || IS_I865G(dev))
>> +			return I915_READ(_CURACNTR) & CURSOR_ENABLE;
>> +		else
>> +			return I915_READ(CURCNTR(crtc->plane)) & CURSOR_MODE;
>> +
> If we're not trying to seamlessly inherit the cursor image, I'm not sure
> I understand what the benefit of figuring out whether the cursor is
> actually on or not is.  Just assuming true, as we do for sprites, would
> be enough to make the cursor always turn off, right?
Yeah, when reworking this patch to apply after modeset revert I fixed that, but kept cursor readout since I wrote it anyway.

Also got me rid of tracking force disabled planes. :-)
>> +	default:
>> +		return true;
>> +	}
>> +}
>> +
>> +static int readout_plane_state(struct drm_atomic_state *state,
>> +			       struct intel_crtc *crtc,
>> +			       struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_plane *p;
>> +	struct drm_plane_state *drm_plane_state;
>> +	bool active = crtc_state->base.active;
>> +
>> +	if (active) {
>> +		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>>  
>> -	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
>> +		/* apply to previous sw state too */
>> +		to_intel_crtc_state(crtc->base.state)->quirks |=
>> +			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
>> +	}
>> +
>> +	for_each_intel_plane(state->dev, p) {
>> +		if (crtc->plane != p->plane)
>> +			continue;
> Was this supposed to be comparing ->pipe rather than ->plane?  For
> primary and cursor planes I believe this works out okay since ->plane
> and ->pipe are basically the same (except for pre-gen4 FBC swapping),
> but for sprite planes, the ->plane pointer indicates whether it's the
> first, second, third, etc. sprite of that specific CRTC.
>
> As written, I think you'll only be operating on the first sprite plane
> of CRTC 0, the second sprite plane of CRTC 1, etc.
Ah thanks, that would explain it. :)

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

Re: [PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Maarten Lankhorst]

Op 11-06-15 om 03:35 schreef Matt Roper:
> On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote:
>> By passing crtc_state to the check_plane functions a lot of duplicated
>> code can be removed. And now that the transitional helpers are gone the
>> crtc_state can be reliably obtained.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
>>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
>>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
>>  drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
>>  4 files changed, 23 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index aa2128369a0a..4d8cacbca777 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>  	crtc = crtc ? crtc : plane->crtc;
>>  	intel_crtc = to_intel_crtc(crtc);
>>  
>> +	intel_state->visible = false;
>> +
> What do we need this change for?  Primary and cursor check functions
> immediately overwrite state->visible, so setting this here has no
> effect.  The sprite case where fb==NULL is the only case where this
> would matter, but moving the assignment from the sprite check function
> to here doesn't seem like it gains us anything.
>
I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible
should already be false, but a bit of paranoia never hunts. :-)

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

Re: [PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Maarten Lankhorst]

Op 15-06-15 om 14:05 schreef Daniel Vetter:
> On Thu, Jun 11, 2015 at 06:23:09AM +0200, Maarten Lankhorst wrote:
>> Op 11-06-15 om 03:35 schreef Matt Roper:
>>> On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote:
>>>> By passing crtc_state to the check_plane functions a lot of duplicated
>>>> code can be removed. And now that the transitional helpers are gone the
>>>> crtc_state can be reliably obtained.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
>>>>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
>>>>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
>>>>  drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
>>>>  4 files changed, 23 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> index aa2128369a0a..4d8cacbca777 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>>>  	crtc = crtc ? crtc : plane->crtc;
>>>>  	intel_crtc = to_intel_crtc(crtc);
>>>>  
>>>> +	intel_state->visible = false;
>>>> +
>>> What do we need this change for?  Primary and cursor check functions
>>> immediately overwrite state->visible, so setting this here has no
>>> effect.  The sprite case where fb==NULL is the only case where this
>>> would matter, but moving the assignment from the sprite check function
>>> to here doesn't seem like it gains us anything.
>>>
>> I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible
>> should already be false, but a bit of paranoia never hunts. :-)
> Resetting of state should be done in the duplicate functions. This makes
> sure we never ever forget to compute it correctly ;-) Sprinkling clearing
> code all over (and especially over the compute code) is imo fragile and
> should be avoided if possible.
> -Daniel
Good point!

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

Re: [PATCH v2 14/27] drm/i915: clean up atomic plane check functions

[Daniel Vetter]

On Thu, Jun 11, 2015 at 06:23:09AM +0200, Maarten Lankhorst wrote:
> Op 11-06-15 om 03:35 schreef Matt Roper:
> > On Thu, Jun 04, 2015 at 02:47:44PM +0200, Maarten Lankhorst wrote:
> >> By passing crtc_state to the check_plane functions a lot of duplicated
> >> code can be removed. And now that the transitional helpers are gone the
> >> crtc_state can be reliably obtained.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic_plane.c |  4 ++-
> >>  drivers/gpu/drm/i915/intel_display.c      | 48 ++++++++++---------------------
> >>  drivers/gpu/drm/i915/intel_drv.h          |  1 +
> >>  drivers/gpu/drm/i915/intel_sprite.c       | 13 +++------
> >>  4 files changed, 23 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index aa2128369a0a..4d8cacbca777 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -119,6 +119,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>  	crtc = crtc ? crtc : plane->crtc;
> >>  	intel_crtc = to_intel_crtc(crtc);
> >>  
> >> +	intel_state->visible = false;
> >> +
> > What do we need this change for?  Primary and cursor check functions
> > immediately overwrite state->visible, so setting this here has no
> > effect.  The sprite case where fb==NULL is the only case where this
> > would matter, but moving the assignment from the sprite check function
> > to here doesn't seem like it gains us anything.
> >
> I was using it to clear intel_state->visible even if no crtc is set. In that case state->visible
> should already be false, but a bit of paranoia never hunts. :-)

Resetting of state should be done in the duplicate functions. This makes
sure we never ever forget to compute it correctly ;-) Sprinkling clearing
code all over (and especially over the compute code) is imo fragile and
should be avoided if possible.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 18/27] drm/i915: Handle disabling planes better.

[Daniel Vetter]

On Thu, Jun 11, 2015 at 05:51:27AM +0200, Maarten Lankhorst wrote:
> Op 11-06-15 om 03:37 schreef Matt Roper:
> > If we're not trying to seamlessly inherit the cursor image, I'm not sure
> > I understand what the benefit of figuring out whether the cursor is
> > actually on or not is.  Just assuming true, as we do for sprites, would
> > be enough to make the cursor always turn off, right?
> Yeah, when reworking this patch to apply after modeset revert I fixed
> that, but kept cursor readout since I wrote it anyway.

Imo it's better to ditch anything but primary plane reconstruction to
avoid complications. We don't need anything else really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx