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

[Pekka Paalanen]

On Fri, 01 Dec 2023 09:31:23 +0000
Simon Ser <contact@xxxxxxxxxxx> wrote:

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

Attachment: pgp0LB4GdctKX.pgp
Description: OpenPGP digital signature