drm/doc: Define KMS atomic state set

Commit Message

André Almeida Nov. 30, 2023, 8:07 p.m. UTC

From: Pekka Paalanen <[e-mail]>

Specify how the atomic state is maintained between userspace and
kernel, plus the special case for async flips.

Signed-off-by: Pekka Paalanen <[e-mail]>
Signed-off-by: André Almeida <[e-mail]>
---

This is a standalone patch from the following serie, the other patches are
already merged:
https://lore.kernel.org/lkml/[e-mail]/

 Documentation/gpu/drm-uapi.rst | 47 ++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Comments

Bagas Sanjaya Dec. 1, 2023, 1:07 a.m. UTC | #1

On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> From: Pekka Paalanen <[e-mail]>
> 
> Specify how the atomic state is maintained between userspace and
> kernel, plus the special case for async flips.
> 
> Signed-off-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: André Almeida <[e-mail]>
> ---
> 
> This is a standalone patch from the following serie, the other patches are
> already merged:
> https://lore.kernel.org/lkml/[e-mail]/
> 
>  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, 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.
> +
> +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.

The wording LGTM, thanks!

Reviewed-by: Bagas Sanjaya <[e-mail]>

Maxime Ripard Dec. 1, 2023, 8:29 a.m. UTC | #2

Hi,

On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> From: Pekka Paalanen <[e-mail]>
> 
> Specify how the atomic state is maintained between userspace and
> kernel, plus the special case for async flips.
> 
> Signed-off-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: André Almeida <[e-mail]>
> ---
> 
> This is a standalone patch from the following serie, the other patches are
> already merged:
> https://lore.kernel.org/lkml/[e-mail]/
> 
>  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?

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.

So yeah, individual object KMS state are indeed complete, but
drm_atomic_state definitely isn't. And it's the whole point of functions
like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
the old/new variants only return a state if it was part of
drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
crtc state into drm_atomic_state if it wasn't part of it.

Maxime

Pekka Paalanen Dec. 1, 2023, 9:06 a.m. UTC | #3

On Fri, 1 Dec 2023 09:29:05 +0100
Maxime Ripard <[e-mail]> wrote:

> Hi,
> 
> On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> > From: Pekka Paalanen <[e-mail]>
> > 
> > Specify how the atomic state is maintained between userspace and
> > kernel, plus the special case for async flips.
> > 
> > Signed-off-by: Pekka Paalanen <[e-mail]>
> > Signed-off-by: André Almeida <[e-mail]>
> > ---
> > 
> > This is a standalone patch from the following serie, the other patches are
> > already merged:
> > https://lore.kernel.org/lkml/[e-mail]/
> > 
> >  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.

All objects are always part of the device KMS state as referred to
in this doc, whether they were mentioned in the atomic commit state set
or not. That's the whole point: all state that was not explicitly
modified remains as it was, and is actively used state by the driver
and hardware. The practical end result state is the same as if all
objects were (redundantly) mentioned.

For example, if you change properties of CRTC 31, it has no effect on
the behaviour of CRTC 54. If CRTC 54 was active, it remains active. If
CRTC 54 had certain property values, it continues to have those
property values. This is opposed to something else; the UAPI could have
been designed to e.g. reset all unmentioned objects to defaults/off by
the atomic commit. Obviously that's not how it works today, so we need
to mention how things do work.

> 
> So yeah, individual object KMS state are indeed complete, but
> drm_atomic_state definitely isn't. And it's the whole point of functions
> like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
> the old/new variants only return a state if it was part of
> drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
> crtc state into drm_atomic_state if it wasn't part of it.

At no point the text is referring to drm_atomic_state or any other
kernel data structure.


Thanks,
pq

Maxime Ripard Dec. 1, 2023, 9:25 a.m. UTC | #4

On Fri, Dec 01, 2023 at 11:06:16AM +0200, Pekka Paalanen wrote:
> On Fri, 1 Dec 2023 09:29:05 +0100
> Maxime Ripard <[e-mail]> wrote:
> 
> > Hi,
> > 
> > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> > > From: Pekka Paalanen <[e-mail]>
> > > 
> > > Specify how the atomic state is maintained between userspace and
> > > kernel, plus the special case for async flips.
> > > 
> > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > Signed-off-by: André Almeida <[e-mail]>
> > > ---
> > > 
> > > This is a standalone patch from the following serie, the other patches are
> > > already merged:
> > > https://lore.kernel.org/lkml/[e-mail]/
> > > 
> > >  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".

