[PATCH 0/4] drm/atomic: Cleanups for adding connector atomic check function.

[PATCH 0/4] drm/atomic: Cleanups for adding connector atomic check function.

[Maarten Lankhorst]

Some small cleanups I came across to make drm_atomic_helper_check_modeset
more readable.

This makes it a lot more clear what atomic_check does and why it's called in
the place it is.

Maarten Lankhorst (4):
  drm/atomic: Unify conflicting encoder handling.
  drm/atomic: Set all the changed flags in one place.
  drm/atomic: Move enable/connector check up in check_modeset()
  drm/atomic: Add connector atomic_check function, v2.

 drivers/gpu/drm/drm_atomic_helper.c      | 84 ++++++++++++++++++++++----------
 include/drm/drm_atomic.h                 |  2 -
 include/drm/drm_modeset_helper_vtables.h | 34 +++++++++++++
 3 files changed, 93 insertions(+), 27 deletions(-)

-- 
2.7.4

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

[PATCH 1/4] drm/atomic: Unify conflicting encoder handling.

[Maarten Lankhorst]

Currently we use a flag to change behavior in atomic commit
whether a conflicting encoder should be enabled or disabled.
This is used for the legacy set_config helper, which disables
connectors that have a conflicting encoder but not part of the
active crtc list.

There's no need for this to be handled in atomic commit, it
could be done in the set_config helper instead. This will
let the atomic check function reject any conflicting encoders,
while set_config can disable conflicting crtc's. This makes it
possible to recalculate the changed flags in 1 loop.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
 include/drm/drm_atomic.h            | 2 --
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c3994b4d5f32..17f557f8eca6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -517,7 +517,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		}
 	}
 
-	ret = handle_conflicting_encoders(state, state->legacy_set_config);
+	ret = handle_conflicting_encoders(state, false);
 	if (ret)
 		return ret;
 
@@ -2289,12 +2289,15 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set,
 	if (!state)
 		return -ENOMEM;
 
-	state->legacy_set_config = true;
 	state->acquire_ctx = ctx;
 	ret = __drm_atomic_helper_set_config(set, state);
 	if (ret != 0)
 		goto fail;
 
+	ret = handle_conflicting_encoders(state, true);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_commit(state);
 
 fail:
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index fd33ed5eaeb4..788daf756f48 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -160,7 +160,6 @@ struct __drm_connnectors_state {
  * @dev: parent DRM device
  * @allow_modeset: allow full modeset
  * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
- * @legacy_set_config: Disable conflicting encoders instead of failing with -EINVAL.
  * @planes: pointer to array of structures with per-plane data
  * @crtcs: pointer to array of CRTC pointers
  * @num_connector: size of the @connectors and @connector_states arrays
@@ -173,7 +172,6 @@ struct drm_atomic_state {
 	struct drm_device *dev;
 	bool allow_modeset : 1;
 	bool legacy_cursor_update : 1;
-	bool legacy_set_config : 1;
 	struct __drm_planes_state *planes;
 	struct __drm_crtcs_state *crtcs;
 	int num_connector;
-- 
2.7.4

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

[PATCH 2/4] drm/atomic: Set all the changed flags in one place.

[Maarten Lankhorst]

Now that handle_conflicting_encoders cannot disable crtc's any more
it makes sense to set all the changed flags in 1 place.
This makes the code slightly less magical.

The (now removed) comment is out of date. The only reason the
active_changed was set late was because handle_conflicting_encoders
could disable connectors. This is no longer the case,
and we can put everything in 1 place.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 17f557f8eca6..d267f45d2c9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -515,6 +515,12 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			new_crtc_state->mode_changed = true;
 			new_crtc_state->connectors_changed = true;
 		}
+
+		if (old_crtc_state->active != new_crtc_state->active) {
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active changed\n",
+					 crtc->base.id, crtc->name);
+			new_crtc_state->active_changed = true;
+		}
 	}
 
 	ret = handle_conflicting_encoders(state, false);
@@ -551,17 +557,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		bool has_connectors =
 			!!new_crtc_state->connector_mask;
 
