Message-ID: <dybgeyjqoh2rjjrvbb5nrnallx63tano2drmxgsgde6n5w6wza@23cfserg7mui>
Date:   Mon, 4 Dec 2023 10:21:03 +0100
From:   Maxime Ripard <mripard@...nel.org>
To:     Pekka Paalanen <pekka.paalanen@...labora.com>
Cc:     André Almeida <andrealmeid@...lia.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        kernel-dev@...lia.com, alexander.deucher@....com,
        christian.koenig@....com, Simon Ser <contact@...rsion.fr>,
        Rob Clark <robdclark@...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>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Subject: Re: [PATCH] drm/doc: Define KMS atomic state set

Hi

On Fri, Dec 01, 2023 at 06:03:48PM +0200, Pekka Paalanen wrote:
> On Fri, 1 Dec 2023 14:20:55 +0100
> Maxime Ripard <mripard@...nel.org> wrote:
> 
> > On Fri, Dec 01, 2023 at 12:06:48PM +0200, Pekka Paalanen wrote:
> > > On Fri, 1 Dec 2023 10:25:09 +0100
> > > Maxime Ripard <mripard@...nel.org> wrote:
> > >   
> > > > On Fri, Dec 01, 2023 at 11:06:16AM +0200, Pekka Paalanen wrote:  
> > > > > On Fri, 1 Dec 2023 09:29:05 +0100
> > > > > Maxime Ripard <mripard@...nel.org> wrote:
> > > > >     
> > > > > > Hi,
> > > > > > 
> > > > > > 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      
> > > > > > 
> > > > > > That part is pretty confusing to me.
> > > > > > 
> > > > > > What are you calling the current and old KMS state?    
> > > > > 
> > > > > Current = old, if you read that "current" is the KMS state before
> > > > > considering the atomic commit at hand.
> > > > >     
> > > > > > What's confusing to me is that, yes, what you're saying is true for a
> > > > > > given object: if it was part of the commit, the new state is the old
> > > > > > state + whatever the new state changed.
> > > > > > 
> > > > > > However, if that object wasn't part of the commit at all, then it's
> > > > > > completely out of the old or new global KMS state.    
> > > > > 
> > > > > This is not talking about kernel data structures at all. This is
> > > > > talking about how KMS looks from the userspace point of view.    
> > > > 
> > > > I mean, that's also true from the userspace point of view. You can very
> > > > well commit only a single property on a single object, and only that
> > > > object will be part of the "global KMS state".  
> > > 
> > > What is "global KMS state"?  
> > 
> > struct drm_atomic_state, ie. the object holding the entire new commit content.
> > 
> > > As a userspace developer, the global KMS state is the complete, total,
> > > hardware and driver instance state. It's not any kind of data
> > > structure, but it is all the condition and all the programming of the
> > > whole device (hardware + driver instance) at any specific time instant.  
> > 
> > That was my understanding, and assumption, too.
> > 
> > I think part of the issue is that drm_atomic_state is documented as "the
> > global state object for atomic updates" which kind of implies that it
> > holds *everything*, except that an atomic update can be partial.
> 
> I haven't read such doc, and I never intended to refer to struct
> drm_atomic_state. It very much sounds like it's not what I mean. I
> avoid reading kernel internals docs, they are uninteresting to
> userspace developers.

Sure, but I'd assume (and kind of hope) that kernel devs will read the
UAPI docs at some point too :)

> Is it really "global" too? Or is it device-wide? Or is it just the bits
> that userspace bothered to mention in an atomic commit?

As far as I'm concerned, global == "device-wide", so I'm not entirely
sure what is the distinction you want to raise here, so I might be off.

But to answer the latter part of your question, drm_atomic_state
contains the changes of all the objects affected by the commit userspace
mentioned to bother. Which is is why I found the "global" to be
confusing, because it's not a device-wide-global state, it's a
commit-global state.

> > So maybe we need to rewrite some other parts of the documentation too
> > then?
> 
> I guess.
> 
> > Or s/drm_atomic_state/drm_atomic_update/ so we don't have two slightly
> > different definitions of what a state is?
> > 
> > Because, yeah, at the moment we have our object state that is the
> > actual, entire, state of the device but the global atomic state is a
> > collection of object state but isn't the entire state of the device in
> > most cases.
> > 
> > If we get rid of the latter, then there's no ambiguity anymore and your
> > sentence makes total sense.
> 
> I have no idea of kernel internals. Userspace should not care about
> kernel internals as that would mean the kernel internals become UABI.
> Some internals leak into UABI anyway as observable behaviour, but it
> could be worse.
> 
> The complete device state is a vague, abstract concept. I do not expect
> it to be an actual C struct. It is hardware-specific, too.
> 
> > > It is not related to any atomic commit or UAPI call, it is how the
> > > hardware is currently programmed.
> > > 
> > > How can we make that clear?
> > > 
> > > Should "KMS state" be replaced with "complete device state" or
> > > something similar?  
> > 
> > I know I've been bitten by that ambiguity, and the part of the doc I've
> > replied too mentions the "KMS state in the kernel" and an atomic commit,
> > so it's easy to make the parallel with drm_atomic_state here.
> > 
> > I guess we can make it clearer that it's from the userspace then?
> 
> It's not from userspace. It is a concept from the userspace
> perspective. I'm not sure how to make that more clear.
> 
> From userspace perspective it looks like the kernel maintains all of a
> device's state. What would you call this "all of a device's state as
> maintained by the kernel"?

Like I said, I think most of the confusion comes from the kernel doc,
not your patch.

I'll send a patch to s/drm_atomic_state/drm_atomic_update/, we'll see
how it goes.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists