[PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Daniel Vetter]

This is the infrastructure for DPMS ported to the atomic world.
Fundamental changes compare to legacy DPMS are:

- No more per-connector dpms state, instead there's just one per each
  display pipeline. So if you clone either you have to unclone first
  if you only want to switch off one screen, or you just switch of
  everything (like all desktops do). This massively reduces complexity
  for cloning since now there's no more half-enabled cloned configs to
  consider.

- Only on/off, dpms standby/suspend are as dead as real CRTs. Again
  reduces complexity a lot.

Now especially for backwards compat the really important part for dpms
support is that dpms on always succeeds (except for hw death and
unplugged cables ofc). Which means everything that could fail (like
configuration checking, resources assignments and buffer management)
must be done irrespective from ->active. ->active is really only a
toggle to change the hardware state. More precisely:

- Drivers MUST NOT look at ->active in their ->atomic_check callbacks.
  Changes to ->active MUST always suceed if nothing else changes.

- Drivers using the atomic helpers MUST NOT look at ->active anywhere,
  period. The helpers will take care of calling the respective
  enable/modeset/disable hooks as necessary. As before the helpers
  will carefully keep track of the state and not call any hooks
  unecessarily, so still no double-disables or enables like with crtc
  helpers.

- ->mode_set hooks are only called when the mode or output
  configuration changes, not for changes in ->active state.

- Drivers which reconstruct the state objects in their ->reset hooks
  or through some other hw state readout infrastructure must ensure
  that ->active reflects actual hw state.

This just implements the core bits and helper logic, a subsequent
patch will implement the helper code to implement legacy dpms with
this.

v2: Rebase on top of the drm ioctl work:
- Move crtc checks to the core check function.
- Also check for ->active_changed when deciding whether a modeset
  might happen (for the ALLOW_MODESET mode).
- Expose the ->active state with an atomic prop.

v3: Review from Rob
- Spelling fix in comment.
- Extract needs_modeset helper to consolidate the ->mode_changed ||
  ->active_changed checks.

v4: Fixup fumble between crtc->state and crtc_state.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 18 +++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c | 54 +++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_crtc.c          | 10 +++++++
 include/drm/drm_crtc.h              |  3 +++
 4 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 1e38dfc8e462..ee68267bb326 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -241,7 +241,13 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		struct drm_crtc_state *state, struct drm_property *property,
 		uint64_t val)
 {
-	if (crtc->funcs->atomic_set_property)
+	struct drm_device *dev = crtc->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	/* FIXME: Mode prop is missing, which also controls ->enable. */
+	if (property == config->prop_active) {
+		state->active = val;
+	} else if (crtc->funcs->atomic_set_property)
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	return -EINVAL;
 }
@@ -282,6 +288,13 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 *
 	 * TODO: Add generic modeset state checks once we support those.
 	 */
+
+	if (state->active && !state->enable) {
+		DRM_DEBUG_KMS("[CRTC:%d] active without enabled\n",
+			      crtc->base.id);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -976,7 +989,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 			if (!crtc)
 				continue;
 
-			if (crtc_state->mode_changed) {
+			if (crtc_state->mode_changed ||
+			    crtc_state->active_changed) {
 				DRM_DEBUG_KMS("[CRTC:%d] requires full modeset\n",
 					      crtc->base.id);
 				return -EINVAL;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 541ba833ed36..3f17933b1d2c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -330,6 +330,12 @@ mode_fixup(struct drm_atomic_state *state)
 	return 0;
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 /**
  * drm_atomic_helper_check - validate state object for modeset changes
  * @dev: DRM device
@@ -404,12 +410,27 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		crtc = state->crtcs[i];
 		crtc_state = state->crtc_states[i];
 
-		if (!crtc || !crtc_state->mode_changed)
+		if (!crtc)
 			continue;
 
-		DRM_DEBUG_KMS("[CRTC:%d] needs full modeset, enable: %c\n",
+		/*
+		 * We must set ->active_changed after walking connectors for
+		 * otherwise an update that only changes active would result in
+		 * a full modeset because update_connector_routing force that.
+		 */
+		if (crtc->state->active != crtc_state->active) {
+			DRM_DEBUG_KMS("[CRTC:%d] active changed\n",
+				      crtc->base.id);
+			crtc_state->active_changed = true;
+		}
+
+		if (!needs_modeset(crtc_state))
+			continue;
+
+		DRM_DEBUG_KMS("[CRTC:%d] needs all connectors, enable: %c, active: %c\n",
 			      crtc->base.id,
-			      crtc_state->enable ? 'y' : 'n');
+			      crtc_state->enable ? 'y' : 'n',
+			      crtc_state->active ? 'y' : 'n');
 
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret != 0)
@@ -545,6 +566,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		struct drm_connector *connector;
 		struct drm_encoder_helper_funcs *funcs;
 		struct drm_encoder *encoder;
+		struct drm_crtc_state *old_crtc_state;
 
 		old_conn_state = old_state->connector_states[i];
 		connector = old_state->connectors[i];
@@ -554,6 +576,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (!old_conn_state || !old_conn_state->crtc)
 			continue;
 
+		old_crtc_state = old_state->crtc_states[drm_crtc_index(old_conn_state->crtc)];
+
+		if (!old_crtc_state->active)
+			continue;
+
 		encoder = old_conn_state->best_encoder;
 
 		/* We shouldn't get this far if we didn't previously have
@@ -586,11 +613,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 	for (i = 0; i < ncrtcs; i++) {
 		struct drm_crtc_helper_funcs *funcs;
 		struct drm_crtc *crtc;
+		struct drm_crtc_state *old_crtc_state;
 
 		crtc = old_state->crtcs[i];
+		old_crtc_state = old_state->crtc_states[i];
 
 		/* Shut down everything that needs a full modeset. */
-		if (!crtc || !crtc->state->mode_changed)
+		if (!crtc || !needs_modeset(crtc->state))
+			continue;
+
+		if (!old_crtc_state->active)
 			continue;
 
 		funcs = crtc->helper_private;
@@ -697,6 +729,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		mode = &new_crtc_state->mode;
 		adjusted_mode = &new_crtc_state->adjusted_mode;
 
+		if (!new_crtc_state->mode_changed)
+			continue;
+
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call call mode_set hooks twice.
@@ -749,7 +784,10 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 		crtc = old_state->crtcs[i];
 
 		/* Need to filter out CRTCs where only planes change. */
-		if (!crtc || !crtc->state->mode_changed)
+		if (!crtc || !needs_modeset(crtc->state))
+			continue;
+
+		if (!crtc->state->active)
 			continue;
 
 		funcs = crtc->helper_private;
@@ -768,6 +806,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 		if (!connector || !connector->state->best_encoder)
 			continue;
 
+		if (!connector->state->crtc->state->active)
+			continue;
+
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
 
@@ -1518,6 +1559,7 @@ retry:
 		WARN_ON(set->num_connectors);
 
 		crtc_state->enable = false;
+		crtc_state->active = false;
 
 		ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
 		if (ret != 0)
@@ -1532,6 +1574,7 @@ retry:
 	WARN_ON(!set->num_connectors);
 
 	crtc_state->enable = true;
+	crtc_state->active = true;
 	drm_mode_copy(&crtc_state->mode, set->mode);
 
 	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
@@ -1894,6 +1937,7 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
 
 	if (state) {
 		state->mode_changed = false;
+		state->active_changed = false;
 		state->planes_changed = false;
 		state->event = NULL;
 	}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 56e3256d5249..30a136b8a4fc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -691,6 +691,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	if (cursor)
 		cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
 
+	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
+		drm_object_attach_property(&crtc->base, config->prop_active, 0);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_crtc_init_with_planes);
@@ -1391,6 +1395,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.prop_crtc_id = prop;
 