-		/*
-		 * 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 (old_crtc_state->active != new_crtc_state->active) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active changed\n",
-					 crtc->base.id, crtc->name);
-			new_crtc_state->active_changed = true;
-		}
-
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
-- 
2.7.4

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

[PATCH 3/4] drm/atomic: Move enable/connector check up in check_modeset()

[Maarten Lankhorst]

Now that handle_conflicting_encoders no longer touches active state,
so there's no need to do the check quite that late any more.

Doing it with all the other checks makes it a lot more clear what the
below block tries to accomplish, and this feels like a better place to
put the check.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d267f45d2c9a..543ac330a1ae 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -494,6 +494,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	int i, ret;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		bool has_connectors =
+			!!new_crtc_state->connector_mask;
+
 		if (!drm_mode_equal(&old_crtc_state->mode, &new_crtc_state->mode)) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode changed\n",
 					 crtc->base.id, crtc->name);
@@ -521,6 +524,13 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 					 crtc->base.id, crtc->name);
 			new_crtc_state->active_changed = true;
 		}
+
+		if (new_crtc_state->enable != has_connectors) {
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
+					 crtc->base.id, crtc->name);
+
+			return -EINVAL;
+		}
 	}
 
 	ret = handle_conflicting_encoders(state, false);
@@ -554,9 +564,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * crtc only changed its mode but has the same set of connectors.
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		bool has_connectors =
-			!!new_crtc_state->connector_mask;
-
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
@@ -572,13 +579,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		ret = drm_atomic_add_affected_planes(state, crtc);
 		if (ret != 0)
 			return ret;
-
-		if (new_crtc_state->enable != has_connectors) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
-					 crtc->base.id, crtc->name);
-
-			return -EINVAL;
-		}
 	}
 
 	return mode_fixup(state);
-- 
2.7.4

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

[PATCH 4/4] drm/atomic: Add connector atomic_check function, v2.

[Maarten Lankhorst]

The atomic_check function is useful for implementing properties, but
it can be used for other connector modeset related checks as well.

Similar to plane check functions, on a modeset atomic_check() is always
called.

Changes since v1:
- Make sure atomic_check() is called on any modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 44 +++++++++++++++++++++++++++++---
 include/drm/drm_modeset_helper_vtables.h | 34 ++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 543ac330a1ae..83567c4091b8 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -459,10 +459,20 @@ mode_fixup(struct drm_atomic_state *state)
  *
  * Check the state object to see if the requested state is physically possible.
  * This does all the crtc and connector related computations for an atomic
- * update and adds any additional connectors needed for full modesets and calls
- * down into &drm_crtc_helper_funcs.mode_fixup and
- * &drm_encoder_helper_funcs.mode_fixup or
- * &drm_encoder_helper_funcs.atomic_check functions of the driver backend.
+ * update and adds any additional connectors needed for full modesets. It calls
+ * the various per-object callbacks in the follow order:
+ *
+ * 1. &drm_connector_helper_funcs.atomic_best_encoder for determining the new encoder.
+ * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state.
+ * 3. If it's determined a modeset is needed then all connectors on the affected crtc
+ *    crtc are added and &drm_connector_helper_funcs.atomic_check is run on them.
+ * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges.
+ * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state.
+ *    This function is only called when the encoder will be part of a configured crtc,
+ *    it must not be used for implementing connector property validation.
+ *    If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called
+ *    instead.
+ * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints.
  *
  * &drm_crtc_state.mode_changed is set when the input mode is changed.
  * &drm_crtc_state.connectors_changed is set when a connector is added or
@@ -492,6 +502,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	struct drm_connector *connector;
 	struct drm_connector_state *old_connector_state, *new_connector_state;
 	int i, ret;
+	unsigned connectors_mask = 0;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		bool has_connectors =
@@ -538,6 +549,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		return ret;
 
 	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+
 		/*
 		 * This only sets crtc->connectors_changed for routing changes,
 		 * drivers must set crtc->connectors_changed themselves when
@@ -555,6 +568,13 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			    new_connector_state->link_status)
 				new_crtc_state->connectors_changed = true;
 		}
+
+		if (funcs->atomic_check)
+			ret = funcs->atomic_check(connector, new_connector_state);
+		if (ret)
+			return ret;
+
+		connectors_mask += BIT(i);
 	}
 
 	/*
@@ -581,6 +601,22 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 			return ret;
 	}
 
+	/*
+	 * Iterate over all connectors again, to make sure atomic_check()
+	 * has been called on them when a modeset is forced.
+	 */
+	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
+		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+
+		if (connectors_mask & BIT(i))
+			continue;
+
+		if (funcs->atomic_check)
+			ret = funcs->atomic_check(connector, new_connector_state);
+		if (ret)
+			return ret;
+	}
+
 	return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 7847babd893c..c01c328f6cc8 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -872,6 +872,40 @@ struct drm_connector_helper_funcs {
 	 */
 	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
 						   struct drm_connector_state *connector_state);
