[PATCH 0/4] drm/atomic: Lockless blocking commits

[PATCH 0/4] drm/atomic: Lockless blocking commits

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've talked about making blocking commits lockless a few
times in the past, so here's finally an attempt at it.
The main benefit I see from this is that TEST_ONLY commits
no longer getting blocked on the mutexes by parallel blocking
commits.

I have a small test here that spools up two threads,
one does just TEST_ONLY commits in a loop, the other
does either blocking or non-blocking page flips. Results
came out as follows on a snb machine here:

test-only-vs-non-blocking:
-85319 TEST_ONLY commits in 2000000 usecs, 23 usecs / commit
+87144 TEST_ONLY commits in 2000006 usecs, 22 usecs / commit

test-only-vs-blocking:
-219 TEST_ONLY commits in 2001768 usecs, 9140 usecs / commit
+82442 TEST_ONLY commits in 2000011 usecs, 24 usecs / commit

Now, I have no idea if anyone actually cares about lack
of parallelism due to locked blocking commits or not. Hence
Cc'd some compositor folks as well. I guess this is more of
an RFC at this point.

Also curious to see if CI goes up in smoke or not...

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Jonas Ådahl <jadahl@gmail.com>

Ville Syrjälä (4):
  drm/atomic: Treat a nonblocking commit following a blocking commit as
    blocking commit
  drm/i915: Don't reuse commit_work for the cleanup
  drm/atomic: Allow lockless blocking commits
  drm/i915: Make blocking commits lockless

 drivers/gpu/drm/drm_atomic.c                  | 32 +++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c           | 19 +++++++----
 drivers/gpu/drm/drm_atomic_uapi.c             | 11 +++++--
 drivers/gpu/drm/i915/display/intel_display.c  | 15 +++------
 .../drm/i915/display/intel_display_types.h    |  1 +
 include/drm/drm_atomic.h                      |  8 +++++
 6 files changed, 64 insertions(+), 22 deletions(-)

-- 
2.35.1

[PATCH 1/4] drm/atomic: Treat a nonblocking commit following a blocking commit as blocking commit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently a nonblocking commit will actually block if it is
preceded by a blocking commit. It just happens block on the
mutex rather than on the completion. I shall call these as
not-actually-nonblocking commits.

I would like to make blocking commits execute locklessly,
just as nonblocking commits already do. The main benefit
would that parallel TEST_ONLY commits would not get blocked
on the mutexes until the parallel blocking commit is done.
To achieve that without a significant change in behaviour
for the not-actually-nonblocking commits let's treat them
exactly the same as blocking commit, ie. instead of
returning -EBUSY they will just block.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 19 ++++++++++++-------
 include/drm/drm_atomic.h            |  7 +++++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ee5fea48b5cb..bff087674cb5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2109,7 +2109,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 			 * Userspace is not allowed to get ahead of the previous
 			 * commit with nonblocking ones.
 			 */
