| Message ID | 20220916163331.6849-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/atomic: Lockless blocking commits | expand |
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(-) >
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.
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(-)