On Tue, Sep 26, 2023 at 01:05:49PM -0400, Ray Strode wrote: > From: Ray Strode <rstr...@redhat.com> > > A drm atomic commit can be quite slow on some hardware. It can lead > to a lengthy queue of commands that need to get processed and waited > on before control can go back to user space. > > If user space is a real-time thread, that delay can have severe > consequences, leading to the process getting killed for exceeding > rlimits. > > This commit addresses the problem by always running the slow part of > a commit on a workqueue, separated from the task initiating the > commit. > > This change makes the nonblocking and blocking paths work in the same way, > and as a result allows the task to sleep and not use up its > RLIMIT_RTTIME allocation. > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2861 > Signed-off-by: Ray Strode <rstr...@redhat.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 292e38eb6218..1a1e68d98d38 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2028,64 +2028,63 @@ int drm_atomic_helper_commit(struct drm_device *dev, > * This is the point of no return - everything below never fails except > * when the hw goes bonghits. Which means we can commit the new state on > * the software side now. > */ > > ret = drm_atomic_helper_swap_state(state, true); > if (ret) > goto err; > > /* > * Everything below can be run asynchronously without the need to grab > * any modeset locks at all under one condition: It must be guaranteed > * that the asynchronous work has either been cancelled (if the driver > * supports it, which at least requires that the framebuffers get > * cleaned up with drm_atomic_helper_cleanup_planes()) or completed > * before the new state gets committed on the software side with > * drm_atomic_helper_swap_state(). > * > * This scheme allows new atomic state updates to be prepared and > * checked in parallel to the asynchronous completion of the previous > * update. Which is important since compositors need to figure out the > * composition of the next frame right after having submitted the > * current layout. > * > * NOTE: Commit work has multiple phases, first hardware commit, then > * cleanup. We want them to overlap, hence need system_unbound_wq to > * make sure work items don't artificially stall on each another. > */ > > drm_atomic_state_get(state); > - if (nonblock) > - queue_work(system_unbound_wq, &state->commit_work); > - else > - commit_tail(state); > + queue_work(system_unbound_wq, &state->commit_work); > + if (!nonblock) > + flush_work(&state->commit_work);
Here's my earlier take on this: https://patchwork.freedesktop.org/series/108668/ execpt I went further and moved the flush past the unlock in the end. -- Ville Syrjälä Intel