-			if (!completed && nonblock) {
+			if (!completed && nonblock && commit->nonblock) {
 				spin_unlock(&crtc->commit_lock);
 				drm_dbg_atomic(crtc->dev,
 					       "[CRTC:%d:%s] busy with a previous commit\n",
@@ -2152,7 +2152,7 @@ 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)
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc, bool nonblock)
 {
 	init_completion(&commit->flip_done);
 	init_completion(&commit->hw_done);
@@ -2160,10 +2160,11 @@ static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
 	INIT_LIST_HEAD(&commit->commit_entry);
 	kref_init(&commit->ref);
 	commit->crtc = crtc;
+	commit->nonblock = nonblock;
 }
 
 static struct drm_crtc_commit *
-crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc, bool nonblock)
 {
 	if (crtc) {
 		struct drm_crtc_state *new_crtc_state;
@@ -2178,7 +2179,7 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
 		if (!state->fake_commit)
 			return NULL;
 
-		init_commit(state->fake_commit, NULL);
+		init_commit(state->fake_commit, NULL, nonblock);
 	}
 
 	return state->fake_commit;
@@ -2250,7 +2251,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		if (!commit)
 			return -ENOMEM;
 
-		init_commit(commit, crtc);
+		init_commit(commit, crtc, nonblock);
 
 		new_crtc_state->commit = commit;
 
@@ -2299,6 +2300,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		 * commit with nonblocking ones.
 		 */
 		if (nonblock && old_conn_state->commit &&
+		    old_conn_state->commit->nonblock &&
 		    !try_wait_for_completion(&old_conn_state->commit->flip_done)) {
 			drm_dbg_atomic(conn->dev,
 				       "[CONNECTOR:%d:%s] busy with a previous commit\n",
@@ -2308,7 +2310,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		}
 
 		/* Always track connectors explicitly for e.g. link retraining. */
-		commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);
+		commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc,
+					     nonblock);
 		if (!commit)
 			return -ENOMEM;
 
