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

[Christian König]

Am 05.10.23 um 11:57 schrieb Daniel Vetter:
> On Tue, Sep 26, 2023 at 01:05:49PM -0400, Ray Strode wrote:
>> 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.
>>
>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2861
>> Signed-off-by: Ray Strode <rstrode at redhat.com>
> So imo the trouble with this is that we suddenly start to make
> realtime/cpu usage guarantees in the atomic ioctl. That's a _huge_ uapi
> change, because even limited to the case of !ALLOW_MODESET we do best
> effort guarantees at best. And some drivers (again amd's dc) spend a ton
> of cpu time recomputing state even for pure plane changes without any crtc
> changes like dpms on/off (at least I remember some bug reports about
> that). And that state recomputation has to happen synchronously, because
> it always influences the ioctl errno return value.
>
> My take is that you're papering over a performance problem here of the
> "the driver is too slow/wastes too much cpu time". We should fix the
> driver, if that's possible.
>
> Another option would be if userspace drops realtime priorities for these
> known-slow operations. And right now _all_ kms operations are potentially
> cpu and real-time wasters, the entire uapi is best effort.
>
> We can also try to change the atomic uapi to give some hard real-time
> guarantees so that running compositors as SCHED_RT is possible, but that
> - means a very serious stream of bugs to fix all over
> - therefore needs some very wide buy-in from drivers that they're willing
>    to make this guarantee
> - probably needs some really carefully carved out limitations, because
>    there's imo flat-out no way we'll make all atomic ioctl hard time limit
>    bound

Well, we actually have a pending request to support some real time use 
cases with the amdgpu driver.

And as I already said to Alex internally this is not a pile, but a 
mountain of work even when we exclude DC.

Fixing DC to not busy wait for events which raise an interrupt is still 
something we should absolutely do anyway, but that is an ongoing process.

> Also, as König has pointed out, you can roll this duct-tape out in
> userspace by making the commit non-blocking and immediately waiting for
> the fences.

Oh, please don't use my last name like this. It reminds me of my 
military time :)

Regards,
Christian.

>
> One thing I didn't see mention is that there's a very subtle uapi
> difference between non-blocking and blocking:
> - non-blocking is not allowed to get ahead of the previous commit, and
>    will return EBUSY in that case. See the comment in
>    drm_atomic_helper_commit()
> - blocking otoh will just block until any previous pending commit has
>    finished
>
> Not taking that into account in your patch here breaks uapi because
> userspace will suddenly get EBUSY when they don't expect that.
>
> Cheers, Sima
>
>
>> ---
>>   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
>> -- 
>> 2.41.0
>>