On Friday, December 1st, 2023 at 10:57, Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx> 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.
