[PATCH] drm/atomic: Perform blocking commits on workqueue

[Christian König]

Am 26.09.23 um 19:05 schrieb Ray Strode:
> From: Ray Strode <rstrode at 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.

Well this patch made me laugh :)

I'm not an expert for that stuff, but as far as I know the whole purpose 
of the blocking functionality is to make sure that the CPU overhead 
caused by the commit is accounted to the right process.

So what you are suggesting here is to actually break that functionality 
and that doesn't seem to make sense.

When it's really not desirable to account the CPU overhead to the 
process initiating it then you probably rather want to use an non 
blocking commit plus a dma_fence to wait for the work to end from userspace.

Regards,
Christian.

>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2861
> Signed-off-by: Ray Strode <rstrode at 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);
>   
>   	return 0;
>   
>   err:
>   	drm_atomic_helper_cleanup_planes(dev, state);
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_commit);
>   
>   /**
>    * DOC: implementing nonblocking commit
>    *
>    * Nonblocking atomic commits should use struct &drm_crtc_commit to sequence
>    * different operations against each another. Locks, especially struct
>    * &drm_modeset_lock, should not be held in worker threads or any other
>    * asynchronous context used to commit the hardware state.
>    *
>    * drm_atomic_helper_commit() implements the recommended sequence for
>    * nonblocking commits, using drm_atomic_helper_setup_commit() internally:
>    *
>    * 1. Run drm_atomic_helper_prepare_planes(). Since this can fail and we
>    * need to propagate out of memory/VRAM errors to userspace, it must be called
>    * synchronously.
>    *
>    * 2. Synchronize with any outstanding nonblocking commit worker threads which
>    * might be affected by the new state update. This is handled by
>    * drm_atomic_helper_setup_commit().
>    *
>    * Asynchronous workers need to have sufficient parallelism to be able to run
>    * different atomic commits on different CRTCs in parallel. The simplest way to