[PATCH 1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[PATCH 1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Maarten Lankhorst]

Most code only cares about the current commit or previous commit.
Fortunately we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)

The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		crtc->funcs->atomic_destroy_state(crtc,
 						  state->crtcs[i].state);
 
-		if (state->crtcs[i].commit) {
-			kfree(state->crtcs[i].commit->event);
-			state->crtcs[i].commit->event = NULL;
-			drm_crtc_commit_put(state->crtcs[i].commit);
-		}
-
-		state->crtcs[i].commit = NULL;
 		state->crtcs[i].ptr = NULL;
 		state->crtcs[i].state = NULL;
 	}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..9c2888cb4081 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 					  struct drm_atomic_state *old_state)
 {
-	struct drm_crtc_state *unused;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i;
 
-	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
-		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+		struct drm_crtc_commit *commit = new_crtc_state->commit;
 		int ret;
 
 		if (!commit)
@@ -1388,11 +1388,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_crtc_commit *commit;
 	struct drm_plane *__plane, *plane = NULL;
 	struct drm_plane_state *__plane_state, *plane_state = NULL;
 	const struct drm_plane_helper_funcs *funcs;
-	int i, j, n_planes = 0;
+	int i, n_planes = 0;
 
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		if (drm_atomic_crtc_needs_modeset(crtc_state))
@@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 		return -EINVAL;
 
 	/*
-	 * Don't do an async update if there is an outstanding commit modifying
+	 * XXX: Don't do an async update if there is an outstanding commit modifying
 	 * the plane.  This prevents our async update's changes from getting
 	 * overridden by a previous synchronous update's state.
 	 */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		if (plane->crtc != crtc)
-			continue;
-
-		spin_lock(&crtc->commit_lock);
-		commit = list_first_entry_or_null(&crtc->commit_list,
-						  struct drm_crtc_commit,
-						  commit_entry);
-		if (!commit) {
-			spin_unlock(&crtc->commit_lock);
-			continue;
-		}
-		spin_unlock(&crtc->commit_lock);
-
-		if (!crtc->state->state)
-			continue;
-
-		for_each_plane_in_state(crtc->state->state, __plane,
-					__plane_state, j) {
-			if (__plane == plane)
-				return -EINVAL;
-		}
-	}
 
 	return funcs->atomic_async_check(plane, plane_state);
 }
@@ -1731,7 +1707,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		kref_init(&commit->ref);
 		commit->crtc = crtc;
 
-		state->crtcs[i].commit = commit;
+		new_crtc_state->commit = commit;
 
 		ret = stall_checks(crtc, nonblock);
 		if (ret)
@@ -1769,22 +1745,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
-	struct drm_crtc_commit *commit;
-	int i = 0;
-
-	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
-		/* skip the first entry, that's the current commit */
-		if (i == 1)
-			return commit;
-		i++;
-	}
-
-	return NULL;
-}
-
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
  * @old_state: atomic state object with old state structures
@@ -1800,17 +1760,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
 
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		spin_lock(&crtc->commit_lock);
-		commit = preceeding_commit(crtc);
-		if (commit)
-			drm_crtc_commit_get(commit);
-		spin_unlock(&crtc->commit_lock);
+	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		commit = old_crtc_state->commit;
 
 		if (!commit)
 			continue;
@@ -1828,8 +1784,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 		if (ret == 0)
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
-
-		drm_crtc_commit_put(commit);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1857,7 +1811,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 	int i;
 
 	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+		commit = new_crtc_state->commit;
 		if (!commit)
 			continue;
 
@@ -1888,7 +1842,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 	long ret;
 
 	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+		commit = new_crtc_state->commit;
 		if (WARN_ON(!commit))
 			continue;
 
@@ -2294,20 +2248,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_private_state *old_obj_state, *new_obj_state;
 
 	if (stall) {
-		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-			spin_lock(&crtc->commit_lock);
-			commit = list_first_entry_or_null(&crtc->commit_list,
-					struct drm_crtc_commit, commit_entry);
-			if (commit)
-				drm_crtc_commit_get(commit);
-			spin_unlock(&crtc->commit_lock);
+		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+			commit = old_crtc_state->commit;
 
 			if (!commit)
 				continue;
 
 			ret = wait_for_completion_interruptible(&commit->hw_done);
-			drm_crtc_commit_put(commit);
-
 			if (ret)
 				return ret;
 		}
@@ -2332,13 +2279,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->crtcs[i].state = old_crtc_state;
 		crtc->state = new_crtc_state;
 
-		if (state->crtcs[i].commit) {
+		if (new_crtc_state->commit) {
 			spin_lock(&crtc->commit_lock);
-			list_add(&state->crtcs[i].commit->commit_entry,
+			list_add(&new_crtc_state->commit->commit_entry,
 				 &crtc->commit_list);
 			spin_unlock(&crtc->commit_lock);
 
-			state->crtcs[i].commit->event = NULL;
+			new_crtc_state->commit->event = NULL;
 		}
 	}
 
@@ -3186,6 +3133,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->connectors_changed = false;
 	state->color_mgmt_changed = false;
 	state->zpos_changed = false;
+	state->commit = NULL;
 	state->event = NULL;
 	state->pageflip_flags = 0;
 }
@@ -3224,6 +3172,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
  */
 void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 {
+	if (state->commit) {
+		kfree(state->commit->event);
+		state->commit->event = NULL;
+		drm_crtc_commit_put(state->commit);
+	}
+
 	drm_property_blob_put(state->mode_blob);
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 8a5808eb5628..e76d9f95c191 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
 struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state, *old_state, *new_state;
-	struct drm_crtc_commit *commit;
 	s32 __user *out_fence_ptr;
 	unsigned last_vblank_count;
 };
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..4cc7c9cdb6ca 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
 	 */
 	struct drm_pending_vblank_event *event;
 
+	/**
+	 * @commit:
+	 *
+	 * This tracks how the commit for this update proceeds through the
+	 * various phases. This is never cleared, except when we destroy the
+	 * state, so that subsequent commits can synchronize with previous ones.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_atomic_state *state;
 };
 
-- 
2.11.0

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

[PATCH 2/2] drm/atomic: Fix freeing connector/plane state too early by tracking commits

[Maarten Lankhorst]

Currently we neatly track the crtc state, but forget to look at
plane/connector state.

When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.

This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
crtc commit for the connector that gets signaled on hw_done.

We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 171 ++++++++++++++++++++++++++++++++++--
 include/drm/drm_connector.h         |   7 ++
 include/drm/drm_plane.h             |   7 ++
 3 files changed, 179 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9c2888cb4081..a4fd500d6200 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1644,6 +1644,39 @@ static void release_crtc_commit(struct completion *completion)
 	drm_crtc_commit_put(commit);
 }
 
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+	init_completion(&commit->flip_done);
+	init_completion(&commit->hw_done);
+	init_completion(&commit->cleanup_done);
+	INIT_LIST_HEAD(&commit->commit_entry);
+	kref_init(&commit->ref);
+	commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+init_or_ref_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+	struct drm_crtc_commit *commit;
+
+	if (crtc) {
+		struct drm_crtc_state *new_crtc_state;
+
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+		commit = new_crtc_state->commit;
+		drm_crtc_commit_get(commit);
+	} else {
+		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
+		if (!commit)
+			return NULL;
+
+		init_commit(commit, NULL);
+	}
+
+	return commit;
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1692,6 +1725,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
 	int i, ret;
 
@@ -1700,12 +1737,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		if (!commit)
 			return -ENOMEM;
 
-		init_completion(&commit->flip_done);
-		init_completion(&commit->hw_done);
-		init_completion(&commit->cleanup_done);
-		INIT_LIST_HEAD(&commit->commit_entry);
-		kref_init(&commit->ref);
-		commit->crtc = crtc;
+		init_commit(commit, crtc);
 
 		new_crtc_state->commit = commit;
 
@@ -1741,6 +1773,36 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		drm_crtc_commit_get(commit);
 	}
 
+	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
+		if (new_conn_state->crtc)
+			continue;
+
+		if (nonblock && old_conn_state->commit &&
+		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
+			return -EBUSY;
+
+		commit = init_or_ref_crtc_commit(state, old_conn_state->crtc);
+		if (!commit)
+			return -ENOMEM;
+
+		new_conn_state->commit = commit;
+	}
+
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		if (new_plane_state->crtc)
+			continue;
+
+		if (nonblock && old_plane_state->commit &&
+		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
+			return -EBUSY;
+
+		commit = init_or_ref_crtc_commit(state, old_plane_state->crtc);
+		if (!commit)
+			return -ENOMEM;
+
+		new_plane_state->commit = commit;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
@@ -1761,6 +1823,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *old_conn_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
@@ -1785,6 +1851,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
 	}
+
+	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+		commit = old_conn_state->commit;
+
+		if (!commit)
+			continue;
+
+		ret = wait_for_completion_timeout(&commit->hw_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
+				  conn->base.id, conn->name);
+
+		/* Currently no support for overwriting flips, hence
+		 * stall for previous one to execute completely. */
+		ret = wait_for_completion_timeout(&commit->flip_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
+				  conn->base.id, conn->name);
+	}
+
+	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+		commit = old_plane_state->commit;
+
+		if (!commit)
+			continue;
+
+		ret = wait_for_completion_timeout(&commit->hw_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
+				  plane->base.id, plane->name);
+
+		/* Currently no support for overwriting flips, hence
+		 * stall for previous one to execute completely. */
+		ret = wait_for_completion_timeout(&commit->flip_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
+				  plane->base.id, plane->name);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
@@ -1807,6 +1915,10 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *new_conn_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *new_plane_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
@@ -1819,6 +1931,23 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 		WARN_ON(new_crtc_state->event);
 		complete_all(&commit->hw_done);
 	}
+
+	for_each_new_connector_in_state(old_state, conn, new_conn_state, i) {
+		commit = new_conn_state->commit;
+		if (commit && !commit->crtc) {
+			complete_all(&commit->hw_done);
+			complete_all(&commit->flip_done);
+		}
+	}
+
+	for_each_new_plane_in_state(old_state, plane, new_plane_state, i) {
+		commit = new_plane_state->commit;
+		if (commit && !commit->crtc) {
+			complete_all(&commit->hw_done);
+			complete_all(&commit->flip_done);
+		}
+	}
+
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
@@ -2258,6 +2387,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 			if (ret)
 				return ret;
 		}
+
+		for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+			commit = old_conn_state->commit;
+
+			if (!commit)
+				continue;
+
+			ret = wait_for_completion_interruptible(&commit->hw_done);
+			if (ret)
+				return ret;
+		}
+
+		for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+			commit = old_plane_state->commit;
+
+			if (!commit)
+				continue;
+
+			ret = wait_for_completion_interruptible(&commit->hw_done);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
@@ -3240,6 +3391,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 		drm_framebuffer_get(state->fb);
 
 	state->fence = NULL;
+	state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
@@ -3281,6 +3433,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 
 	if (state->fence)
 		dma_fence_put(state->fence);
+
+	if (state->commit)
+		drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
@@ -3359,6 +3514,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 	memcpy(state, connector->state, sizeof(*state));
 	if (state->crtc)
 		drm_connector_get(connector);