+
+	/**
+	 * @atomic_check:
+	 *
+	 * This hook is used to validate connector state. This function is
+	 * called from &drm_atomic_helper_check_modeset, and is called when
+	 * a connector property is set, or a modeset on the crtc is forced.
+	 *
+	 * Because &drm_atomic_helper_check_modeset may be called multiple times,
+	 * this function should handle being called multiple times as well.
+	 *
+	 * This function is also allowed to inspect any other object's state and
+	 * can add more state objects to the atomic commit if needed. Care must
+	 * be taken though to ensure that state check and compute functions for
+	 * these added states are all called, and derived state in other objects
+	 * all updated. Again the recommendation is to just call check helpers
+	 * until a maximal configuration is reached.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the free-standing
+	 * state objects passed-in or assembled in the overall &drm_atomic_state
+	 * update tracking structure.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success, -EINVAL if the state or the transition can't be
+	 * supported, -ENOMEM on memory allocation failure and -EDEADLK if an
+	 * attempt to obtain another state object ran into a &drm_modeset_lock
+	 * deadlock.
+	 */
+	int (*atomic_check)(struct drm_connector *connector,
+			    struct drm_connector_state *state);
 };
 
 /**
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/atomic: Cleanups for adding connector atomic check function.

[Patchwork]

== Series Details ==

Series: drm/atomic: Cleanups for adding connector atomic check function.
URL   : https://patchwork.freedesktop.org/series/22577/
State : success

== Summary ==

Series 22577v1 drm/atomic: Cleanups for adding connector atomic check function.
https://patchwork.freedesktop.org/api/1.0/series/22577/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 434s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 428s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 579s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 504s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 482s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 412s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 406s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 424s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 490s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 468s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 458s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 569s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 444s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 578s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 461s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 486s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 431s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 530s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 415s
fi-bxt-t5700 failed to collect. IGT log at Patchwork_4419/fi-bxt-t5700/igt.log
fi-byt-j1900 failed to collect. IGT log at Patchwork_4419/fi-byt-j1900/igt.log

6732aaa5e8d9f1bfa454efed21a548715b9b07d4 drm-tip: 2017y-04m-06d-10h-12m-12s UTC integration manifest
32ac58c drm/atomic: Add connector atomic_check function, v2.
a0bca8b drm/atomic: Move enable/connector check up in check_modeset()
0b0bc0b drm/atomic: Set all the changed flags in one place.
864e9ad drm/atomic: Unify conflicting encoder handling.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4419/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] drm/atomic: Cleanups for adding connector atomic check function.

[Sean Paul]

On Thu, Apr 06, 2017 at 01:18:59PM +0200, Maarten Lankhorst wrote:
> Some small cleanups I came across to make drm_atomic_helper_check_modeset
> more readable.
> 
> This makes it a lot more clear what atomic_check does and why it's called in
> the place it is.
> 
> Maarten Lankhorst (4):
>   drm/atomic: Unify conflicting encoder handling.
>   drm/atomic: Set all the changed flags in one place.
>   drm/atomic: Move enable/connector check up in check_modeset()
>   drm/atomic: Add connector atomic_check function, v2.

Hi Maarten,
For the set,

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> 
>  drivers/gpu/drm/drm_atomic_helper.c      | 84 ++++++++++++++++++++++----------
>  include/drm/drm_atomic.h                 |  2 -
>  include/drm/drm_modeset_helper_vtables.h | 34 +++++++++++++
>  3 files changed, 93 insertions(+), 27 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] drm/atomic: Cleanups for adding connector atomic check function.

[Sean Paul]

On Thu, Apr 06, 2017 at 08:48:36AM -0400, Sean Paul wrote:
> On Thu, Apr 06, 2017 at 01:18:59PM +0200, Maarten Lankhorst wrote:
> > Some small cleanups I came across to make drm_atomic_helper_check_modeset
> > more readable.
> > 
> > This makes it a lot more clear what atomic_check does and why it's called in
> > the place it is.
> > 
> > Maarten Lankhorst (4):
> >   drm/atomic: Unify conflicting encoder handling.
> >   drm/atomic: Set all the changed flags in one place.
> >   drm/atomic: Move enable/connector check up in check_modeset()
> >   drm/atomic: Add connector atomic_check function, v2.
> 
> Hi Maarten,
> For the set,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Applied to -misc-next with reviews from v1

Sean

> 
> > 
> >  drivers/gpu/drm/drm_atomic_helper.c      | 84 ++++++++++++++++++++++----------
> >  include/drm/drm_atomic.h                 |  2 -
> >  include/drm/drm_modeset_helper_vtables.h | 34 +++++++++++++
> >  3 files changed, 93 insertions(+), 27 deletions(-)
> > 
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel