[PATCH 3/4] drm: introduce DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP

[Simon Ser]

On Tuesday, August 30th, 2022 at 10:08, Ville Syrjälä <ville.syrjala at linux.intel.com> 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.