Message-ID: <40gonZRoP7FjDn_ugL_LpXsqwoSCZtypIe7jiWg0t8lkTx94-gESc60Cuu5eWxivJoZCNg3i-cUG9kNpKQZeYdCJPawDpTSIXivJ_t_a87E=@emersion.fr>
Date:   Fri, 01 Dec 2023 09:31:23 +0000
From:   Simon Ser <contact@...rsion.fr>
To:     André Almeida <andrealmeid@...lia.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        kernel-dev@...lia.com, alexander.deucher@....com,
        christian.koenig@....com, Rob Clark <robdclark@...il.com>,
        Pekka Paalanen <ppaalanen@...il.com>, 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>,
        Randy Dunlap <rdunlap@...radead.org>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Maxime Ripard <mripard@...nel.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Pekka Paalanen <pekka.paalanen@...labora.com>
Subject: Re: [PATCH] drm/doc: Define KMS atomic state set

Thanks for writing these docs! A few comments below.

On Thursday, November 30th, 2023 at 21:07, André Almeida <andrealmeid@...lia.com> wrote:

> +KMS atomic state
> +================
> +
> +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

It would be nice to link DRM_MODE_ATOMIC_TEST_ONLY to the actual docs here.
This can be done with markup such as:

    :c:macro:`DRM_MODE_ATOMIC_TEST_ONLY`

Same applies to other #defines.

> +complete state change is validated but not applied.  Userspace should use this

I'd s/should/can/ here, because there are valid cases where user-space doesn't
really need to test before applying. Applying a state first validates it in the
kernel anwyays.

> +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 try to avoid
> +no-operation changes, so it is safe for userspace to send redundant property
> +settings.  However, not every situation allows for no-op changes, due to the
> +need to acquire locks for some attributes. Userspace needs to be aware that some
> +redundant information might result in oversynchronization issues.  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. As a special exception for VRR needs, explicitly setting
> +FB_ID to its current value is not a no-op.

I'm not sure talking about FB_ID is the right thing to do here. There is
nothing special about FB_ID in particular. For instance, setting CRTC_ID to the
same value as before has the same effect. Talking specifically about FB_ID here
can be surprising for user-space: reading these docs, I'd assume setting
CRTC_ID to the same value as before is a no-op, but in reality it's not.

Instead, I'd suggest explaining how referencing a plane/CRTC/connector in an
atomic commit adds it to the new state, even if there are no effective property
value changes.

> +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 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. Each driver may relax this restriction if they have guarantees that
> +such property change doesn't cause modesets. Userspace can use TEST_ONLY commits
> +to query the driver about this.

This doesn't 100% match reality at the moment, because core DRM now rejects any
async commit which changes FB_ID on a non-primary plane. And there is no way
for drivers to relax this currently.

I'm not sure this is a good place to state such a rule. In the end, it's the
same as always: the kernel will reject commits it can't perform.
DRM_MODE_PAGE_FLIP_ASYNC does not need to be a special case here. Even when
changing only FB_ID, the kernel might reject the commit (e.g. i915 does in some
cases).

Powered by blists - more mailing lists