On Fri, 01 Dec 2023 09:31:23 +0000 Simon Ser <cont...@emersion.fr> wrote:
> Thanks for writing these docs! A few comments below. > > On Thursday, November 30th, 2023 at 21:07, André Almeida > <andrealm...@igalia.com> 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
pgpGABErTmv_0.pgp
Description: OpenPGP digital signature
