On Tuesday, August 30th, 2022 at 10:08, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > > In the documentation patch discussion [1], it appears it's not clear what > > drivers should do when async flip isn't possible with the legacy uAPI. > > > > For the atomic uAPI, we need to pick on of these two approaches: > > > > 1. Let the kernel fall back to a sync flip if async isn't possible. This > > simplifies user-space, but then user-space has no reliable way to figure out > > what really happened (sync or async?). That could be fixed with a new > > read-only CRTC prop indicating whether the last flip was async or not. > > However, maybe someone will come up in the future with user-space which > > needs to only apply an update if async flip is possible, in which case this > > approach falls short. > > 2. Make the kernel return EINVAL if async flip isn't possible. This adds more > > complexity to user-space, but enables a more reliable and deterministic > > uAPI. This is also more consistent with the rest of the existing atomic > > uAPI. > > The current behaviour is somewhat a combination of the two. We return > an error if async flip is not possible at all given the current state. > > When async flip is possible we return success, but may still internally > fall back to a sync flip for the first flip. That is required on some > borked hardware that can't switch from sync flips to async flips without > doing an extra sync flip. Also on some other hardware we intentionally > fall back to a sync flip for the first async flip, so that we can > reprogram some display FIFO stuff (aimed to make the subsequent async > flips faster). Hm. Would it be possible for the atomic uAPI to return EINVAL in this case too, to let user-space know what really happened? I suppose user-space could then (mistakenly) assume that async flip is never possible, and never try again? > > Note, current user-space would only need to opportunistically enable async > > flip. IOW, I think that for current user-space plans "async if possible, > > otherwise sync" is good enough. That behavior maps well to the Vulkan present > > modes as well (which says that "this mode *may* result in visible tearing", but > > doesn't guarantee it). > > The current behaviour is to fall back to a blit if the async > flip fails. So you still get the same effective behaviour, just > not as efficient. I think that's a reasonable way to handle it. After some discussion on IRC: the above describes Xorg's behavior. To reproduce this behavior with the atomic uAPI, it is necessary to use approach (2): make the atomic commit fail if async flip isn't possible, to let user-space fall back to a blit. > > Another possible shortcoming of the proposed new uAPI here is that user-space > > cannot submit a single atomic commit which updates multiple CRTCs, and > > individually select which CRTC does an async flip. This could be fixed with > > a "ASYNC_FLIP" CRTC prop which the kernel always resets to 0 on commit. I'm not > > sure we want/need to cross that bridge right now, it would be easy enough to > > add as a second step if some user-space would benefit from it. > > Technically it should really be per-plane since that is what does > the flip. But I have a feeling that allowing a mix of async and > sync in the same commit is just going to make everything more > complicated without really helping anything (async flips won't > happen atomically anyway with anything else). Agreed. > One (crazy?) idea I had for the atomic api is that we could even > reject most of the properties already on the uapi level before anyone > gets to examine the final state. Ie. as soon as the atomic ioctl sees > eg. a gamma LUT property change it would just immediately return > an error if async flip is also requested. Hm, I guess... Although amdgpu doesn't really need this, it already has all of the logic checking for stuff like gamma LUT property change + async. Could maybe be a DRM helper if other drivers need it.