> All objects are always part of the device KMS state as referred to
> in this doc, whether they were mentioned in the atomic commit state set
> or not. That's the whole point: all state that was not explicitly
> modified remains as it was, and is actively used state by the driver
> and hardware. The practical end result state is the same as if all
> objects were (redundantly) mentioned.
> 
> For example, if you change properties of CRTC 31, it has no effect on
> the behaviour of CRTC 54. If CRTC 54 was active, it remains active. If
> CRTC 54 had certain property values, it continues to have those
> property values.

I'm not quite sure I followed your previous paragraph, sorry, but we
agree here and it's kind of my point really: CRTC-54 would not be part
of the new KMS state, so claiming that it is complete is confusing.

It's not complete to me precisely because it doesn't contain the state
of all objects.

> This is opposed to something else; the UAPI could have
> been designed to e.g. reset all unmentioned objects to defaults/off by
> the atomic commit. Obviously that's not how it works today, so we need
> to mention how things do work.

Sure, I'm not claiming we should change anything but the wording of that
doc.

> > 
> > So yeah, individual object KMS state are indeed complete, but
> > drm_atomic_state definitely isn't. And it's the whole point of functions
> > like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
> > the old/new variants only return a state if it was part of
> > drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
> > crtc state into drm_atomic_state if it wasn't part of it.
> 
> At no point the text is referring to drm_atomic_state or any other
> kernel data structure.

Then it's even more confusing, because the sentence I was quoting was
"The changes recorded in an atomic commit apply on top the current KMS
state *in the kernel*", which is ambiguous then.

Maxime

Simon Ser Dec. 1, 2023, 9:31 a.m. UTC | #5

Thanks for writing these docs! A few comments below.

On Thursday, November 30th, 2023 at 21:07, André Almeida <[e-mail]> 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).

Pekka Paalanen Dec. 1, 2023, 9:57 a.m. UTC | #6

On Fri, 01 Dec 2023 09:31:23 +0000
Simon Ser <[e-mail]> wrote:

> Thanks for writing these docs! A few comments below.
> 
> On Thursday, November 30th, 2023 at 21:07, André Almeida <[e-mail]> 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.

Those cases a very much an exception. If you want to explain in what
cases testing is not necessary, that's fine to add, but without it I do
want to always recommend testing first. There is no harm in testing too
much, but there is harm in not testing which implies that there is
likely no fallback either. Without fallbacks, the kernel developers
have less room to change things, and the userspace itself is probably
too fragile to be generally useful.

Or, if you think this concern is moot, then why would userspace ever
use testing?

However, I have understood from past kernel discussions that userspace
really does need to test and fall back on test failure in almost all
cases. Otherwise userspace becomes too driver/hardware dependent.

In general, I think recommending best practices with "should" is a good
idea.

> > +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.

Whoa, I never knew that! That's a big surprise!

People have always been talking only about FB_ID so far.

> 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.

So, if a CRTC object is pulled into drm_atomic_state(?) at all, on VRR
it will trigger a new scanout cycle always, avoiding the front porch
timeout?

Yikes.

> > +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).

I think the paragraph is good to drop here, if it's documented in a
more appropriate place.


Thanks,
pq

Pekka Paalanen Dec. 1, 2023, 10:06 a.m. UTC | #7

On Fri, 1 Dec 2023 10:25:09 +0100
Maxime Ripard <[e-mail]> 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 <[e-mail]> wrote:
> >   
> > > Hi,
> > > 
> > > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:  
> > > > From: Pekka Paalanen <[e-mail]>
> > > > 
> > > > Specify how the atomic state is maintained between userspace and
> > > > kernel, plus the special case for async flips.
> > > > 
> > > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > > Signed-off-by: André Almeida <[e-mail]>
> > > > ---
> > > > 
> > > > This is a standalone patch from the following serie, the other patches are
> > > > already merged:
> > > > https://lore.kernel.org/lkml/[e-mail]/
> > > > 
> > > >  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"?

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.
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?

> > All objects are always part of the device KMS state as referred to
> > in this doc, whether they were mentioned in the atomic commit state set
> > or not. That's the whole point: all state that was not explicitly
> > modified remains as it was, and is actively used state by the driver
> > and hardware. The practical end result state is the same as if all
> > objects were (redundantly) mentioned.
> > 
> > For example, if you change properties of CRTC 31, it has no effect on
> > the behaviour of CRTC 54. If CRTC 54 was active, it remains active. If
> > CRTC 54 had certain property values, it continues to have those
> > property values.  
> 
> I'm not quite sure I followed your previous paragraph, sorry, but we
> agree here and it's kind of my point really: CRTC-54 would not be part
> of the new KMS state, so claiming that it is complete is confusing.
> 
> It's not complete to me precisely because it doesn't contain the state
> of all objects.

Did my explanation of what "KMS state" means from userspace perspective
above help?

> > This is opposed to something else; the UAPI could have
> > been designed to e.g. reset all unmentioned objects to defaults/off by
> > the atomic commit. Obviously that's not how it works today, so we need
> > to mention how things do work.  
> 
> Sure, I'm not claiming we should change anything but the wording of that
> doc.
> 
> > > 
> > > So yeah, individual object KMS state are indeed complete, but
> > > drm_atomic_state definitely isn't. And it's the whole point of functions
> > > like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
> > > the old/new variants only return a state if it was part of
> > > drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
> > > crtc state into drm_atomic_state if it wasn't part of it.  
> > 
> > At no point the text is referring to drm_atomic_state or any other
> > kernel data structure.  
> 
> Then it's even more confusing, because the sentence I was quoting was
> "The changes recorded in an atomic commit apply on top the current KMS
> state *in the kernel*", which is ambiguous then.

It's perhaps a misguided attempt to say that the kernel maintains the
complete device state, and that the complete device state is modified
in the kernel. If it helps, the "in the kernel" can be dropped.


Thanks,
pq

Simon Ser Dec. 1, 2023, 10:25 a.m. UTC | #8

On Friday, December 1st, 2023 at 10:57, Pekka Paalanen <[e-mail]> wrote:

> > > +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.
> 
> Those cases a very much an exception. If you want to explain in what
> cases testing is not necessary, that's fine to add, but without it I do
> want to always recommend testing first. There is no harm in testing too
> much, but there is harm in not testing which implies that there is
> likely no fallback either. Without fallbacks, the kernel developers
> have less room to change things, and the userspace itself is probably
> too fragile to be generally useful.
> 
> Or, if you think this concern is moot, then why would userspace ever
> use testing?
> 
> However, I have understood from past kernel discussions that userspace
> really does need to test and fall back on test failure in almost all
> cases. Otherwise userspace becomes too driver/hardware dependent.
> 
> In general, I think recommending best practices with "should" is a good
> idea.

I was mostly thinking about very simple KMS clients that only use the most
basic configuration (full-screen buffer with no scaling/cropping). That's
actually a quite common case.

But I see what you mean here, I don't mind keeping the current wording.

> > > +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.
> 
> Whoa, I never knew that! That's a big surprise!

Aha! Seems like KMS always has a trick up its sleeve to surprise user-space
devs :)

> People have always been talking only about FB_ID so far.
> 
> > 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.
> 
> So, if a CRTC object is pulled into drm_atomic_state(?) at all, on VRR
> it will trigger a new scanout cycle always, avoiding the front porch
> timeout?
> 
> Yikes.

Yeah, I believe so. Any property (regardless of whether the value actually
changed or not) included in the atomic commit may directly (applied on a CRTC
object) or indirectly (applied on a plane/connector linked to a CRTC) pull in
a CRTC and have side-effects. (Also, as noted on IRC, a driver might pull in a
CRTC on its own, e.g. when reconfiguring a DP-MST tree.)

> > > +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).
> 
> I think the paragraph is good to drop here, if it's documented in a
> more appropriate place.

Yeah, maybe we should expand the DRM_MODE_PAGE_FLIP_ASYNC docs a bit.

Maxime Ripard Dec. 1, 2023, 1:20 p.m. UTC | #9

On Fri, Dec 01, 2023 at 12:06:48PM +0200, Pekka Paalanen wrote:
> On Fri, 1 Dec 2023 10:25:09 +0100
> Maxime Ripard <[e-mail]> 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 <[e-mail]> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:  
> > > > > From: Pekka Paalanen <[e-mail]>
> > > > > 
> > > > > Specify how the atomic state is maintained between userspace and
> > > > > kernel, plus the special case for async flips.
> > > > > 
> > > > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > > > Signed-off-by: André Almeida <[e-mail]>
> > > > > ---
> > > > > 
> > > > > This is a standalone patch from the following serie, the other patches are
> > > > > already merged:
> > > > > https://lore.kernel.org/lkml/[e-mail]/
> > > > > 
> > > > >  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.

So maybe we need to rewrite some other parts of the documentation too
then?

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.

> 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?

> > > All objects are always part of the device KMS state as referred to
> > > in this doc, whether they were mentioned in the atomic commit state set
> > > or not. That's the whole point: all state that was not explicitly
> > > modified remains as it was, and is actively used state by the driver
> > > and hardware. The practical end result state is the same as if all
> > > objects were (redundantly) mentioned.
> > > 
> > > For example, if you change properties of CRTC 31, it has no effect on
> > > the behaviour of CRTC 54. If CRTC 54 was active, it remains active. If
> > > CRTC 54 had certain property values, it continues to have those
> > > property values.  
> > 
> > I'm not quite sure I followed your previous paragraph, sorry, but we
> > agree here and it's kind of my point really: CRTC-54 would not be part
> > of the new KMS state, so claiming that it is complete is confusing.
> > 
> > It's not complete to me precisely because it doesn't contain the state
> > of all objects.
> 
> Did my explanation of what "KMS state" means from userspace perspective
> above help?
> 
> > > This is opposed to something else; the UAPI could have
> > > been designed to e.g. reset all unmentioned objects to defaults/off by
> > > the atomic commit. Obviously that's not how it works today, so we need
> > > to mention how things do work.  
> > 
> > Sure, I'm not claiming we should change anything but the wording of that
> > doc.
> > 
> > > > 
> > > > So yeah, individual object KMS state are indeed complete, but
> > > > drm_atomic_state definitely isn't. And it's the whole point of functions
> > > > like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
> > > > the old/new variants only return a state if it was part of
> > > > drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
> > > > crtc state into drm_atomic_state if it wasn't part of it.  
> > > 
> > > At no point the text is referring to drm_atomic_state or any other
> > > kernel data structure.  
> > 
> > Then it's even more confusing, because the sentence I was quoting was
> > "The changes recorded in an atomic commit apply on top the current KMS
> > state *in the kernel*", which is ambiguous then.
> 
> It's perhaps a misguided attempt to say that the kernel maintains the
> complete device state, and that the complete device state is modified
> in the kernel. If it helps, the "in the kernel" can be dropped.

Yeah, that would probably help too

Maxime

Ville Syrjälä Dec. 1, 2023, 3 p.m. UTC | #10

On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> From: Pekka Paalanen <[e-mail]>
> 
> Specify how the atomic state is maintained between userspace and
> kernel, plus the special case for async flips.
> 
> Signed-off-by: Pekka Paalanen <[e-mail]>
> Signed-off-by: André Almeida <[e-mail]>
> ---
> 
> This is a standalone patch from the following serie, the other patches are
> already merged:
> https://lore.kernel.org/lkml/[e-mail]/
> 
>  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.

Pekka Paalanen Dec. 1, 2023, 4:03 p.m. UTC | #11

On Fri, 1 Dec 2023 14:20:55 +0100
Maxime Ripard <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > >     
> > > > > Hi,
> > > > > 
> > > > > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:    
> > > > > > From: Pekka Paalanen <[e-mail]>
> > > > > > 
> > > > > > Specify how the atomic state is maintained between userspace and
> > > > > > kernel, plus the special case for async flips.
> > > > > > 
> > > > > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > > > > Signed-off-by: André Almeida <[e-mail]>
> > > > > > ---
> > > > > > 
> > > > > > This is a standalone patch from the following serie, the other patches are
> > > > > > already merged:
> > > > > > https://lore.kernel.org/lkml/[e-mail]/
> > > > > > 
> > > > > >  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.

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?

> 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"?


Thanks,
pq

