| Message ID | 20250116162528.2235991-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: document DRM_MODE_PAGE_FLIP_EVENT interactions with atomic | expand |
On Thu, 16 Jan 2025 16:25:35 +0000 Simon Ser <contact@emersion.fr> wrote: > It's not obvious off-hand which CRTCs will get a page-flip event > when using this flag in an atomic commit, because it's all > implicitly implied based on the contents of the atomic commit. > Document requirements for using this flag and > and? > Note, because prepare_signaling() runs right after > drm_atomic_set_property() calls, page-flip events are not delivered > for CRTCs pulled in later by DRM core (e.g. on modeset by > drm_atomic_helper_check_modeset()) or the driver (e.g. other CRTCs > sharing a DP-MST connector). > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Simona Vetter <simona.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > Cc: David Turner <david.turner@raspberrypi.com> > --- > include/uapi/drm/drm_mode.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index c082810c08a8..a122bea25593 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -962,6 +962,14 @@ struct hdr_output_metadata { > * Request that the kernel sends back a vblank event (see > * struct drm_event_vblank) with the &DRM_EVENT_FLIP_COMPLETE type when the > * page-flip is done. > + * > + * 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). Sounds right. I imagine this doc needs to be extended when drm_colorop lands, as yet another way to pull in a CRTC. Wasn't this also conditional on the DRM_CAP_CRTC_IN_VBLANK_EVENT or did userspace really need to count the events even without it? Nevertheless, should there be a "see also DRM_CAP_CRTC_IN_VBLANK_EVENT"? > */ > #define DRM_MODE_PAGE_FLIP_EVENT 0x01 > /** Thanks, pq
On Thu, Jan 16, 2025 at 04:25:35PM +0000, Simon Ser wrote: > It's not obvious off-hand which CRTCs will get a page-flip event > when using this flag in an atomic commit, because it's all > implicitly implied based on the contents of the atomic commit. > Document requirements for using this flag and > > Note, because prepare_signaling() runs right after > drm_atomic_set_property() calls, page-flip events are not delivered > for CRTCs pulled in later by DRM core (e.g. on modeset by > drm_atomic_helper_check_modeset()) or the driver (e.g. other CRTCs > sharing a DP-MST connector). > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Simona Vetter <simona.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > Cc: David Turner <david.turner@raspberrypi.com> > --- > include/uapi/drm/drm_mode.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index c082810c08a8..a122bea25593 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -962,6 +962,14 @@ struct hdr_output_metadata { > * Request that the kernel sends back a vblank event (see > * struct drm_event_vblank) with the &DRM_EVENT_FLIP_COMPLETE type when the > * page-flip is done. > + * > + * 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. 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
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; } > 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.
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.
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index c082810c08a8..a122bea25593 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -962,6 +962,14 @@ struct hdr_output_metadata { * Request that the kernel sends back a vblank event (see * struct drm_event_vblank) with the &DRM_EVENT_FLIP_COMPLETE type when the * page-flip is done. + * + * 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). */ #define DRM_MODE_PAGE_FLIP_EVENT 0x01 /**
It's not obvious off-hand which CRTCs will get a page-flip event when using this flag in an atomic commit, because it's all implicitly implied based on the contents of the atomic commit. Document requirements for using this flag and Note, because prepare_signaling() runs right after drm_atomic_set_property() calls, page-flip events are not delivered for CRTCs pulled in later by DRM core (e.g. on modeset by drm_atomic_helper_check_modeset()) or the driver (e.g. other CRTCs sharing a DP-MST connector). Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Simona Vetter <simona.vetter@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Pekka Paalanen <pekka.paalanen@collabora.com> Cc: David Turner <david.turner@raspberrypi.com> --- include/uapi/drm/drm_mode.h | 8 ++++++++ 1 file changed, 8 insertions(+)