[PATCH] drm: document DRM_MODE_PAGE_FLIP_EVENT interactions with atomic

[PATCH] drm: document DRM_MODE_PAGE_FLIP_EVENT interactions with atomic

[Simon Ser]

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).
  */
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 /**
-- 
2.47.1

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

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

[Simon Ser]

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.

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

[Ville Syrjälä]

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

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

[Simon Ser]

On Friday, January 17th, 2025 at 12:15, Pekka Paalanen <pekka.paalanen@haloniitty.fi> wrote:

> 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?

Oops, sounds like I stopped here to write the next paragraph and forgot
to go back and finish the sentence…

> > 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.

Yeah, I suppose so.

> 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?

DRM_CAP_CRTC_IN_VBLANK_EVENT is unconditionally set to 1. It doesn't seem
like there is any interaction between these two. So yeah, I suppose
user-space needs to count if they are on kernel < v4.12.

> Nevertheless, should there be a "see also DRM_CAP_CRTC_IN_VBLANK_EVENT"?

This sounds a bit out-of-place to me TBH. It's orthogonal to event delivery
and it's linked from struct drm_event_vblank already.

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

[Daniel Stone]

On Mon, 14 Apr 2025 at 09:24, Simon Ser <contact@emersion.fr> wrote:
> On Friday, January 17th, 2025 at 12:15, Pekka Paalanen <pekka.paalanen@haloniitty.fi> wrote:
> > 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?
>
> DRM_CAP_CRTC_IN_VBLANK_EVENT is unconditionally set to 1. It doesn't seem
> like there is any interaction between these two. So yeah, I suppose
> user-space needs to count if they are on kernel < v4.12.
>
> > Nevertheless, should there be a "see also DRM_CAP_CRTC_IN_VBLANK_EVENT"?
>
> This sounds a bit out-of-place to me TBH. It's orthogonal to event delivery
> and it's linked from struct drm_event_vblank already.

Right. One completion event will be delivered per CRTC in any case.
It's just that *cough* someone forgot to include the CRTC ID in the
event in the version of atomic support that first landed, so you get n
events which each refer to _a_ CRTC, but userspace has no idea which.

I don't think we need to refer too much to the caps. Realistically, I
suspect everyone who ever tried to use a kernel which didn't have the
CRTC ID in the completion event with atomic userspace is in this
thread, apart from Maarten.

Cheers,
Daniel