> > > > All objects are always part of the device KMS state as referred to
> > > > in this doc, whether they were mentioned in the atomic commit state set
> > > > or not. That's the whole point: all state that was not explicitly
> > > > modified remains as it was, and is actively used state by the driver
> > > > and hardware. The practical end result state is the same as if all
> > > > objects were (redundantly) mentioned.
> > > > 
> > > > For example, if you change properties of CRTC 31, it has no effect on
> > > > the behaviour of CRTC 54. If CRTC 54 was active, it remains active. If
> > > > CRTC 54 had certain property values, it continues to have those
> > > > property values.    
> > > 
> > > I'm not quite sure I followed your previous paragraph, sorry, but we
> > > agree here and it's kind of my point really: CRTC-54 would not be part
> > > of the new KMS state, so claiming that it is complete is confusing.
> > > 
> > > It's not complete to me precisely because it doesn't contain the state
> > > of all objects.  
> > 
> > Did my explanation of what "KMS state" means from userspace perspective
> > above help?
> >   
> > > > This is opposed to something else; the UAPI could have
> > > > been designed to e.g. reset all unmentioned objects to defaults/off by
> > > > the atomic commit. Obviously that's not how it works today, so we need
> > > > to mention how things do work.    
> > > 
> > > Sure, I'm not claiming we should change anything but the wording of that
> > > doc.
> > >   
> > > > > 
> > > > > So yeah, individual object KMS state are indeed complete, but
> > > > > drm_atomic_state definitely isn't. And it's the whole point of functions
> > > > > like drm_atomic_get_crtc_state() vs drm_atomic_get_old/new_crtc_state:
> > > > > the old/new variants only return a state if it was part of
> > > > > drm_atomic_state to begin with. drm_atomic_get_crtc_state() brings the
> > > > > crtc state into drm_atomic_state if it wasn't part of it.    
> > > > 
> > > > At no point the text is referring to drm_atomic_state or any other
> > > > kernel data structure.    
> > > 
> > > Then it's even more confusing, because the sentence I was quoting was
> > > "The changes recorded in an atomic commit apply on top the current KMS
> > > state *in the kernel*", which is ambiguous then.  
> > 
> > It's perhaps a misguided attempt to say that the kernel maintains the
> > complete device state, and that the complete device state is modified
> > in the kernel. If it helps, the "in the kernel" can be dropped.  
> 
> Yeah, that would probably help too
> 
> Maxime

Pekka Paalanen Dec. 1, 2023, 4:16 p.m. UTC | #12

On Fri, 1 Dec 2023 17:00:32 +0200
Ville Syrjälä <[e-mail]> wrote:

> On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> > From: Pekka Paalanen <[e-mail]>
> > 
> > Specify how the atomic state is maintained between userspace and
> > kernel, plus the special case for async flips.
> > 
> > Signed-off-by: Pekka Paalanen <[e-mail]>
> > Signed-off-by: André Almeida <[e-mail]>
> > ---
> > 
> > This is a standalone patch from the following serie, the other patches are
> > already merged:
> > https://lore.kernel.org/lkml/[e-mail]/
> > 
> >  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.

I don't think userspace ever got that memo. If I was cheeky, I could
ask where that is documented, so you could point at it and say "told
you so".

When I was working on Weston atomic KMS support many years ago, I
created a framework that emitted KMS property changes only when they
actually needed changing. By review feedback (*), all that machinery was
dropped in a re-design, and today Weston always emits all KMS
properties it knows to program for a specific CRTC update including all
relevant planes and connectors.

(*) Why do we need to repeat the same state tracking that the kernel
does anyway, and also risk getting out of sync with the kernel due to
bugs which then become more difficult to diagnose. I guess (assumed)
kernel internals leaked to userspace. Oops.

> > 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.

It is not too hard to keep CRTCs well separated, until the kernel
driver decides under the hood to pull in an unwanted CRTC.

But yes, that caveat could use extending in the doc.

> 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.

Well, this is all surprise news to me.

> So for optimal behaviour userspace should be minimizing the commits.
> 


Thanks,
pq

Ville Syrjälä Dec. 1, 2023, 6:09 p.m. UTC | #13

On Fri, Dec 01, 2023 at 06:16:16PM +0200, Pekka Paalanen wrote:
> On Fri, 1 Dec 2023 17:00:32 +0200
> Ville Syrjälä <[e-mail]> wrote:
> 
> > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:
> > > From: Pekka Paalanen <[e-mail]>
> > > 
> > > Specify how the atomic state is maintained between userspace and
> > > kernel, plus the special case for async flips.
> > > 
> > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > Signed-off-by: André Almeida <[e-mail]>
> > > ---
> > > 
> > > This is a standalone patch from the following serie, the other patches are
> > > already merged:
> > > https://lore.kernel.org/lkml/[e-mail]/
> > > 
> > >  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.
> 
> I don't think userspace ever got that memo. If I was cheeky, I could
> ask where that is documented, so you could point at it and say "told
> you so".

Probably not docuemented anywhere.

> 
> When I was working on Weston atomic KMS support many years ago, I
> created a framework that emitted KMS property changes only when they
> actually needed changing. By review feedback (*), all that machinery was
> dropped in a re-design, and today Weston always emits all KMS
> properties it knows to program for a specific CRTC update including all
> relevant planes and connectors.
> 
> (*) Why do we need to repeat the same state tracking that the kernel
> does anyway, and also risk getting out of sync with the kernel due to
> bugs which then become more difficult to diagnose. I guess (assumed)
> kernel internals leaked to userspace. Oops.