+	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
+			"ACTIVE");
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.prop_active = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8022ab11958a..4d3f3b874dd6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -249,6 +249,7 @@ struct drm_atomic_state;
  * @enable: whether the CRTC should be enabled, gates all other state
  * @active: whether the CRTC is actively displaying (used for DPMS)
  * @mode_changed: for use by helpers and drivers when computing state updates
+ * @active_changed: for use by helpers and drivers when computing state updates
  * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
  * @last_vblank_count: for helpers and drivers to capture the vblank of the
  * 	update to ensure framebuffer cleanup isn't done too early
@@ -274,6 +275,7 @@ struct drm_crtc_state {
 	/* computed state bits used by helpers and drivers */
 	bool planes_changed : 1;
 	bool mode_changed : 1;
+	bool active_changed : 1;
 
 	/* attached planes bitmask:
 	 * WARNING: transitional helpers do not maintain plane_mask so
@@ -1128,6 +1130,7 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_h;
 	struct drm_property *prop_fb_id;
 	struct drm_property *prop_crtc_id;
+	struct drm_property *prop_active;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
-- 
2.1.4

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

[PATCH 2/5] drm/atomic-helper: add connector->dpms() implementation

[Daniel Vetter]

This builds on top of the crtc->active infrastructure to implement
legacy DPMS. My choice of semantics is somewhat arbitrary, but the
entire pipe is enabled as along as one output is still enabled.

Of course it also clamps everything that's not ON to OFF.

v2: Fix spelling in one comment.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 +
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3f17933b1d2c..736ab924ddc9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1887,6 +1887,81 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_page_flip);
 
 /**
+ * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
+ * @connector: affected connector
+ * @mode: DPMS mode
+ *
+ * This is the main helper function provided by the atomic helper framework for
+ * implementing the legacy DPMS connector interface. It computes the new desired
+ * ->active state for the corresponding CRTC (if the connector is enabled) and
+ *  updates it.
+ */
+void drm_atomic_helper_connector_dpms(struct drm_connector *connector,
+				      int mode)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_atomic_state *state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	struct drm_connector *tmp_connector;
+	int ret;
+	bool active = false;
+
+	if (mode != DRM_MODE_DPMS_ON)
+		mode = DRM_MODE_DPMS_OFF;
+
+	connector->dpms = mode;
+	crtc = connector->state->crtc;
+
+	if (!crtc)
+		return;
+
+	/* FIXME: ->dpms has no return value so can't forward the -ENOMEM. */
+	state = drm_atomic_state_alloc(connector->dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+retry:
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+
+	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
+
+	list_for_each_entry(tmp_connector, &config->connector_list, head) {
+		if (connector->state->crtc != crtc)
+			continue;
+
+		if (connector->dpms == DRM_MODE_DPMS_ON) {
+			active = true;
+			break;
+		}
+	}
+	crtc_state->active = active;
+
+	ret = drm_atomic_async_commit(state);
+	if (ret != 0)
+		goto fail;
+
+	/* Driver takes ownership of state on successful async commit. */
+	return;
+fail:
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	drm_atomic_state_free(state);
+
+	WARN(1, "Driver bug: Changing ->active failed with ret=%i\n", ret);
+
+	return;
+backoff:
+	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
+
+	goto retry;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
+
+/**
  * DOC: atomic state reset and initialization
  *
  * Both the drm core and the atomic helpers assume that there is always the full
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 2095917ff8c7..cf501df9e513 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -82,6 +82,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
 				uint32_t flags);
+void drm_atomic_helper_connector_dpms(struct drm_connector *connector,
+				      int mode);
 
 /* default implementations for state handling */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm/atomic-helpers: Recover full cursor plane behaviour

[Daniel Vetter]

Cursor plane updates have historically been fully async and mutliple
updates batched together for the next vsync. And userspace relies upon
that. Since implementing a full queue of async atomic updates is a bit
of work lets just recover the cursor specific behaviour with a hint
flag and some hacks to drop the vblank wait.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++
 include/drm/drm_crtc.h              |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 736ab924ddc9..a74a22db4dea 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -909,6 +909,11 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (!crtc->state->enable)
 			continue;
 
+		/* Legacy cursor ioctls are completely unsynced, and userspace
+		 * relies on that (by doing tons of cursor updates). */
+		if (old_state->legacy_cursor_update)
+			continue;
+
 		if (!framebuffer_changed(dev, old_state, crtc))
 			continue;
 
@@ -1335,6 +1340,9 @@ retry:
 	if (ret != 0)
 		goto fail;
 
+	if (plane == crtc->cursor)
+		state->legacy_cursor_update = true;
+
 	/* Driver takes ownership of state on successful commit. */
 	return 0;
 fail:
@@ -1410,6 +1418,9 @@ retry:
 	plane_state->src_h = 0;
 	plane_state->src_w = 0;
 
+	if (plane == plane->crtc->cursor)
+		state->legacy_cursor_update = true;
+
 	ret = drm_atomic_commit(state);
 	if (ret != 0)
 		goto fail;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4d3f3b874dd6..8b626ff3d3bd 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -929,6 +929,7 @@ struct drm_bridge {
  * struct struct drm_atomic_state - the global state object for atomic updates
  * @dev: parent DRM device
  * @allow_modeset: allow full modeset
+ * @allow_modeset: hint to enforce legacy cursor ioctl semantics
  * @planes: pointer to array of plane pointers
  * @plane_states: pointer to array of plane states pointers
  * @crtcs: pointer to array of CRTC pointers
@@ -941,6 +942,7 @@ struct drm_bridge {
 struct drm_atomic_state {
 	struct drm_device *dev;
 	bool allow_modeset : 1;
+	bool legacy_cursor_update : 1;
 	struct drm_plane **planes;
 	struct drm_plane_state **plane_states;
 	struct drm_crtc **crtcs;
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] drm/atomic-helpers: Saner encoder/crtc callbacks

[Daniel Vetter]

For historical reasons going all the way back to how the Xrandr code
was implemented the semantics of the callbacks used to enable/disable
crtcs and encoders are ... interesting.

But with atomic helpers all that complexity has been binned, with only
a well-defined on/off action left. Unfortunately the names stuck.

Let's fix that by adding enable/disable hooks every, make them the
preferred variant for atomic and update documentations.

Later on we add debug warnings when drivers have deprecated hooks. But
while everything is in-flight with lots of drivers converting to
atomic that's a bit too much - better wait for things to settle a bit
first.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++++++-----
 include/drm/drm_crtc_helper.h       | 20 ++++++++++++++++++--
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a74a22db4dea..b67754035fec 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -599,7 +599,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 			encoder->bridge->funcs->disable(encoder->bridge);
 
 		/* Right function depends upon target state. */
-		if (connector->state->crtc)
+		if (connector->state->crtc && funcs->prepare)
 			funcs->prepare(encoder);
 		else if (funcs->disable)
 			funcs->disable(encoder);
@@ -628,7 +628,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		funcs = crtc->helper_private;
 
 		/* Right function depends upon target state. */
-		if (crtc->state->enable)
+		if (crtc->state->enable && funcs->prepare)
 			funcs->prepare(crtc);
 		else if (funcs->disable)
 			funcs->disable(crtc);
@@ -792,8 +792,12 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 
 		funcs = crtc->helper_private;
 
-		if (crtc->state->enable)
-			funcs->commit(crtc);
+		if (crtc->state->enable) {
+			if (funcs->enable)
+				funcs->enable(crtc);
+			else
+				funcs->commit(crtc);
+		}
 	}
 
 	for (i = 0; i < old_state->num_connector; i++) {
@@ -819,7 +823,10 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 		if (encoder->bridge)
 			encoder->bridge->funcs->pre_enable(encoder->bridge);
 
-		funcs->commit(encoder);
+		if (funcs->enable)
+			funcs->enable(encoder);
+		else
+			funcs->commit(encoder);
 
 		if (encoder->bridge)
 			encoder->bridge->funcs->enable(encoder->bridge);
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index e76828d81a8b..c36f1bc26837 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -58,11 +58,19 @@ enum mode_set_atomic {
  * @mode_set_base_atomic: non-blocking mode set (used for kgdb support)
  * @load_lut: load color palette
  * @disable: disable CRTC when no longer in use
+ * @disable: enable CRTC
  * @atomic_check: check for validity of an atomic state
  * @atomic_begin: begin atomic update
  * @atomic_flush: flush atomic update
  *
  * The helper operations are called by the mid-layer CRTC helper.
+ *
+ * Note that with atomic helpers @dpms, @prepare and @commit hooks are
+ * deprecated. Used @enable and @disable instead exclusively.
+ *
+ * With legacy crtc helpers there's a big semantic difference between @disable
+ * and the other hooks: @disable also needs to release any resources acquired in
+ * @mode_set (like shared PLLs).
  */
 struct drm_crtc_helper_funcs {
 	/*
@@ -93,8 +101,8 @@ struct drm_crtc_helper_funcs {
 	/* reload the current crtc LUT */
 	void (*load_lut)(struct drm_crtc *crtc);
 
-	/* disable crtc when not in use - more explicit than dpms off */
 	void (*disable)(struct drm_crtc *crtc);
+	void (*enable)(struct drm_crtc *crtc);
 
 	/* atomic helpers */
 	int (*atomic_check)(struct drm_crtc *crtc,
@@ -115,8 +123,16 @@ struct drm_crtc_helper_funcs {
  * @get_crtc: return CRTC that the encoder is currently attached to
  * @detect: connection status detection
  * @disable: disable encoder when not in use (overrides DPMS off)
+ * @disable: enable encoder
  *
  * The helper operations are called by the mid-layer CRTC helper.
+ *
+ * Note that with atomic helpers @dpms, @prepare and @commit hooks are
+ * deprecated. Used @enable and @disable instead exclusively.
+ *
+ * With legacy crtc helpers there's a big semantic difference between @disable
+ * and the other hooks: @disable also needs to release any resources acquired in
+ * @mode_set (like shared PLLs).
  */
 struct drm_encoder_helper_funcs {
 	void (*dpms)(struct drm_encoder *encoder, int mode);
@@ -135,8 +151,8 @@ struct drm_encoder_helper_funcs {
 	/* detect for DAC style encoders */
 	enum drm_connector_status (*detect)(struct drm_encoder *encoder,
 					    struct drm_connector *connector);
-	/* disable encoder when not in use - more explicit than dpms off */
 	void (*disable)(struct drm_encoder *encoder);
+	void (*enable)(struct drm_encoder *encoder);
 };
 
 /**
-- 
2.1.4

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

[PATCH 5/5] drm/atomic-helper: debug output for modesets

[Daniel Vetter]

With the combination of ->enable and ->active it's a bit complicated
to follow what exactly is going on sometimes within a full modeset.
Add debug output to make this all traceable.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b67754035fec..b7ac83555b11 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -591,6 +591,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = encoder->helper_private;
 
+		DRM_DEBUG_KMS("disabling [ENCODER:%d:%s]\n",
+			      encoder->base.id, encoder->name);
+
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call call disable hooks twice.
@@ -627,6 +630,10 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
+		DRM_DEBUG_KMS("disabling [CRTC:%d]\n",
+			      crtc->base.id);
+
+
 		/* Right function depends upon target state. */
 		if (crtc->state->enable && funcs->prepare)
 			funcs->prepare(crtc);
@@ -707,8 +714,12 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		if (crtc->state->enable)
+		if (crtc->state->enable) {
+			DRM_DEBUG_KMS("modeset on [CRTC:%d]\n",
+				      crtc->base.id);
+
 			funcs->mode_set_nofb(crtc);
+		}
 	}
 
 	for (i = 0; i < old_state->num_connector; i++) {
@@ -732,6 +743,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (!new_crtc_state->mode_changed)
 			continue;
 
+		DRM_DEBUG_KMS("modeset on [ENCODER:%d:%s]\n",
+			      encoder->base.id, encoder->name);
+
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call call mode_set hooks twice.
@@ -793,6 +807,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 		funcs = crtc->helper_private;
 
 		if (crtc->state->enable) {
+			DRM_DEBUG_KMS("enabling [CRTC:%d]\n",
+				      crtc->base.id);
+
 			if (funcs->enable)
 				funcs->enable(crtc);
 			else
@@ -816,6 +833,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
 		encoder = connector->state->best_encoder;
 		funcs = encoder->helper_private;
 
+		DRM_DEBUG_KMS("enabling [ENCODER:%d:%s]\n",
+			      encoder->base.id, encoder->name);
+
 		/*
 		 * Each encoder has at most one connector (since we always steal
 		 * it away), so we won't call call enable hooks twice.
-- 
2.1.4

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

Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Ville Syrjälä]

On Thu, Jan 22, 2015 at 04:36:21PM +0100, Daniel Vetter wrote:
> This is the infrastructure for DPMS ported to the atomic world.
> Fundamental changes compare to legacy DPMS are:
> 
> - No more per-connector dpms state, instead there's just one per each
>   display pipeline. So if you clone either you have to unclone first
>   if you only want to switch off one screen, or you just switch of
>   everything (like all desktops do). This massively reduces complexity
>   for cloning since now there's no more half-enabled cloned configs to
>   consider.
> 
> - Only on/off, dpms standby/suspend are as dead as real CRTs. Again
>   reduces complexity a lot.
> 
> Now especially for backwards compat the really important part for dpms
> support is that dpms on always succeeds (except for hw death and
> unplugged cables ofc). Which means everything that could fail (like
> configuration checking, resources assignments and buffer management)
> must be done irrespective from ->active. ->active is really only a
> toggle to change the hardware state. More precisely:
> 
> - Drivers MUST NOT look at ->active in their ->atomic_check callbacks.
>   Changes to ->active MUST always suceed if nothing else changes.
> 
> - Drivers using the atomic helpers MUST NOT look at ->active anywhere,
>   period. The helpers will take care of calling the respective
>   enable/modeset/disable hooks as necessary. As before the helpers
>   will carefully keep track of the state and not call any hooks
>   unecessarily, so still no double-disables or enables like with crtc
>   helpers.
> 
> - ->mode_set hooks are only called when the mode or output
>   configuration changes, not for changes in ->active state.
> 
> - Drivers which reconstruct the state objects in their ->reset hooks
>   or through some other hw state readout infrastructure must ensure
>   that ->active reflects actual hw state.
> 
> This just implements the core bits and helper logic, a subsequent
> patch will implement the helper code to implement legacy dpms with
> this.

So we now have multiple conflicting properties for the same thing? I
don't particularly relish that idea.

I suppose a rather easy way to way to deal with that would be to hide
the dpms properties from non-atomic clients, and conversly hide the
active property from legacy clients.

> 
> v2: Rebase on top of the drm ioctl work:
> - Move crtc checks to the core check function.
> - Also check for ->active_changed when deciding whether a modeset
>   might happen (for the ALLOW_MODESET mode).
> - Expose the ->active state with an atomic prop.
> 
> v3: Review from Rob
> - Spelling fix in comment.
> - Extract needs_modeset helper to consolidate the ->mode_changed ||
>   ->active_changed checks.
> 
> v4: Fixup fumble between crtc->state and crtc_state.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 18 +++++++++++--
>  drivers/gpu/drm/drm_atomic_helper.c | 54 +++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_crtc.c          | 10 +++++++
>  include/drm/drm_crtc.h              |  3 +++
>  4 files changed, 78 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 1e38dfc8e462..ee68267bb326 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -241,7 +241,13 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state, struct drm_property *property,
>  		uint64_t val)
>  {
> -	if (crtc->funcs->atomic_set_property)
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	/* FIXME: Mode prop is missing, which also controls ->enable. */
> +	if (property == config->prop_active) {
> +		state->active = val;
> +	} else if (crtc->funcs->atomic_set_property)
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	return -EINVAL;
>  }
> @@ -282,6 +288,13 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 *
>  	 * TODO: Add generic modeset state checks once we support those.
>  	 */
> +
> +	if (state->active && !state->enable) {
> +		DRM_DEBUG_KMS("[CRTC:%d] active without enabled\n",
> +			      crtc->base.id);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -976,7 +989,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  			if (!crtc)
>  				continue;
>  
> -			if (crtc_state->mode_changed) {
> +			if (crtc_state->mode_changed ||
> +			    crtc_state->active_changed) {
>  				DRM_DEBUG_KMS("[CRTC:%d] requires full modeset\n",
>  					      crtc->base.id);
>  				return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 541ba833ed36..3f17933b1d2c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -330,6 +330,12 @@ mode_fixup(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool
> +needs_modeset(struct drm_crtc_state *state)
> +{
> +	return state->mode_changed || state->active_changed;
> +}
> +
>  /**
>   * drm_atomic_helper_check - validate state object for modeset changes
>   * @dev: DRM device
> @@ -404,12 +410,27 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		crtc = state->crtcs[i];
>  		crtc_state = state->crtc_states[i];
>  
> -		if (!crtc || !crtc_state->mode_changed)
> +		if (!crtc)
>  			continue;
>  
> -		DRM_DEBUG_KMS("[CRTC:%d] needs full modeset, enable: %c\n",
> +		/*
> +		 * We must set ->active_changed after walking connectors for
> +		 * otherwise an update that only changes active would result in
> +		 * a full modeset because update_connector_routing force that.
> +		 */
> +		if (crtc->state->active != crtc_state->active) {
> +			DRM_DEBUG_KMS("[CRTC:%d] active changed\n",
> +				      crtc->base.id);
> +			crtc_state->active_changed = true;
> +		}
> +
> +		if (!needs_modeset(crtc_state))
> +			continue;
> +
> +		DRM_DEBUG_KMS("[CRTC:%d] needs all connectors, enable: %c, active: %c\n",
>  			      crtc->base.id,
> -			      crtc_state->enable ? 'y' : 'n');
> +			      crtc_state->enable ? 'y' : 'n',
> +			      crtc_state->active ? 'y' : 'n');
>  
>  		ret = drm_atomic_add_affected_connectors(state, crtc);
>  		if (ret != 0)
> @@ -545,6 +566,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		struct drm_connector *connector;
>  		struct drm_encoder_helper_funcs *funcs;
>  		struct drm_encoder *encoder;
> +		struct drm_crtc_state *old_crtc_state;
>  
>  		old_conn_state = old_state->connector_states[i];
>  		connector = old_state->connectors[i];
> @@ -554,6 +576,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		if (!old_conn_state || !old_conn_state->crtc)
>  			continue;
>  
> +		old_crtc_state = old_state->crtc_states[drm_crtc_index(old_conn_state->crtc)];
> +
> +		if (!old_crtc_state->active)
> +			continue;
> +
>  		encoder = old_conn_state->best_encoder;
>  
>  		/* We shouldn't get this far if we didn't previously have
> @@ -586,11 +613,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  	for (i = 0; i < ncrtcs; i++) {
>  		struct drm_crtc_helper_funcs *funcs;
>  		struct drm_crtc *crtc;
> +		struct drm_crtc_state *old_crtc_state;
>  
>  		crtc = old_state->crtcs[i];
> +		old_crtc_state = old_state->crtc_states[i];
>  
>  		/* Shut down everything that needs a full modeset. */
> -		if (!crtc || !crtc->state->mode_changed)
> +		if (!crtc || !needs_modeset(crtc->state))
> +			continue;
> +
> +		if (!old_crtc_state->active)
>  			continue;
>  
>  		funcs = crtc->helper_private;
> @@ -697,6 +729,9 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		mode = &new_crtc_state->mode;
>  		adjusted_mode = &new_crtc_state->adjusted_mode;
>  
> +		if (!new_crtc_state->mode_changed)
> +			continue;
> +
>  		/*
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call call mode_set hooks twice.
> @@ -749,7 +784,10 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>  		crtc = old_state->crtcs[i];
>  
>  		/* Need to filter out CRTCs where only planes change. */
> -		if (!crtc || !crtc->state->mode_changed)
> +		if (!crtc || !needs_modeset(crtc->state))
> +			continue;
> +
> +		if (!crtc->state->active)
>  			continue;
>  
>  		funcs = crtc->helper_private;
> @@ -768,6 +806,9 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
>  		if (!connector || !connector->state->best_encoder)
>  			continue;
>  
> +		if (!connector->state->crtc->state->active)
> +			continue;
> +
>  		encoder = connector->state->best_encoder;
>  		funcs = encoder->helper_private;
>  
> @@ -1518,6 +1559,7 @@ retry:
>  		WARN_ON(set->num_connectors);
>  
>  		crtc_state->enable = false;
> +		crtc_state->active = false;
>  
>  		ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
>  		if (ret != 0)
> @@ -1532,6 +1574,7 @@ retry:
>  	WARN_ON(!set->num_connectors);
>  
>  	crtc_state->enable = true;
> +	crtc_state->active = true;
>  	drm_mode_copy(&crtc_state->mode, set->mode);
>  
>  	ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
> @@ -1894,6 +1937,7 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	if (state) {
>  		state->mode_changed = false;
> +		state->active_changed = false;
>  		state->planes_changed = false;
>  		state->event = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 56e3256d5249..30a136b8a4fc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -691,6 +691,10 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  	if (cursor)
>  		cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>  
> +	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> +		drm_object_attach_property(&crtc->base, config->prop_active, 0);
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_crtc_init_with_planes);
> @@ -1391,6 +1395,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_crtc_id = prop;
>  
> +	prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
> +			"ACTIVE");
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_active = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8022ab11958a..4d3f3b874dd6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -249,6 +249,7 @@ struct drm_atomic_state;
>   * @enable: whether the CRTC should be enabled, gates all other state
>   * @active: whether the CRTC is actively displaying (used for DPMS)
>   * @mode_changed: for use by helpers and drivers when computing state updates
> + * @active_changed: for use by helpers and drivers when computing state updates
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
>   * @last_vblank_count: for helpers and drivers to capture the vblank of the
>   * 	update to ensure framebuffer cleanup isn't done too early
> @@ -274,6 +275,7 @@ struct drm_crtc_state {
>  	/* computed state bits used by helpers and drivers */
>  	bool planes_changed : 1;
>  	bool mode_changed : 1;
> +	bool active_changed : 1;
>  
>  	/* attached planes bitmask:
>  	 * WARNING: transitional helpers do not maintain plane_mask so
> @@ -1128,6 +1130,7 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_h;
>  	struct drm_property *prop_fb_id;
>  	struct drm_property *prop_crtc_id;
> +	struct drm_property *prop_active;
>  
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Daniel Vetter]

On Thu, Jan 22, 2015 at 05:56:54PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 22, 2015 at 04:36:21PM +0100, Daniel Vetter wrote:
> > This is the infrastructure for DPMS ported to the atomic world.
> > Fundamental changes compare to legacy DPMS are:
> > 
> > - No more per-connector dpms state, instead there's just one per each
> >   display pipeline. So if you clone either you have to unclone first
> >   if you only want to switch off one screen, or you just switch of
> >   everything (like all desktops do). This massively reduces complexity
> >   for cloning since now there's no more half-enabled cloned configs to
> >   consider.
> > 
> > - Only on/off, dpms standby/suspend are as dead as real CRTs. Again
> >   reduces complexity a lot.
> > 
> > Now especially for backwards compat the really important part for dpms
> > support is that dpms on always succeeds (except for hw death and
> > unplugged cables ofc). Which means everything that could fail (like
> > configuration checking, resources assignments and buffer management)
> > must be done irrespective from ->active. ->active is really only a
> > toggle to change the hardware state. More precisely:
> > 
> > - Drivers MUST NOT look at ->active in their ->atomic_check callbacks.
> >   Changes to ->active MUST always suceed if nothing else changes.
> > 
> > - Drivers using the atomic helpers MUST NOT look at ->active anywhere,
> >   period. The helpers will take care of calling the respective
> >   enable/modeset/disable hooks as necessary. As before the helpers
> >   will carefully keep track of the state and not call any hooks
> >   unecessarily, so still no double-disables or enables like with crtc
> >   helpers.
> > 
> > - ->mode_set hooks are only called when the mode or output
> >   configuration changes, not for changes in ->active state.
> > 
> > - Drivers which reconstruct the state objects in their ->reset hooks
> >   or through some other hw state readout infrastructure must ensure
> >   that ->active reflects actual hw state.
> > 
> > This just implements the core bits and helper logic, a subsequent
> > patch will implement the helper code to implement legacy dpms with
> > this.
> 
> So we now have multiple conflicting properties for the same thing? I
> don't particularly relish that idea.
> 
> I suppose a rather easy way to way to deal with that would be to hide
> the dpms properties from non-atomic clients, and conversly hide the
> active property from legacy clients.

Which is pretty much what I do - you can only access the per-crtc ACTIVE
property from the atomic ioctl, the per-connector dpms property is _not_
exposed through atomic. Vice-versa legacy clients wont see atomic
properties (and hence this new one) either.

Is that good enough?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Ville Syrjälä]

On Thu, Jan 22, 2015 at 05:15:26PM +0100, Daniel Vetter wrote:
> On Thu, Jan 22, 2015 at 05:56:54PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 22, 2015 at 04:36:21PM +0100, Daniel Vetter wrote:
> > > This is the infrastructure for DPMS ported to the atomic world.
> > > Fundamental changes compare to legacy DPMS are:
> > > 
> > > - No more per-connector dpms state, instead there's just one per each
> > >   display pipeline. So if you clone either you have to unclone first
> > >   if you only want to switch off one screen, or you just switch of
> > >   everything (like all desktops do). This massively reduces complexity
> > >   for cloning since now there's no more half-enabled cloned configs to
> > >   consider.
> > > 
> > > - Only on/off, dpms standby/suspend are as dead as real CRTs. Again
> > >   reduces complexity a lot.
> > > 
> > > Now especially for backwards compat the really important part for dpms
> > > support is that dpms on always succeeds (except for hw death and
> > > unplugged cables ofc). Which means everything that could fail (like
> > > configuration checking, resources assignments and buffer management)
> > > must be done irrespective from ->active. ->active is really only a
> > > toggle to change the hardware state. More precisely:
> > > 
> > > - Drivers MUST NOT look at ->active in their ->atomic_check callbacks.
> > >   Changes to ->active MUST always suceed if nothing else changes.
> > > 
> > > - Drivers using the atomic helpers MUST NOT look at ->active anywhere,
> > >   period. The helpers will take care of calling the respective
> > >   enable/modeset/disable hooks as necessary. As before the helpers
> > >   will carefully keep track of the state and not call any hooks
> > >   unecessarily, so still no double-disables or enables like with crtc
> > >   helpers.
> > > 
> > > - ->mode_set hooks are only called when the mode or output
> > >   configuration changes, not for changes in ->active state.
> > > 
> > > - Drivers which reconstruct the state objects in their ->reset hooks
> > >   or through some other hw state readout infrastructure must ensure
> > >   that ->active reflects actual hw state.
> > > 
> > > This just implements the core bits and helper logic, a subsequent
> > > patch will implement the helper code to implement legacy dpms with
> > > this.
> > 
> > So we now have multiple conflicting properties for the same thing? I
> > don't particularly relish that idea.
> > 
> > I suppose a rather easy way to way to deal with that would be to hide
> > the dpms properties from non-atomic clients, and conversly hide the
> > active property from legacy clients.
> 
> Which is pretty much what I do - you can only access the per-crtc ACTIVE
> property from the atomic ioctl, the per-connector dpms property is _not_
> exposed through atomic. Vice-versa legacy clients wont see atomic
> properties (and hence this new one) either.

Oh, OK. I didn't see anything obvious that would filter out the dpms
prop for non-atomic, nor do I see code to reject stuff in setprop/getprop
ioctls etc. But I suppose such stuff may be in flight and I've just not
paid attention.

> Is that good enough?

I suppose.

Another thing that came to mind wrt. the 'this can't fail rule' was
fb pinning. So is the rule now going to be that we need to pin even
when ACTIVE==false, or otherwise make sure all the FBs can be pinned
simultaneosly? If we don't want to everything pinned all the time when
ACTIVE==false, then we would need to prevent pinning of anything else
in the meantime to make sure we don't run out of address space.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Daniel Vetter]

On Thu, Jan 22, 2015 at 5:42 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> Which is pretty much what I do - you can only access the per-crtc ACTIVE
>> property from the atomic ioctl, the per-connector dpms property is _not_
>> exposed through atomic. Vice-versa legacy clients wont see atomic
>> properties (and hence this new one) either.
>
> Oh, OK. I didn't see anything obvious that would filter out the dpms
> prop for non-atomic, nor do I see code to reject stuff in setprop/getprop
> ioctls etc. But I suppose such stuff may be in flight and I've just not
> paid attention.

On the atomic side the dpms prop is simple not decoded. The driver
/could/ do that itself, but that would be really pointless. In
getprops atomic ioctls are filtered out for non-atomic clients. Which
means that an atomic client could do a dpms on the connector through
the legacy setprop ioctl, but that would be rather stupid.

>> Is that good enough?
>
> I suppose.
>
> Another thing that came to mind wrt. the 'this can't fail rule' was
> fb pinning. So is the rule now going to be that we need to pin even
> when ACTIVE==false, or otherwise make sure all the FBs can be pinned
> simultaneosly? If we don't want to everything pinned all the time when
> ACTIVE==false, then we would need to prevent pinning of anything else
> in the meantime to make sure we don't run out of address space.

fb pinning is done irrespective of the state of active. So if you
update the fb while the display pipe is off the helpers will upin/pin
correctly. Imo that's totally fine, since failing to pin when setting
the display back to active really isn't a great thing. And we need to
be able to tell userspace when something has gone wrong with the
pinning (e.g. due to giant framebuffer or something).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic-helper: add connector->dpms() implementation

[Daniel Vetter]

This builds on top of the crtc->active infrastructure to implement
legacy DPMS. My choice of semantics is somewhat arbitrary, but the
entire pipe is enabled as along as one output is still enabled.

Of course it also clamps everything that's not ON to OFF.

v2: Fix spelling in one comment.

v3: Don't do an async commit (Thierry)

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 75 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 +
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3f17933b1d2c..f693344d9573 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1887,6 +1887,81 @@ backoff:
 EXPORT_SYMBOL(drm_atomic_helper_page_flip);
 
 /**
+ * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
+ * @connector: affected connector
+ * @mode: DPMS mode
+ *
+ * This is the main helper function provided by the atomic helper framework for
+ * implementing the legacy DPMS connector interface. It computes the new desired
+ * ->active state for the corresponding CRTC (if the connector is enabled) and
+ *  updates it.
+ */
+void drm_atomic_helper_connector_dpms(struct drm_connector *connector,
+				      int mode)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	struct drm_atomic_state *state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	struct drm_connector *tmp_connector;
+	int ret;
+	bool active = false;
+
+	if (mode != DRM_MODE_DPMS_ON)
+		mode = DRM_MODE_DPMS_OFF;
+
+	connector->dpms = mode;
+	crtc = connector->state->crtc;
+
+	if (!crtc)
+		return;
+
+	/* FIXME: ->dpms has no return value so can't forward the -ENOMEM. */
+	state = drm_atomic_state_alloc(connector->dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
+retry:
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+
+	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
+
+	list_for_each_entry(tmp_connector, &config->connector_list, head) {
+		if (connector->state->crtc != crtc)
+			continue;
+
+		if (connector->dpms == DRM_MODE_DPMS_ON) {
+			active = true;
+			break;
+		}
+	}
+	crtc_state->active = active;
+
+	ret = drm_atomic_commit(state);
+	if (ret != 0)
+		goto fail;
+
+	/* Driver takes ownership of state on successful async commit. */
+	return;
+fail:
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	drm_atomic_state_free(state);
+
+	WARN(1, "Driver bug: Changing ->active failed with ret=%i\n", ret);
+
+	return;
+backoff:
+	drm_atomic_state_clear(state);
+	drm_atomic_legacy_backoff(state);
+
+	goto retry;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_dpms);
+
+/**
  * DOC: atomic state reset and initialization
  *
  * Both the drm core and the atomic helpers assume that there is always the full
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 2095917ff8c7..cf501df9e513 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -82,6 +82,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				struct drm_pending_vblank_event *event,
 				uint32_t flags);
+void drm_atomic_helper_connector_dpms(struct drm_connector *connector,
+				      int mode);
 
 /* default implementations for state handling */
 void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
-- 
2.1.4

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

Re: [PATCH 1/5] drm/atomic: Add drm_crtc_state->active

[Ville Syrjälä]

On Thu, Jan 22, 2015 at 06:38:32PM +0100, Daniel Vetter wrote:
> On Thu, Jan 22, 2015 at 5:42 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> Which is pretty much what I do - you can only access the per-crtc ACTIVE
> >> property from the atomic ioctl, the per-connector dpms property is _not_
> >> exposed through atomic. Vice-versa legacy clients wont see atomic
> >> properties (and hence this new one) either.
> >
> > Oh, OK. I didn't see anything obvious that would filter out the dpms
> > prop for non-atomic, nor do I see code to reject stuff in setprop/getprop
> > ioctls etc. But I suppose such stuff may be in flight and I've just not
> > paid attention.
> 
> On the atomic side the dpms prop is simple not decoded. The driver
> /could/ do that itself, but that would be really pointless. In
> getprops atomic ioctls are filtered out for non-atomic clients. Which
> means that an atomic client could do a dpms on the connector through
> the legacy setprop ioctl, but that would be rather stupid.

Well I'd rather it refused the entire thing. Less weird interactions to
worry about if we're strict and block all the silly stuff at the top.

> 
> >> Is that good enough?
> >
> > I suppose.
> >
> > Another thing that came to mind wrt. the 'this can't fail rule' was
> > fb pinning. So is the rule now going to be that we need to pin even
> > when ACTIVE==false, or otherwise make sure all the FBs can be pinned
> > simultaneosly? If we don't want to everything pinned all the time when
> > ACTIVE==false, then we would need to prevent pinning of anything else
> > in the meantime to make sure we don't run out of address space.
> 
> fb pinning is done irrespective of the state of active. So if you
> update the fb while the display pipe is off the helpers will upin/pin
> correctly. Imo that's totally fine, since failing to pin when setting
> the display back to active really isn't a great thing. And we need to
> be able to tell userspace when something has gone wrong with the
> pinning (e.g. due to giant framebuffer or something).

Yeah that was pretty much my original idea too. I suppose now that's the
call comes from the core it's a bit less likely to be messed up again.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic-helper: add connector->dpms() implementation

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5629
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  353/353              353/353
SNB              +1                 399/422              400/422
IVB                                  486/487              486/487
BYT                                  296/296              296/296
HSW              +1-3              507/508              505/508
BDW              +1                 399/402              400/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_flip_nonexisting-fb      NSPT(1, M35)PASS(2, M35)      PASS(1, M35)
*HSW  igt_gem_pwrite_pread_display-copy-performance      PASS(2, M19M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_pwrite_pread_snooped-copy-performance      PASS(2, M19M20)      DMESG_WARN(1, M20)
*HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      PASS(2, M19M20)      DMESG_WARN(1, M20)
 HSW  igt_gem_storedw_loop_blt      DMESG_WARN(2, M19M20)PASS(1, M20)      PASS(1, M20)
 BDW  igt_gem_pwrite_pread_display-pwrite-blt-gtt_mmap-performance      DMESG_WARN(2, M28)PASS(1, M30)      PASS(1, M30)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx