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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote: > > > > > > > From: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> > > > > > > > > > > > > > > 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@xxxxxxxxxxxxx> > > > > > > > Signed-off-by: André Almeida <andrealmeid@xxxxxxxxxx> > > > > > > > --- > > > > > > > > > > > > > > This is a standalone patch from the following serie, the other patches are > > > > > > > already merged: > > > > > > > https://lore.kernel.org/lkml/20231122161941.320564-1-andrealmeid@xxxxxxxxxx/ > > > > > > > > > > > > > > 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
Attachment:
signature.asc
Description: PGP signature
