Re: [PATCH] drm: document DRM_MODE_PAGE_FLIP_EVENT interactions with atomic

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 17, 2025 at 11:40:15AM +0000, Simon Ser wrote:
> On Friday, January 17th, 2025 at 12:32, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > > + * When used with atomic uAPI, one event will be delivered per CRTC included in
> > > + * the atomic commit. A CRTC is included in an atomic commit if one of its
> > > + * properties is set, or if a property is set on a connector or plane linked
> > > + * via the CRTC_ID property to the CRTC. At least one CRTC must be included,
> > > + * and all pulled in CRTCs must be either previously or newly powered on (in
> > > + * other words, a powered off CRTC which stays off cannot be included in the
> > > + * atomic commit).
> > 
> > I don't understand all this stuff about powered off crtcs? If
> > someone sucks in a powered off thing then things will generally
> > work just fine.
> 
> Not with the page-flip event flag:
> 
> 	/*
> 	 * Reject event generation for when a CRTC is off and stays off.
> 	 * It wouldn't be hard to implement this, but userspace has a track
> 	 * record of happily burning through 100% cpu (or worse, crash) when the
> 	 * display pipe is suspended. To avoid all that fun just reject updates
> 	 * that ask for events since likely that indicates a bug in the
> 	 * compositor's drawing loop. This is consistent with the vblank IOCTL
> 	 * and legacy page_flip IOCTL which also reject service on a disabled
> 	 * pipe.
> 	 */
> 	if (new_crtc_state->event &&
> 	    !new_crtc_state->active && !old_crtc_state->active) {
> 		drm_dbg_atomic(crtc->dev,
> 			       "[CRTC:%d:%s] requesting event but off\n",
> 			       crtc->base.id, crtc->name);
> 		return -EINVAL;
> 	}

OK, so we're only talking about userspace stuff here. The kernel
can still pull stuff in without too many issues (apart from the
one mentined below).

> 
> > There is a bit of corner case with the way we internally complete
> > the commits for disabled things (not just crtcs, but also planes
> > and connectors) and that can apparently happen a bit later than
> > the commit completion for the enabled things. That seems to be
> > causing a bit of grief for sway which insists on adding all kinds
> > of disabled planes to every commit:
> > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13410
> 
> Hm, interesting. So including an already-disabled cursor plane in a
> commit may fail the next commit with EBUY? I expect a lot of user-space
> to do this as well, e.g. Weston.

We may need to think if we could move that internal fake commit
completion earlier a bit. But I don't actually remeber the
specifics why it was added (presumably some commit ordering
vs. cleanup problem) so not sure if that's easily doable or not.

-- 
Ville Syrjälä
Intel