+	state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3485,6 +3641,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 {
 	if (state->crtc)
 		drm_connector_put(state->connector);
+
+	if (state->commit)
+		drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ea8da401c93c..8837649d16e8 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -347,6 +347,13 @@ struct drm_connector_state {
 
 	struct drm_atomic_state *state;
 
+	/**
+	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
+	 *
+	 * Is only set when @crtc is NULL.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_tv_connector_state tv;
 
 	/**
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73f90f9d057f..7d96116fd4c4 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -123,6 +123,13 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+	/**
+	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
+	 *
+	 * Is only set when @crtc is NULL.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_atomic_state *state;
 };
 
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.
URL   : https://patchwork.freedesktop.org/series/29476/
State : success

== Summary ==

Series 29476v1 series starting with [1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.
https://patchwork.freedesktop.org/api/1.0/series/29476/revisions/1/mbox/

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:455s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:444s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:365s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:573s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:253s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:527s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:529s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:526s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:436s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:618s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:448s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:424s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:426s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:516s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:601s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:473s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:482s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:493s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:440s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:484s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:548s
fi-snb-2600      total:279  pass:249  dwarn:0   dfail:0   fail:1   skip:29  time:410s
fi-pnv-d510 failed to connect after reboot

627598734e5ed449b1173ff8158126c57c361a40 drm-tip: 2017y-08m-29d-09h-52m-12s UTC integration manifest
73a398a3ba87 drm/atomic: Fix freeing connector/plane state too early by tracking commits
23c240dcae13 drm/atomic: Move drm_crtc_commit to drm_crtc_state.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5519/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.
URL   : https://patchwork.freedesktop.org/series/29476/
State : failure

== Summary ==

Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_cursor_legacy:
        Subgroup nonblocking-modeset-vs-cursor-atomic:
                pass       -> INCOMPLETE (shard-hsw)
        Subgroup long-nonblocking-modeset-vs-cursor-atomic:
                pass       -> INCOMPLETE (shard-hsw)
Test kms_atomic_transition:
        Subgroup plane-use-after-nonblocking-unbind:
                fail       -> PASS       (shard-hsw) fdo#101847 +1
        Subgroup plane-all-transition-fencing:
                skip       -> PASS       (shard-hsw)
Test kms_flip:
        Subgroup dpms-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw)
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-B:
                pass       -> INCOMPLETE (shard-hsw)
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
        Subgroup plane-position-hole-dpms-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912
Test kms_properties:
        Subgroup plane-properties-legacy:
                skip       -> PASS       (shard-hsw)

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2105 pass:1158 dwarn:0   dfail:0   fail:16  skip:928 time:8690s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5519/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Daniel Vetter]

On Tue, Aug 29, 2017 at 04:02:02PM +0200, Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortunately we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
> 
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  7 ---
>  drivers/gpu/drm/drm_atomic_helper.c | 92 ++++++++++---------------------------
>  include/drm/drm_atomic.h            |  1 -
>  include/drm/drm_crtc.h              |  9 ++++
>  4 files changed, 32 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		crtc->funcs->atomic_destroy_state(crtc,
>  						  state->crtcs[i].state);
>  
> -		if (state->crtcs[i].commit) {
> -			kfree(state->crtcs[i].commit->event);
> -			state->crtcs[i].commit->event = NULL;
> -			drm_crtc_commit_put(state->crtcs[i].commit);
> -		}
> -
> -		state->crtcs[i].commit = NULL;
>  		state->crtcs[i].ptr = NULL;
>  		state->crtcs[i].state = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e53aae9a1fb..9c2888cb4081 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  					  struct drm_atomic_state *old_state)
>  {
> -	struct drm_crtc_state *unused;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	int i;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> -		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> +	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		struct drm_crtc_commit *commit = new_crtc_state->commit;
>  		int ret;
>  
>  		if (!commit)
> @@ -1388,11 +1388,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
>  	struct drm_plane *__plane, *plane = NULL;
>  	struct drm_plane_state *__plane_state, *plane_state = NULL;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
> @@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	/*
> -	 * Don't do an async update if there is an outstanding commit modifying
> +	 * XXX: Don't do an async update if there is an outstanding commit modifying
>  	 * the plane.  This prevents our async update's changes from getting
>  	 * overridden by a previous synchronous update's state.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> -
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {
> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
>  
>  	return funcs->atomic_async_check(plane, plane_state);
>  }
> @@ -1731,7 +1707,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		kref_init(&commit->ref);
>  		commit->crtc = crtc;
>  
> -		state->crtcs[i].commit = commit;
> +		new_crtc_state->commit = commit;
>  
>  		ret = stall_checks(crtc, nonblock);
>  		if (ret)
> @@ -1769,22 +1745,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>  
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> -	struct drm_crtc_commit *commit;
> -	int i = 0;
> -
> -	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> -		/* skip the first entry, that's the current commit */
> -		if (i == 1)
> -			return commit;
> -		i++;
> -	}
> -
> -	return NULL;
> -}
> -
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
>   * @old_state: atomic state object with old state structures
> @@ -1800,17 +1760,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		spin_lock(&crtc->commit_lock);
> -		commit = preceeding_commit(crtc);
> -		if (commit)
> -			drm_crtc_commit_get(commit);
> -		spin_unlock(&crtc->commit_lock);
> +	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		commit = old_crtc_state->commit;
>  
>  		if (!commit)
>  			continue;
> @@ -1828,8 +1784,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  		if (ret == 0)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
> -
> -		drm_crtc_commit_put(commit);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> @@ -1857,7 +1811,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  	int i;
>  
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (!commit)
>  			continue;
>  
> @@ -1888,7 +1842,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  	long ret;
>  
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (WARN_ON(!commit))
>  			continue;
>  
> @@ -2294,20 +2248,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_private_state *old_obj_state, *new_obj_state;
>  
>  	if (stall) {
> -		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -			spin_lock(&crtc->commit_lock);
> -			commit = list_first_entry_or_null(&crtc->commit_list,
> -					struct drm_crtc_commit, commit_entry);
> -			if (commit)
> -				drm_crtc_commit_get(commit);
> -			spin_unlock(&crtc->commit_lock);
> +		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			commit = old_crtc_state->commit;
>  
>  			if (!commit)
>  				continue;
>  
>  			ret = wait_for_completion_interruptible(&commit->hw_done);
> -			drm_crtc_commit_put(commit);
> -
>  			if (ret)
>  				return ret;
>  		}
> @@ -2332,13 +2279,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		state->crtcs[i].state = old_crtc_state;
>  		crtc->state = new_crtc_state;
>  
> -		if (state->crtcs[i].commit) {
> +		if (new_crtc_state->commit) {
>  			spin_lock(&crtc->commit_lock);
> -			list_add(&state->crtcs[i].commit->commit_entry,
> +			list_add(&new_crtc_state->commit->commit_entry,
>  				 &crtc->commit_list);
>  			spin_unlock(&crtc->commit_lock);
>  
> -			state->crtcs[i].commit->event = NULL;
> +			new_crtc_state->commit->event = NULL;
>  		}
>  	}
>  
> @@ -3186,6 +3133,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
>  	state->zpos_changed = false;
> +	state->commit = NULL;
>  	state->event = NULL;
>  	state->pageflip_flags = 0;
>  }
> @@ -3224,6 +3172,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>   */
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>  {
> +	if (state->commit) {
> +		kfree(state->commit->event);
> +		state->commit->event = NULL;
> +		drm_crtc_commit_put(state->commit);
> +	}
> +
>  	drm_property_blob_put(state->mode_blob);
>  	drm_property_blob_put(state->degamma_lut);
>  	drm_property_blob_put(state->ctm);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8a5808eb5628..e76d9f95c191 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,7 +144,6 @@ struct __drm_planes_state {
>  struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state, *old_state, *new_state;
> -	struct drm_crtc_commit *commit;
>  	s32 __user *out_fence_ptr;
>  	unsigned last_vblank_count;
>  };
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a642020e306..4cc7c9cdb6ca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -253,6 +253,15 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_pending_vblank_event *event;
>  
> +	/**
> +	 * @commit:
> +	 *
> +	 * This tracks how the commit for this update proceeds through the
> +	 * various phases. This is never cleared, except when we destroy the
> +	 * state, so that subsequent commits can synchronize with previous ones.
> +	 */
> +	struct drm_crtc_commit *commit;

Please update the kerneldoc for @commit_list a new paragraph at the end:

"Note that the commit for a state change is also tracked in
&drm_crtc_state.commit. For accessing the immediately preceeding commit in
an atomic update it is recommended to just use that pointer in the old
CRTC state, since accessing that doesn't need any locking or
list-walking. @commit_list should only be used to stall for framebuffer
cleanup that's signalled through &drm_crtc_commit.cleanup_done."

With that, and assuming it passes full igt in CI:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I think a good follow-up patch would be to remove the waits and comments
in drm_atomic_helper_commit_cleanup_done, since that's no no longer
needed. Removing that definitely requires an updated kernel-doc for
@commit_list though.
-Daniel

> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/atomic: Fix freeing connector/plane state too early by tracking commits

[Daniel Vetter]

Onnkhorst> that would be the right thing to do
<danvet> chickens didn't want to audit 20+ drivers :-)
<mlankhorst> neither, could we just break them? :pTue, Aug 29, 2017 at 04:02:03PM +0200, Maarten Lankhorst wrote:
> Currently we neatly track the crtc state, but forget to look at
> plane/connector state.
> 
> When doing a nonblocking modeset, immediately followed by a setprop
> before the modeset completes, the setprop will see the modesets new
> state as the old state and free it.
> 
> This has to be solved by waiting for hw_done on the connector, even
> if it's not assigned to a crtc. When a connector is unbound we take
> the last crtc commit, and when it stays unbound we create a new
> crtc commit for the connector that gets signaled on hw_done.
> 
> We wait for it the same way as we do for crtc's, which will make
> sure we never run into a use-after-free situation.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 171 ++++++++++++++++++++++++++++++++++--
>  include/drm/drm_connector.h         |   7 ++
>  include/drm/drm_plane.h             |   7 ++
>  3 files changed, 179 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9c2888cb4081..a4fd500d6200 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1644,6 +1644,39 @@ static void release_crtc_commit(struct completion *completion)
>  	drm_crtc_commit_put(commit);
>  }
>  
> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
> +{
> +	init_completion(&commit->flip_done);
> +	init_completion(&commit->hw_done);
> +	init_completion(&commit->cleanup_done);
> +	INIT_LIST_HEAD(&commit->commit_entry);
> +	kref_init(&commit->ref);
> +	commit->crtc = crtc;
> +}
> +
> +static struct drm_crtc_commit *
> +init_or_ref_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> +{
> +	struct drm_crtc_commit *commit;
> +
> +	if (crtc) {
> +		struct drm_crtc_state *new_crtc_state;
> +
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +		commit = new_crtc_state->commit;
> +		drm_crtc_commit_get(commit);
> +	} else {
> +		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> +		if (!commit)
> +			return NULL;
> +
> +		init_commit(commit, NULL);
> +	}
> +
> +	return commit;
> +}
> +
>  /**
>   * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>   * @state: new modeset state to be committed
> @@ -1692,6 +1725,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_crtc_commit *commit;
>  	int i, ret;
>  
> @@ -1700,12 +1737,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		if (!commit)
>  			return -ENOMEM;
>  
> -		init_completion(&commit->flip_done);
> -		init_completion(&commit->hw_done);
> -		init_completion(&commit->cleanup_done);
> -		INIT_LIST_HEAD(&commit->commit_entry);
> -		kref_init(&commit->ref);
> -		commit->crtc = crtc;
> +		init_commit(commit, crtc);
>  
>  		new_crtc_state->commit = commit;
>  
> @@ -1741,6 +1773,36 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		drm_crtc_commit_get(commit);
>  	}
>  
> +	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
> +		if (new_conn_state->crtc)
> +			continue;
> +
> +		if (nonblock && old_conn_state->commit &&
> +		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
> +			return -EBUSY;
> +
> +		commit = init_or_ref_crtc_commit(state, old_conn_state->crtc);
> +		if (!commit)
> +			return -ENOMEM;
> +
> +		new_conn_state->commit = commit;
> +	}
> +
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> +		if (new_plane_state->crtc)
> +			continue;
> +
> +		if (nonblock && old_plane_state->commit &&
> +		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
> +			return -EBUSY;
> +
> +		commit = init_or_ref_crtc_commit(state, old_plane_state->crtc);
> +		if (!commit)
> +			return -ENOMEM;
> +
> +		new_plane_state->commit = commit;
> +	}
> +
>  	return 0;
>  }

Ok, I think this works, but it's a bit confusing to have a chain of
drm_crtc_commits only for when the plane/connector isn't connected to
anything. I agree that with your design something else is clearer.

I had something slightly different in mind, with slightly different
semantics:

- we add a ->disabling_commit to the crtc state

- in setup_commit we do the following
  - if state->crtc != NULL, we do nothing, since the commit is tracked
    through the crtc
  - if old_state->crtc && !new_state->crtc, then we are disabling the
    connector/plane in this commit. In that case we'll reuse the same
    drm_crtc_commit for the current crtc commmit, i.e.

	new_conn_state->disabling_commit =
		new_crtc_state(old_conn_state->crtc)->commit;
	drm_crtc_commit_get(new_conn_state->disabling_commit);

  - if the connector is disabled and stays disabled, we check whether the
    commit has completed already. If not, we copy it forward:

	new_conn_state->disabling_commit = old_conn_state->disabling_commit;
	drm_crtc_commit_get(new_conn_state->disabling_commit);

The last point is where your and my idea differ, by simply copying the
same commit forward we don't end up with a chain of somewhat confusing
fake commit objects. We also don't have to call complete_all on any
connector/plane commits (since they're always real drm_crtc_commit
trackers for a real crtc).

I think calling it ->disabling_commit also makes it more clear what
exactly is the special case and when we need it. Maybe even call it
last_disabling_commit for more clarity.

>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> @@ -1761,6 +1823,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *old_conn_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
> @@ -1785,6 +1851,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
>  	}
> +
> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
> +		commit = old_conn_state->commit;
> +
> +		if (!commit)
> +			continue;
> +
> +		ret = wait_for_completion_timeout(&commit->hw_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
> +				  conn->base.id, conn->name);
> +
> +		/* Currently no support for overwriting flips, hence
> +		 * stall for previous one to execute completely. */
> +		ret = wait_for_completion_timeout(&commit->flip_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
> +				  conn->base.id, conn->name);
> +	}
> +
> +	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
> +		commit = old_plane_state->commit;
> +
> +		if (!commit)
> +			continue;
> +
> +		ret = wait_for_completion_timeout(&commit->hw_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
> +				  plane->base.id, plane->name);
> +
> +		/* Currently no support for overwriting flips, hence
> +		 * stall for previous one to execute completely. */
> +		ret = wait_for_completion_timeout(&commit->flip_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
> +				  plane->base.id, plane->name);
> +	}

