linux-kernel - Re: [PATCH] drm/doc: Define KMS atomic state set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZWn1EC04wBSN9hu2@intel.com>
Date:   Fri, 1 Dec 2023 17:00:32 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     André Almeida <andrealmeid@...lia.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Maxime Ripard <mripard@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Pekka Paalanen <pekka.paalanen@...labora.com>,
        'Marek Olšák' <maraeo@...il.com>,
        Michel Dänzer <michel.daenzer@...lbox.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-doc@...r.kernel.org, Pekka Paalanen <ppaalanen@...il.com>,
        Thomas Zimmermann <tzimmermann@...e.de>, kernel-dev@...lia.com,
        alexander.deucher@....com, christian.koenig@....com
Subject: Re: [PATCH] drm/doc: Define KMS atomic state set

On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> From: Pekka Paalanen <pekka.paalanen@...labora.com>
> 
> Specify how the atomic state is maintained between userspace and
> kernel, plus the special case for async flips.
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen@...labora.com>
> Signed-off-by: André Almeida <andrealmeid@...lia.com>
> ---
> 
> This is a standalone patch from the following serie, the other patches are
> already merged:
> https://lore.kernel.org/lkml/20231122161941.320564-1-andrealmeid@igalia.com/
> 
>  Documentation/gpu/drm-uapi.rst | 47 ++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 370d820be248..d0693f902a5c 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -570,3 +570,50 @@ dma-buf interoperability
>  
>  Please see Documentation/userspace-api/dma-buf-alloc-exchange.rst for
>  information on how dma-buf is integrated and exposed within DRM.
> +
> +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
> +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 try to avoid
> +no-operation changes,

Not how things work. The driver may try to avoid some really
expensive operations, but generally it will just blindly blast
the full state to the hardware.

IIRC this was discussed long ago when atomic was being designed
and the general concensus was that the kernel shouldn't generally
do this kind of stuff, and instead we just leave it to userspace
to generate optimal commits.

> so it is safe for userspace to send redundant property
> +settings.

Safe but not optimal. Any object included in the state will cause said
object to be part of the commit, and side effects will also need to be
observed.

So if you add an extra crtc (either directly or indirectly) it will
have a new commit inserted into the queue and thus and any subsequent
commit will either block or be rejected with -EBUSY. Also for directly
added crtcs an event will be emitted once the commit is done.

Any plane added will also need to observe side effects even if the FB
doesn't change, such as invalidating any internal compressed version
of the old FB contents, PSR/DSI command mode/etc. will need to upload
the frame to the display, etc. I suppose we could specify that if no
FB is specified at all then these kind of side effects could be ignored,
but that is certainly not how things are implemented right now.

So for optimal behaviour userspace should be minimizing the commits.

-- 
Ville Syrjälä
Intel