@@ -2321,6 +2324,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		 * commit with nonblocking ones.
 		 */
 		if (nonblock && old_plane_state->commit &&
+		    old_plane_state->commit->nonblock &&
 		    !try_wait_for_completion(&old_plane_state->commit->flip_done)) {
 			drm_dbg_atomic(plane->dev,
 				       "[PLANE:%d:%s] busy with a previous commit\n",
@@ -2330,7 +2334,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		}
 
 		/* Always track planes explicitly for async pageflip support. */
-		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
+		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc,
+					     nonblock);
 		if (!commit)
 			return -ENOMEM;
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 10b1990bc1f6..0924c322ddfb 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -155,6 +155,13 @@ struct drm_crtc_commit {
 	 * used by the free code to remove the second reference if commit fails.
 	 */
 	bool abort_completion;
+
+	/**
+	 * @nonblock:
+	 *
+	 * Nonblocking commit?
+	 */
+	bool nonblock;
 };
 
 struct __drm_planes_state {
-- 
2.35.1

[PATCH 2/4] drm/i915: Don't reuse commit_work for the cleanup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we reuse the commit_work for a later cleanup step.
Let's not do that so that atomic ioctl handler won't accidentally
wait for the cleanup work when it really wants to just wait on the
commit_tail() part. We'll just add another work struct for the
cleanup.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c       | 6 +++---
 drivers/gpu/drm/i915/display/intel_display_types.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dd008ba8afe3..cd617046e0ee 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7422,7 +7422,7 @@ static void intel_cleanup_dsbs(struct intel_atomic_state *state)
 static void intel_atomic_cleanup_work(struct work_struct *work)
 {
 	struct intel_atomic_state *state =
-		container_of(work, struct intel_atomic_state, base.commit_work);
+		container_of(work, struct intel_atomic_state, cleanup_work);
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
 
 	intel_cleanup_dsbs(state);
@@ -7643,8 +7643,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	 * schedule point (cond_resched()) here anyway to keep latencies
 	 * down.
 	 */
-	INIT_WORK(&state->base.commit_work, intel_atomic_cleanup_work);
-	queue_work(system_highpri_wq, &state->base.commit_work);
+	INIT_WORK(&state->cleanup_work, intel_atomic_cleanup_work);
+	queue_work(system_highpri_wq, &state->cleanup_work);
 }
 
 static void intel_atomic_commit_work(struct work_struct *work)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 298d00a11f47..971e2b1e1b26 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -655,6 +655,7 @@ struct intel_atomic_state {
 
 	struct i915_sw_fence commit_ready;
 
+	struct work_struct cleanup_work;
 	struct llist_node freed;
 };
 
-- 
2.35.1

[PATCH 3/4] drm/atomic: Allow lockless blocking commits

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The easiest way to execute blocking commits locklessly is to just
schedule them onto the workqueue excatly as we do for nonblocking
commits. And to preserve the blocking behaviour of the ioctl we
just flush the work before exiting the kernel.

We do need to reorder the state_put() vs drop_locks() of course
so we don't flush_work() while still holding the locks.

Note that a lot of the current users of drm_atomic_commit()
(eg. lot of the atomic helpers) have the ww_ctx stuff outside
the drm_atomic_state handling. With that structure we can't
actually pull the flush_work() past the drop_locks(). So in
order to make those places actually lockless we'll need
reverse the layers. That is left for a future excercise
and for now we just roll the flush_work() straight into
drm_atomic_commit(), leaving the non-flushing version for
just the atomic ioctl handler.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c      | 32 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_uapi.c | 11 ++++++++---
 include/drm/drm_atomic.h          |  1 +
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..6d728af4e8cf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1411,7 +1411,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 EXPORT_SYMBOL(drm_atomic_check_only);
 
 /**
- * drm_atomic_commit - commit configuration atomically
+ * drm_atomic_commit_noflush - commit configuration atomically, without waiting for the commit
  * @state: atomic configuration to check
  *
  * Note that this function can return -EDEADLK if the driver needed to acquire
@@ -1424,7 +1424,7 @@ EXPORT_SYMBOL(drm_atomic_check_only);
  * Returns:
  * 0 on success, negative error code on failure.
  */
-int drm_atomic_commit(struct drm_atomic_state *state)
+int drm_atomic_commit_noflush(struct drm_atomic_state *state)
 {
 	struct drm_mode_config *config = &state->dev->mode_config;
 	struct drm_printer p = drm_info_printer(state->dev->dev);
@@ -1441,6 +1441,34 @@ int drm_atomic_commit(struct drm_atomic_state *state)
 
 	return config->funcs->atomic_commit(state->dev, state, false);
 }
+EXPORT_SYMBOL(drm_atomic_commit_noflush);
+
+/**
+ * drm_atomic_commit - commit configuration atomically, waiting for the commit to finish
+ * @state: atomic configuration to check
+ *
+ * Note that this function can return -EDEADLK if the driver needed to acquire
+ * more locks but encountered a deadlock. The caller must then do the usual w/w
+ * backoff dance and restart. All other errors are fatal.
+ *
+ * This function will take its own reference on @state.
+ * Callers should always release their reference with drm_atomic_state_put().
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_atomic_commit(struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = drm_atomic_commit_noflush(state);
+	if (ret)
+		return ret;
+
+	flush_work(&state->commit_work);
+
+	return 0;
+}
 EXPORT_SYMBOL(drm_atomic_commit);
 
 /**
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..73ec26fe3393 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1290,6 +1290,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_out_fence_state *fence_state;
+	bool flush = false;
 	int ret = 0;
 	unsigned int i, j, num_fences;
 
@@ -1423,7 +1424,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_nonblocking_commit(state);
 	} else {
-		ret = drm_atomic_commit(state);
+		ret = drm_atomic_commit_noflush(state);
+		flush = ret == 0;
 	}
 
 out:
@@ -1436,10 +1438,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			goto retry;
 	}
 
-	drm_atomic_state_put(state);
-
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
+	if (flush)
+		flush_work(&state->commit_work);
+
+	drm_atomic_state_put(state);
+
 	return ret;
 }
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 0924c322ddfb..d19ce8898bd4 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -740,6 +740,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc);
 
 int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
+int __must_check drm_atomic_commit_noflush(struct drm_atomic_state *state);
 int __must_check drm_atomic_commit(struct drm_atomic_state *state);
 int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
 
-- 
2.35.1

[PATCH 4/4] drm/i915: Make blocking commits lockless

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make blocking commits execute locklessly (just as nonblocking
commits do) by scheduling them onto the workqueues as well.
There will be a later flush_work() done by whoever called
the commit hook to make sure the blocking behaviour of the
ioctl/etc. is preserved.

I also wondered about just dropping the locks straight from the
driver, but I guess whoever called us might still depend on
having the locks so that seems like a terrible idea. Also calling
commit_tail() directly when not holding the locks would then
allow eg. two ioctls to execute full modesets in parallel,
which we don't want as we haven't fully evaluated all modeset
codepaths for concurrency issues. Currently we achieve serial
excution with a combination of an ordered workqueue (for
nonblocking commits) and reliance on the singular connection_mutex
(for blocking commits), and a flush_workqueue() to sync between
the two.

So by always scheduling everything onto the workqueues we
don't have to worry about racing between the lockless direct
commit_tail() calls, and we don't need some kind of new atomic
hook that would do said call for us after dropping the locks.
Also less codepaths in general seems like a beneficial thing.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index cd617046e0ee..18a5f14e7f41 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7771,15 +7771,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 	INIT_WORK(&state->base.commit_work, intel_atomic_commit_work);
 
 	i915_sw_fence_commit(&state->commit_ready);
-	if (nonblock && state->modeset) {
+	if (state->modeset)
 		queue_work(dev_priv->display.wq.modeset, &state->base.commit_work);
-	} else if (nonblock) {
+	else
 		queue_work(dev_priv->display.wq.flip, &state->base.commit_work);
-	} else {
-		if (state->modeset)
-			flush_workqueue(dev_priv->display.wq.modeset);
-		intel_atomic_commit_tail(state);
-	}
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 0/4] drm/atomic: Lockless blocking commits

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

On Fri, 16 Sep 2022 19:33:27 +0300
Ville Syrjala <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I've talked about making blocking commits lockless a few
> times in the past, so here's finally an attempt at it.
> The main benefit I see from this is that TEST_ONLY commits
> no longer getting blocked on the mutexes by parallel blocking
> commits.
> 
> I have a small test here that spools up two threads,
> one does just TEST_ONLY commits in a loop, the other
> does either blocking or non-blocking page flips. Results
> came out as follows on a snb machine here:
> 
> test-only-vs-non-blocking:
> -85319 TEST_ONLY commits in 2000000 usecs, 23 usecs / commit
> +87144 TEST_ONLY commits in 2000006 usecs, 22 usecs / commit
> 
> test-only-vs-blocking:
> -219 TEST_ONLY commits in 2001768 usecs, 9140 usecs / commit
> +82442 TEST_ONLY commits in 2000011 usecs, 24 usecs / commit
> 
> Now, I have no idea if anyone actually cares about lack
> of parallelism due to locked blocking commits or not. Hence
> Cc'd some compositor folks as well. I guess this is more of
> an RFC at this point.
> 
> Also curious to see if CI goes up in smoke or not...

Hi Ville,

thanks for thinking about this. If I understand correctly, the issue
you are solving here happens only when a blocking commit is underway
while TEST_ONLY commits are done. This can only happen if userspace
does the blocking commits from one thread, while another thread is
doing TEST_ONLY probing on the same DRM device. It is inconsequential
whether the two threads target distinct CRTCs or same CRTCs.

If so, this is not a problem for Weston for two reasons:

- Weston is fundamentally single-threaded, so if it does use a blocking
  commit, it's not going to do anything else at the same time.

- Weston practically always uses non-blocking commits.

I cannot imagine those two facts to change.

Ah, but there is a case: KMS leasing!

With leasing you have two processes poking distinct CRTCs on the same
device at the same time. Even if Weston never blocks, an arbitrary
leasing client might, and I presume that would then stall Weston's
TEST_ONLY commits.

I believe working on optimising this could be useful for KMS leasing use
cases, assuming lessees do blocking commits. I don't know if any do.


Thanks,
pq



> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Jonas Ådahl <jadahl@gmail.com>
> 
> Ville Syrjälä (4):
>   drm/atomic: Treat a nonblocking commit following a blocking commit as
>     blocking commit
>   drm/i915: Don't reuse commit_work for the cleanup
>   drm/atomic: Allow lockless blocking commits
>   drm/i915: Make blocking commits lockless
> 
>  drivers/gpu/drm/drm_atomic.c                  | 32 +++++++++++++++++--
>  drivers/gpu/drm/drm_atomic_helper.c           | 19 +++++++----
>  drivers/gpu/drm/drm_atomic_uapi.c             | 11 +++++--
>  drivers/gpu/drm/i915/display/intel_display.c  | 15 +++------
>  .../drm/i915/display/intel_display_types.h    |  1 +
>  include/drm/drm_atomic.h                      |  8 +++++
>  6 files changed, 64 insertions(+), 22 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] drm/atomic: Lockless blocking commits

[Ville Syrjälä]

On Tue, Sep 20, 2022 at 10:34:15AM +0300, Pekka Paalanen wrote:
> On Fri, 16 Sep 2022 19:33:27 +0300
> Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > I've talked about making blocking commits lockless a few
> > times in the past, so here's finally an attempt at it.
> > The main benefit I see from this is that TEST_ONLY commits
> > no longer getting blocked on the mutexes by parallel blocking
> > commits.
> > 
> > I have a small test here that spools up two threads,
> > one does just TEST_ONLY commits in a loop, the other
> > does either blocking or non-blocking page flips. Results
> > came out as follows on a snb machine here:
> > 
> > test-only-vs-non-blocking:
> > -85319 TEST_ONLY commits in 2000000 usecs, 23 usecs / commit
> > +87144 TEST_ONLY commits in 2000006 usecs, 22 usecs / commit
> > 
> > test-only-vs-blocking:
> > -219 TEST_ONLY commits in 2001768 usecs, 9140 usecs / commit
> > +82442 TEST_ONLY commits in 2000011 usecs, 24 usecs / commit
> > 
> > Now, I have no idea if anyone actually cares about lack
> > of parallelism due to locked blocking commits or not. Hence
> > Cc'd some compositor folks as well. I guess this is more of
> > an RFC at this point.
> > 
> > Also curious to see if CI goes up in smoke or not...
> 
> Hi Ville,
> 
> thanks for thinking about this. If I understand correctly, the issue
> you are solving here happens only when a blocking commit is underway
> while TEST_ONLY commits are done. This can only happen if userspace
> does the blocking commits from one thread, while another thread is
> doing TEST_ONLY probing on the same DRM device. It is inconsequential
> whether the two threads target distinct CRTCs or same CRTCs.
> 
> If so, this is not a problem for Weston for two reasons:
> 
> - Weston is fundamentally single-threaded, so if it does use a blocking
>   commit, it's not going to do anything else at the same time.
> 
> - Weston practically always uses non-blocking commits.
> 
> I cannot imagine those two facts to change.

I figured that is likely the case. Thanks for confirming.

> 
> Ah, but there is a case: KMS leasing!
> 
> With leasing you have two processes poking distinct CRTCs on the same
> device at the same time. Even if Weston never blocks, an arbitrary
> leasing client might, and I presume that would then stall Weston's
> TEST_ONLY commits.
> 
> I believe working on optimising this could be useful for KMS leasing use
> cases, assuming lessees do blocking commits. I don't know if any do.

Hmm, yeah didn't even think about leasing. Never have really.

The other reason (one I already forgot) I had for this is
drm_private_obj which has its own lock embbedded inside now.
So currently you have to think hard before actually using one
so as to not make everything block on it. With the locks not
held so much maybe drm_private_obj might become more palatable
for some things.

Oh, I guess there might also be some internal commits (or commit
like things) happening in the driver in some cases, such as DP
link retraining. At least with i915 those currently happen with
the locks held, but maybe could also be made lockless. But I
admit that those should be exceedingly rare situations.

-- 
Ville Syrjälä
Intel