Why do you wait for flip_done and not hw_done? The general rule is that
you can signal flip_done when the hw stops scanning out stuff, which when
disabling, can be way before we've shut down the hw properly.

For CRTC we wait for both, I think the same must be done for
planes/connectors (yes more code).

Also, you're missing the corresponding code in swap_state. i915 might be
able to get away without that, but these helpers here aren't just for
i915.

>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
> @@ -1807,6 +1915,10 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *new_crtc_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *new_plane_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  
> @@ -1819,6 +1931,23 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  		WARN_ON(new_crtc_state->event);
>  		complete_all(&commit->hw_done);
>  	}
> +
> +	for_each_new_connector_in_state(old_state, conn, new_conn_state, i) {
> +		commit = new_conn_state->commit;
> +		if (commit && !commit->crtc) {
> +			complete_all(&commit->hw_done);
> +			complete_all(&commit->flip_done);
> +		}
> +	}
> +
> +	for_each_new_plane_in_state(old_state, plane, new_plane_state, i) {
> +		commit = new_plane_state->commit;
> +		if (commit && !commit->crtc) {
> +			complete_all(&commit->hw_done);
> +			complete_all(&commit->flip_done);
> +		}
> +	}
> +
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
> @@ -2258,6 +2387,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (ret)
>  				return ret;
>  		}
> +
> +		for_each_old_connector_in_state(state, connector, old_conn_state, i) {
> +			commit = old_conn_state->commit;
> +
> +			if (!commit)
> +				continue;
> +
> +			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> +			commit = old_plane_state->commit;
> +
> +			if (!commit)
> +				continue;
> +
> +			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
> @@ -3240,6 +3391,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  		drm_framebuffer_get(state->fb);
>  
>  	state->fence = NULL;
> +	state->commit = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>  
> @@ -3281,6 +3433,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  
>  	if (state->fence)
>  		dma_fence_put(state->fence);
> +
> +	if (state->commit)
> +		drm_crtc_commit_put(state->commit);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> @@ -3359,6 +3514,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  	memcpy(state, connector->state, sizeof(*state));
>  	if (state->crtc)
>  		drm_connector_get(connector);
> +	state->commit = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>  
> @@ -3485,6 +3641,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>  {
>  	if (state->crtc)
>  		drm_connector_put(state->connector);
> +
> +	if (state->commit)
> +		drm_crtc_commit_put(state->commit);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ea8da401c93c..8837649d16e8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -347,6 +347,13 @@ struct drm_connector_state {
>  
>  	struct drm_atomic_state *state;
>  
> +	/**
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.

For multiline member comments please do an empty line between the heading
and the real text. It's definitely the style guide, but it also might
confuse kerneldoc.

Cheers, Daniel

> +	 *
> +	 * Is only set when @crtc is NULL.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_tv_connector_state tv;
>  
>  	/**
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 73f90f9d057f..7d96116fd4c4 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -123,6 +123,13 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +	/**
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 *
> +	 * Is only set when @crtc is NULL.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/atomic: Move drm_crtc_commit to drm_crtc_state.

[Laurent Pinchart]

On Tuesday, 29 August 2017 17:02:02 EEST Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortunately we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
> 
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |  7 ---
>  drivers/gpu/drm/drm_atomic_helper.c | 92 ++++++++--------------------------
>  include/drm/drm_atomic.h            |  1 -
>  include/drm/drm_crtc.h              |  9 ++++
>  4 files changed, 32 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct
> drm_atomic_state *state) crtc->funcs->atomic_destroy_state(crtc,
>  						  state->crtcs[i].state);
> 
> -		if (state->crtcs[i].commit) {
> -			kfree(state->crtcs[i].commit->event);
> -			state->crtcs[i].commit->event = NULL;
> -			drm_crtc_commit_put(state->crtcs[i].commit);
> -		}
> -
> -		state->crtcs[i].commit = NULL;
>  		state->crtcs[i].ptr = NULL;
>  		state->crtcs[i].state = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 4e53aae9a1fb..9c2888cb4081
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  					  struct drm_atomic_state *old_state)
>  {
> -	struct drm_crtc_state *unused;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	int i;
> 
> -	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> -		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> +	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		struct drm_crtc_commit *commit = new_crtc_state->commit;
>  		int ret;
> 
>  		if (!commit)
> @@ -1388,11 +1388,10 @@ int drm_atomic_helper_async_check(struct drm_device
> *dev, {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
>  	struct drm_plane *__plane, *plane = NULL;
>  	struct drm_plane_state *__plane_state, *plane_state = NULL;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
> 
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
> @@ -1420,33 +1419,10 @@ int drm_atomic_helper_async_check(struct drm_device
> *dev, return -EINVAL;
> 
>  	/*
> -	 * Don't do an async update if there is an outstanding commit modifying
> +	 * XXX: Don't do an async update if there is an outstanding commit
> modifying
> 	 * the plane.  This prevents our async update's changes from getting
> 	 * overridden by a previous synchronous update's state.
>  	 */

This looks like work in progress. The code is gone and the comment left. I'm 
not sure what 'XXX' means. If it's a TODO, patch 2/2 doesn't address it. And 
if you really meant to drop this check permanently, the reason should be 
explained in the commit message.

> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> -
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {
> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
> 
>  	return funcs->atomic_async_check(plane, plane_state);
>  }
> @@ -1731,7 +1707,7 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state, kref_init(&commit->ref);
>  		commit->crtc = crtc;
> 
> -		state->crtcs[i].commit = commit;
> +		new_crtc_state->commit = commit;
> 
>  		ret = stall_checks(crtc, nonblock);
>  		if (ret)
> @@ -1769,22 +1745,6 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state, }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> 
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> -	struct drm_crtc_commit *commit;
> -	int i = 0;
> -
> -	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> -		/* skip the first entry, that's the current commit */
> -		if (i == 1)
> -			return commit;
> -		i++;
> -	}
> -
> -	return NULL;
> -}
> -
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding
> commits * @old_state: atomic state object with old state structures
> @@ -1800,17 +1760,13 @@ static struct drm_crtc_commit
> *preceeding_commit(struct drm_crtc *crtc) void
> drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
> 
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		spin_lock(&crtc->commit_lock);
> -		commit = preceeding_commit(crtc);
> -		if (commit)
> -			drm_crtc_commit_get(commit);
> -		spin_unlock(&crtc->commit_lock);
> +	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		commit = old_crtc_state->commit;
> 
>  		if (!commit)
>  			continue;
> @@ -1828,8 +1784,6 @@ void drm_atomic_helper_wait_for_dependencies(struct
> drm_atomic_state *old_state) if (ret == 0)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
> -
> -		drm_crtc_commit_put(commit);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> @@ -1857,7 +1811,7 @@ void drm_atomic_helper_commit_hw_done(struct
> drm_atomic_state *old_state) int i;
> 
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (!commit)
>  			continue;
> 
> @@ -1888,7 +1842,7 @@ void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *old_state) long ret;
> 
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +		commit = new_crtc_state->commit;
>  		if (WARN_ON(!commit))
>  			continue;
> 
> @@ -2294,20 +2248,13 @@ int drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, struct drm_private_state *old_obj_state,
> *new_obj_state;
> 
>  	if (stall) {
> -		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -			spin_lock(&crtc->commit_lock);
> -			commit = list_first_entry_or_null(&crtc->commit_list,
> -					struct drm_crtc_commit, commit_entry);
> -			if (commit)
> -				drm_crtc_commit_get(commit);
> -			spin_unlock(&crtc->commit_lock);
> +		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			commit = old_crtc_state->commit;
> 
>  			if (!commit)
>  				continue;
> 
>  			ret = wait_for_completion_interruptible(&commit->hw_done);
> -			drm_crtc_commit_put(commit);
> -
>  			if (ret)
>  				return ret;
>  		}
> @@ -2332,13 +2279,13 @@ int drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, state->crtcs[i].state = old_crtc_state;
>  		crtc->state = new_crtc_state;
> 
> -		if (state->crtcs[i].commit) {
> +		if (new_crtc_state->commit) {
>  			spin_lock(&crtc->commit_lock);
> -			list_add(&state->crtcs[i].commit->commit_entry,
> +			list_add(&new_crtc_state->commit->commit_entry,
>  				 &crtc->commit_list);
>  			spin_unlock(&crtc->commit_lock);
> 
> -			state->crtcs[i].commit->event = NULL;
> +			new_crtc_state->commit->event = NULL;
>  		}
>  	}
> 
> @@ -3186,6 +3133,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct
> drm_crtc *crtc, state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
>  	state->zpos_changed = false;
> +	state->commit = NULL;
>  	state->event = NULL;
>  	state->pageflip_flags = 0;
>  }
> @@ -3224,6 +3172,12 @@
> EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); */
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>  {
> +	if (state->commit) {
> +		kfree(state->commit->event);
> +		state->commit->event = NULL;
> +		drm_crtc_commit_put(state->commit);
> +	}
> +
>  	drm_property_blob_put(state->mode_blob);
>  	drm_property_blob_put(state->degamma_lut);
>  	drm_property_blob_put(state->ctm);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8a5808eb5628..e76d9f95c191 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,7 +144,6 @@ struct __drm_planes_state {
>  struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state, *old_state, *new_state;
> -	struct drm_crtc_commit *commit;
>  	s32 __user *out_fence_ptr;
>  	unsigned last_vblank_count;
>  };
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a642020e306..4cc7c9cdb6ca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -253,6 +253,15 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_pending_vblank_event *event;
> 
> +	/**
> +	 * @commit:
> +	 *
> +	 * This tracks how the commit for this update proceeds through the
> +	 * various phases. This is never cleared, except when we destroy the
> +	 * state, so that subsequent commits can synchronize with previous ones.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_atomic_state *state;
>  };


-- 
Regards,

Laurent Pinchart

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