The kernel does track the full state sure, but it doesn't generally
go out of its way to figure out what specifically changed in that state.
Doing so would be a lot of extra checks, and kinda less convenient to
do inside the driver since at that point the state is already spread 
all over the various structures. And the fact that those structures
are a mismash of uapi and internal bits of state (and other metadata 
for the single commit that really shouldn't be stored there) doesn't
help matters. I did propose to split the state cleanly into pure uapi
vs. internal stuff but that didn't gain any traction unfortunately.

So I think it might be simpler to do on the uapi property level. It may
result in a somewhat coarser idea of what changed, but it avoids having
to track down all the little bits of state everwhere that could have
changed in response to a single property changing. The kernel could do
that I suppose, but someone would need to come up with a good way to
track that information. Currently there are a handful of foo_changed
booleans ad-hocced here and there, but nothing consistent that covers
everything.

> 
> > > 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.
> 
> It is not too hard to keep CRTCs well separated,

Sure. But the way this was worded implied that you can just throw
everything and the kitchen sink into the commit without any
repercussions, which is not the case.

> until the kernel
> driver decides under the hood to pull in an unwanted CRTC.

That is sadly needed too sometimes. Hardware design is often
a bit disappointing.

> 
> But yes, that caveat could use extending in the doc.
> 
> > 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.
> 
> Well, this is all surprise news to me.
> 
> > So for optimal behaviour userspace should be minimizing the commits.
> > 
> 
> 
> Thanks,
> pq

Maxime Ripard Dec. 4, 2023, 9:21 a.m. UTC | #14

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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > >     
> > > > > > Hi,
> > > > > > 
> > > > > > On Thu, Nov 30, 2023 at 05:07:40PM -0300, André Almeida wrote:    
> > > > > > > From: Pekka Paalanen <[e-mail]>
> > > > > > > 
> > > > > > > Specify how the atomic state is maintained between userspace and
> > > > > > > kernel, plus the special case for async flips.
> > > > > > > 
> > > > > > > Signed-off-by: Pekka Paalanen <[e-mail]>
> > > > > > > Signed-off-by: André Almeida <[e-mail]>
> > > > > > > ---
> > > > > > > 
> > > > > > > This is a standalone patch from the following serie, the other patches are
> > > > > > > already merged:
> > > > > > > https://lore.kernel.org/lkml/[e-mail]/
> > > > > > > 
> > > > > > >  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

Maxime Ripard Dec. 4, 2023, 12:20 p.m. UTC | #15

On Fri, Dec 01, 2023 at 08:09:22PM +0200, Ville Syrjälä wrote:
> > When I was working on Weston atomic KMS support many years ago, I
> > created a framework that emitted KMS property changes only when they
> > actually needed changing. By review feedback (*), all that machinery was
> > dropped in a re-design, and today Weston always emits all KMS
> > properties it knows to program for a specific CRTC update including all
> > relevant planes and connectors.
> > 
> > (*) Why do we need to repeat the same state tracking that the kernel
> > does anyway, and also risk getting out of sync with the kernel due to
> > bugs which then become more difficult to diagnose. I guess (assumed)
> > kernel internals leaked to userspace. Oops.
> 
> The kernel does track the full state sure, but it doesn't generally
> go out of its way to figure out what specifically changed in that state.
> Doing so would be a lot of extra checks, and kinda less convenient to
> do inside the driver since at that point the state is already spread 
> all over the various structures. And the fact that those structures
> are a mismash of uapi and internal bits of state (and other metadata 
> for the single commit that really shouldn't be stored there) doesn't
> help matters. I did propose to split the state cleanly into pure uapi
> vs. internal stuff but that didn't gain any traction unfortunately.

Also, that's not how drivers are modelled, so even though we could
possibly figure out the entire state of the device, we don't have any
code for it because no one really needs to.

Maxime

Pekka Paalanen Dec. 5, 2023, 8:35 a.m. UTC | #16

On Mon, 4 Dec 2023 10:21:03 +0100
Maxime Ripard <[e-mail]> wrote:

> 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 <[e-mail]> wrote:
> >   
> > > On Fri, Dec 01, 2023 at 12:06:48PM +0200, Pekka Paalanen wrote:  

...

> > 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.

I think the word "global" should be simply avoided. Nothing here is
truly global (machine wide? kernel instance wide? worldwide like
UUID?), and its meaning varies by speaker and context.


Thanks,
pq

Patch

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, 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.
+
+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.