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

["Michel Dänzer" <michel.daenzer@mailbox.org>]

On 9/28/23 08:56, Christian König wrote:
> Am 27.09.23 um 22:25 schrieb Ray Strode:
>> On Wed, Sep 27, 2023 at 4:05 AM Christian König
>> <christian.koenig@amd.com> wrote:
> 
>>> 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.
>> Well, first I don't think that's very convenient. You're talking about
>> a per-plane property, so there would need to be a separate file
>> descriptor allocated for every plane, right? and user-space would have
>> to block on all of them before proceeding?

OUT_FENCE_PTR is a per-CRTC property, not per-plane. Also, at least in this particular case, a single sync file (not dma_fence) for any CRTC might suffice.


>> Also, are you sure that works in all cases? The problematic case we're facing right >> now is when all planes and all crtcs are getting disabled. I'm just skimming
>> over the code here, but I see this:
>>
>> →       for_each_new_crtc_in_state(state, crtc, crtc_state, i) {•
>> ...
>> →       →       if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {•
>> ...
>> →       →       →       e = create_vblank_event(crtc, arg->user_data);•
>> ...
>> →       →       →       crtc_state->event = e;•
>> →       →       }•
>> ...
>> →       →       if (fence_ptr) {•
>> ...
>> →       →       →       fence = drm_crtc_create_fence(crtc);•
>> ...
>> →       →       →       ret = setup_out_fence(&f[(*num_fences)++], fence);•
>> ...
>> →       →       →       crtc_state->event->base.fence = fence;•
>> →       →       }•
>>
>> Is the code really going to allocate a vblank_event when all the
>> crtc's are disabled ? I guess it's possible, but that's
>> counterintuitive to me. I really don't know. You're confident a set of
>> fences will actually work?
> 
> No when you disable everything there is of course no fence allocated.

I don't see why not. "new_crtc_in_state" in this code means the atomic commit contains some state for the CRTC (such as setting the OUT_FENCE_PTR property), it could disable or leave it disabled though. I can't see any other code which would prevent this from working with a disabled CRTC, I could be missing something though.


> But then you also should never see anything waiting for to long to actually be able to trigger the RLIMIT_RTTIME.

amdgpu DC exceeds 200ms on some setups, see the linked issue.


>> Regardless, this seems like a roundabout way to address a problem that
>> we could just ... fix.

Handling modesets asynchronously does seem desirable in mutter to me. E.g. it would allow doing modesets in parallel with modesets or other atomic commits on other GPUs.


> From what I know RLIMIT_RTTIME usually works in units of multiple seconds. 

RealtimeKit seems to allow 200ms maximum.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer