Message-ID: <20230703113836.60869599@eldfell>
Date:   Mon, 3 Jul 2023 11:38:36 +0300
From:   Pekka Paalanen <ppaalanen@...il.com>
To:     André Almeida <andrealmeid@...lia.com>
Cc:     dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, wayland-devel@...ts.freedesktop.org,
        kernel-dev@...lia.com, alexander.deucher@....com,
        christian.koenig@....com, pierre-eric.pelloux-prayer@....com,
        Simon Ser <contact@...rsion.fr>,
        Rob Clark <robdclark@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Daniel Stone <daniel@...ishbar.org>,
        'Marek Olšák' <maraeo@...il.com>,
        Dave Airlie <airlied@...il.com>,
        Michel Dänzer <michel.daenzer@...lbox.org>,
        Italo Nicola <italonicola@...labora.com>,
        Randy Dunlap <rdunlap@...radead.org>, hwentlan@....com,
        joshua@...ggi.es, ville.syrjala@...ux.intel.com
Subject: Re: [PATCH v4 6/6] drm/doc: Define KMS atomic state set

On Fri, 30 Jun 2023 23:09:17 -0300
André Almeida <andrealmeid@...lia.com> wrote:

> Specify how the atomic state is maintained between userspace and
> kernel, plus the special case for async flips.
> 
> Signed-off-by: André Almeida <andrealmeid@...lia.com>
> ---
> v4: new patch
> ---
>  Documentation/gpu/drm-uapi.rst | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 65fb3036a580..5464376051cc 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -486,3 +486,22 @@ and the CRTC index is its position in this array.
>  
>  .. kernel-doc:: include/uapi/drm/drm_mode.h
>     :internal:
> +
> +KMS atomic state
> +================
> +
> +If a userspace using the DRM atomic API would like to change the modeset, it

s/modeset/video mode/

> +needs to do in an atomic way, changing all desired properties in a single
> +commit.

This applies any and all state changes, not only video modes. Plane
configuration is a good example, where video mode does not change.

> Following commits may contain the same properties again, as if they were
> +new. The kernel can then judge if those properties requires modesetting and real
> +changes, or it's just the same state as before. In summary, userspace commits do
> +not need to set the minimal state as possible and can commit redundant
> +information, and the kernel will ignore it.

Maybe the whole paragraph should be more like... ahem, looks I got a
bit carried away in writing this. Please, take what's useful, I'm not
sure if any of this has been actually documented before:


An atomic commit can change multiple KMS properties in an atomic
fashion, without ever applying intermediate or partial state changes.
Either the whole commit succeeds or fails, and it will never be applied
partially. This is the fundamental improvement of the atomic API over
the older non-atomic API which is referred to as the "legacy API".
Applying intermediate state could unexpectedly fail, cause visible
glitches, or delay reaching the final state.

An atomic commit can be flagged with DRM_MODE_ATOMIC_TEST_ONLY,
which means the complete state change is validated but not applied.
Userspace should use this flag to validate any state change before
asking to apply it. If validation fails for any reason, userspace
should attempt to fall back to another, perhaps simpler, final state.
This allows userspace to probe for various configurations without
causing visible glitches on screen and without the need to undo a
probing change.

The changes recorded in an atomic commit apply on top the current KMS
state in the kernel. Hence, the complete new KMS state is the complete
old KMS state with the committed property settings done on top. The
kernel will automatically avoid no-operation changes, so it is safe and
even expected for userspace to send redundant property settings.
No-operation changes do not count towards actually needed changes, e.g.
setting MODE_ID to a different blob with identical contents as the
current KMS state shall not be a modeset on its own.

A "modeset" is a change in KMS state that might enable, disable, or
temporarily disrupt the emitted video signal, possibly causing visible
glitches on screen. A modeset may also take considerably more time to
complete than other kinds of changes, and the video sink might also
need time to adapt to the new signal properties. Therefore a modeset
must be explicitly allowed with the flag DRM_MODE_ATOMIC_ALLOW_MODESET.
This in combination with DRM_MODE_ATOMIC_TEST_ONLY allows userspace to
determine if a state change is likely to cause visible disruption on
screen and avoid such changes when end users do not expect them.

> +
> +An observation must be made for atomic operations with DRM_MODE_PAGE_FLIP_ASYNC.
> +In such scenarios properties values can be sent, but the if they change
> +something, the kernel will reject the flip. This is done because property
> +changes can lead to modesetting, that would defeat the goal of flipping as fast
> +as possible. The only exceptions are the framebuffer ID to be flipped to and
> +mode IDs changes, which could be referring to an identical mode, thus not
> +requiring modeset.

That's a bit... roundabout way to describe it. Doesn't async flip want
to prevent all kinds of other-than-FB changes, not only the modesetting
ones? Modesets are already gated by ALLOW_MODESET anyway. How about
something like:

An atomic commit with the flag DRM_MODE_PAGE_FLIP_ASYNC is allowed to
effectively change only the FB_ID property on any planes. No-operation
changes are ignored as always. Changing any other property will cause
the commit to be rejected.


If you want to take these and need my Sob, that would be
Signed-off-by: Pekka Paalanen <pekka.paalanen@...labora.com>


Thanks,
pq

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists