[PATCH] drm: document uAPI page-flip flags

[PATCH] drm: document uAPI page-flip flags

[Simon Ser]

Document flags accepted by the page-flip and atomic IOCTLs.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 include/uapi/drm/drm_mode.h | 44 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fa953309d9ce..e1b04ffd54c3 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -935,12 +935,30 @@ struct hdr_output_metadata {
 	};
 };
 
+/**
+ * DRM_MODE_PAGE_FLIP_EVENT
+ *
+ * Request that the kernel sends back a vblank event (see
+ * struct drm_event_vblank) when the page-flip is done.
+ */
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
+/**
+ * DRM_MODE_PAGE_FLIP_ASYNC
+ *
+ * Request that the page-flip is performed as soon as possible, ie. with no
+ * delay due to waiting for vblank. This may cause tearing to be visible on
+ * the screen.
+ */
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
 #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
 #define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
 #define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
 				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
+/**
+ * DRM_MODE_PAGE_FLIP_FLAGS
+ *
+ * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.
+ */
 #define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
 				  DRM_MODE_PAGE_FLIP_ASYNC | \
 				  DRM_MODE_PAGE_FLIP_TARGET)
@@ -1034,11 +1052,35 @@ struct drm_mode_destroy_dumb {
 	__u32 handle;
 };
 
-/* page-flip flags are valid, plus: */
+/**
+ * DRM_MODE_ATOMIC_TEST_ONLY
+ *
+ * Do not apply the atomic commit, instead check whether the hardware supports
+ * this configuration.
+ *
+ * See drm_mode_config_funcs.atomic_check for more details on test-only
+ * commits.
+ */
 #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
+/**
+ * DRM_MODE_ATOMIC_NONBLOCK
+ *
+ * Do not block while applying the atomic commit.
+ */
 #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
+/**
+ * DRM_MODE_ATOMIC_ALLOW_MODESET
+ *
+ * Allow the update to result in visible artifacts such as a black screen.
+ */
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
 
+/**
+ * DRM_MODE_ATOMIC_FLAGS
+ *
+ * Bitfield of flags accepted by the &DRM_IOCTL_MODE_ATOMIC IOCTL in
+ * &drm_mode_atomic.flags.
+ */
 #define DRM_MODE_ATOMIC_FLAGS (\
 		DRM_MODE_PAGE_FLIP_EVENT |\
 		DRM_MODE_PAGE_FLIP_ASYNC |\
-- 
2.37.2

Re: drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Wed, 24 Aug 2022 17:45:06 +0000
Simon Ser <contact@emersion.fr> wrote:

> Document flags accepted by the page-flip and atomic IOCTLs.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  include/uapi/drm/drm_mode.h | 44 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index fa953309d9ce..e1b04ffd54c3 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -935,12 +935,30 @@ struct hdr_output_metadata {
>  	};
>  };
>  
> +/**
> + * DRM_MODE_PAGE_FLIP_EVENT
> + *
> + * Request that the kernel sends back a vblank event (see
> + * struct drm_event_vblank) when the page-flip is done.

...with type = DRM_EVENT_FLIP_COMPLETE?

This was a bit new to me, because libdrm abstracts vblank and pageflip
events into different APIs.

> + */
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> +/**
> + * DRM_MODE_PAGE_FLIP_ASYNC
> + *
> + * Request that the page-flip is performed as soon as possible, ie. with no
> + * delay due to waiting for vblank. This may cause tearing to be visible on
> + * the screen.

Btw. does the kernel fail the flip if the driver does not support async?
Or does it silently fall back to sync flip?
Asking for both legacy and atomic APIs.

> + */
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>  #define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
>  #define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
>  				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> +/**
> + * DRM_MODE_PAGE_FLIP_FLAGS
> + *
> + * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.
> + */
>  #define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
>  				  DRM_MODE_PAGE_FLIP_ASYNC | \
>  				  DRM_MODE_PAGE_FLIP_TARGET)
> @@ -1034,11 +1052,35 @@ struct drm_mode_destroy_dumb {
>  	__u32 handle;
>  };
>  
> -/* page-flip flags are valid, plus: */
> +/**
> + * DRM_MODE_ATOMIC_TEST_ONLY
> + *
> + * Do not apply the atomic commit, instead check whether the hardware supports
> + * this configuration.
> + *
> + * See drm_mode_config_funcs.atomic_check for more details on test-only
> + * commits.
> + */
>  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> +/**
> + * DRM_MODE_ATOMIC_NONBLOCK
> + *
> + * Do not block while applying the atomic commit.

Maybe add something like:

	The atomic commit ioctl returns immediately instead of waiting
	for the changes to be applied in hardware.

> + */
>  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> +/**
> + * DRM_MODE_ATOMIC_ALLOW_MODESET
> + *
> + * Allow the update to result in visible artifacts such as a black screen.

Maybe add:

	...temporary or transient visible artifacts while the update is
	being applied. Applying the update may also take significantly
	more time than a page flip. The visual artifacts will not
	appear after the update is completed.

	This flag must be set when the KMS update might cause visible
	artifacts. Without this flag such KMS update will return EINVAL
	error. What kind of updates may cause visible artifacts depends
	on the driver and the hardware. Userspace that needs to know
	beforehand if an update might cause visible artifacts can use
	DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
	to see if it fails.

	Visual artifacts are guaranteed to not appear when this flag is
	not set.

That "artifacts will not appear after the update is completed" is a bit
awkward, because when this commit completes and triggers the completion
event (if requested), the visual artifacts might still be on screen, but
as soon as the scanout cycle that just started finishes, all artifacts
are gone for good. Isn't that how it works?

Or does the kernel wait with the completion event until a good picture
has been fully scanned out at least once? I'd expect not.

> + */
>  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
>  
> +/**
> + * DRM_MODE_ATOMIC_FLAGS
> + *
> + * Bitfield of flags accepted by the &DRM_IOCTL_MODE_ATOMIC IOCTL in
> + * &drm_mode_atomic.flags.
> + */
>  #define DRM_MODE_ATOMIC_FLAGS (\
>  		DRM_MODE_PAGE_FLIP_EVENT |\
>  		DRM_MODE_PAGE_FLIP_ASYNC |\


Thanks,
pq

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

Re: drm: document uAPI page-flip flags

[Sebastian Wick]

On Fri, Aug 26, 2022 at 10:58 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Wed, 24 Aug 2022 17:45:06 +0000
> Simon Ser <contact@emersion.fr> wrote:
>
> > Document flags accepted by the page-flip and atomic IOCTLs.
> >
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  include/uapi/drm/drm_mode.h | 44 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index fa953309d9ce..e1b04ffd54c3 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -935,12 +935,30 @@ struct hdr_output_metadata {
> >       };
> >  };
> >
> > +/**
> > + * DRM_MODE_PAGE_FLIP_EVENT
> > + *
> > + * Request that the kernel sends back a vblank event (see
> > + * struct drm_event_vblank) when the page-flip is done.
>
> ...with type = DRM_EVENT_FLIP_COMPLETE?
>
> This was a bit new to me, because libdrm abstracts vblank and pageflip
> events into different APIs.
>
> > + */
> >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > +/**
> > + * DRM_MODE_PAGE_FLIP_ASYNC
> > + *
> > + * Request that the page-flip is performed as soon as possible, ie. with no
> > + * delay due to waiting for vblank. This may cause tearing to be visible on
> > + * the screen.
>
> Btw. does the kernel fail the flip if the driver does not support async?
> Or does it silently fall back to sync flip?
> Asking for both legacy and atomic APIs.
>
> > + */
> >  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> >  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> >  #define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
> >  #define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
> >                                  DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> > +/**
> > + * DRM_MODE_PAGE_FLIP_FLAGS
> > + *
> > + * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.
> > + */
> >  #define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
> >                                 DRM_MODE_PAGE_FLIP_ASYNC | \
> >                                 DRM_MODE_PAGE_FLIP_TARGET)
> > @@ -1034,11 +1052,35 @@ struct drm_mode_destroy_dumb {
> >       __u32 handle;
> >  };
> >
> > -/* page-flip flags are valid, plus: */
> > +/**
> > + * DRM_MODE_ATOMIC_TEST_ONLY
> > + *
> > + * Do not apply the atomic commit, instead check whether the hardware supports
> > + * this configuration.
> > + *
> > + * See drm_mode_config_funcs.atomic_check for more details on test-only
> > + * commits.
> > + */
> >  #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> > +/**
> > + * DRM_MODE_ATOMIC_NONBLOCK
> > + *
> > + * Do not block while applying the atomic commit.
>
> Maybe add something like:
>
>         The atomic commit ioctl returns immediately instead of waiting
>         for the changes to be applied in hardware.
>
> > + */
> >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> > +/**
> > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > + *
> > + * Allow the update to result in visible artifacts such as a black screen.
>
> Maybe add:
>
>         ...temporary or transient visible artifacts while the update is
>         being applied. Applying the update may also take significantly
>         more time than a page flip. The visual artifacts will not
>         appear after the update is completed.
>
>         This flag must be set when the KMS update might cause visible
>         artifacts. Without this flag such KMS update will return EINVAL
>         error. What kind of updates may cause visible artifacts depends
>         on the driver and the hardware. Userspace that needs to know
>         beforehand if an update might cause visible artifacts can use
>         DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
>         to see if it fails.
>
>         Visual artifacts are guaranteed to not appear when this flag is
>         not set.

That doesn't seem to be true, though. For example setting
HDR_OUTPUT_METADATA for example does result in visual artifacts on my
display no matter if the flag is specified or not because the
artifacts are not the result of a mode set but of the display reacting
to some AVI InfoFrame.

>
> That "artifacts will not appear after the update is completed" is a bit
> awkward, because when this commit completes and triggers the completion
> event (if requested), the visual artifacts might still be on screen, but
> as soon as the scanout cycle that just started finishes, all artifacts
> are gone for good. Isn't that how it works?
>
> Or does the kernel wait with the completion event until a good picture
> has been fully scanned out at least once? I'd expect not.
>
> > + */
> >  #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> >
> > +/**
> > + * DRM_MODE_ATOMIC_FLAGS
> > + *
> > + * Bitfield of flags accepted by the &DRM_IOCTL_MODE_ATOMIC IOCTL in
> > + * &drm_mode_atomic.flags.
> > + */
> >  #define DRM_MODE_ATOMIC_FLAGS (\
> >               DRM_MODE_PAGE_FLIP_EVENT |\
> >               DRM_MODE_PAGE_FLIP_ASYNC |\
>
>
> Thanks,
> pq

Re: drm: document uAPI page-flip flags

[Simon Ser]

On Friday, August 26th, 2022 at 11:49, Sebastian Wick <sebastian.wick@redhat.com> wrote:

> > > +/*
> > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > + *
> > > + * Allow the update to result in visible artifacts such as a black screen.
> > 
> > Maybe add:
> > 
> > ...temporary or transient visible artifacts while the update is
> > being applied. Applying the update may also take significantly
> > more time than a page flip. The visual artifacts will not
> > appear after the update is completed.
> > 
> > This flag must be set when the KMS update might cause visible
> > artifacts. Without this flag such KMS update will return EINVAL
> > error. What kind of updates may cause visible artifacts depends
> > on the driver and the hardware. Userspace that needs to know
> > beforehand if an update might cause visible artifacts can use
> > DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> > to see if it fails.
> > 
> > Visual artifacts are guaranteed to not appear when this flag is
> > not set.
> 
> That doesn't seem to be true, though. For example setting
> HDR_OUTPUT_METADATA for example does result in visual artifacts on my
> display no matter if the flag is specified or not because the
> artifacts are not the result of a mode set but of the display reacting
> to some AVI InfoFrame.

One would need to read the HDMI spec to see if there's anything in
there about artifacts on AVI InfoFrame change, then figure out whether
this is a bug in the physical screen itself or whether the kernel
driver should require ALLOW_MODESET when updating the HDR metadata.

Re: drm: document uAPI page-flip flags

[Sebastian Wick]

On Fri, Aug 26, 2022 at 2:17 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Friday, August 26th, 2022 at 11:49, Sebastian Wick <sebastian.wick@redhat.com> wrote:
>
> > > > +/*
> > > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > > + *
> > > > + * Allow the update to result in visible artifacts such as a black screen.
> > >
> > > Maybe add:
> > >
> > > ...temporary or transient visible artifacts while the update is
> > > being applied. Applying the update may also take significantly
> > > more time than a page flip. The visual artifacts will not
> > > appear after the update is completed.
> > >
> > > This flag must be set when the KMS update might cause visible
> > > artifacts. Without this flag such KMS update will return EINVAL
> > > error. What kind of updates may cause visible artifacts depends
> > > on the driver and the hardware. Userspace that needs to know
> > > beforehand if an update might cause visible artifacts can use
> > > DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> > > to see if it fails.
> > >
> > > Visual artifacts are guaranteed to not appear when this flag is
> > > not set.
> >
> > That doesn't seem to be true, though. For example setting
> > HDR_OUTPUT_METADATA for example does result in visual artifacts on my
> > display no matter if the flag is specified or not because the
> > artifacts are not the result of a mode set but of the display reacting
> > to some AVI InfoFrame.
>
> One would need to read the HDMI spec to see if there's anything in
> there about artifacts on AVI InfoFrame change, then figure out whether
> this is a bug in the physical screen itself or whether the kernel
> driver should require ALLOW_MODESET when updating the HDR metadata.

I'm not even sure if that's the right thing to do. ALLOW_MODESET isn't
really about if a commit can lead to visual artifacts but about
keeping the existing links alive (someone with more knowledge on this
subject probably has a better description for this). An async commit
can also introduce visual artifacts for example and there are probably
more cases.

>

Re: drm: document uAPI page-flip flags

[Simon Ser]

On Friday, August 26th, 2022 at 15:10, Sebastian Wick <sebastian.wick@redhat.com> wrote:

> On Fri, Aug 26, 2022 at 2:17 PM Simon Ser contact@emersion.fr wrote:
> 
> > On Friday, August 26th, 2022 at 11:49, Sebastian Wick sebastian.wick@redhat.com wrote:
> > 
> > > > > +/*
> > > > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > > > + *
> > > > > + * Allow the update to result in visible artifacts such as a black screen.
> > > > 
> > > > Maybe add:
> > > > 
> > > > ...temporary or transient visible artifacts while the update is
> > > > being applied. Applying the update may also take significantly
> > > > more time than a page flip. The visual artifacts will not
> > > > appear after the update is completed.
> > > > 
> > > > This flag must be set when the KMS update might cause visible
> > > > artifacts. Without this flag such KMS update will return EINVAL
> > > > error. What kind of updates may cause visible artifacts depends
> > > > on the driver and the hardware. Userspace that needs to know
> > > > beforehand if an update might cause visible artifacts can use
> > > > DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> > > > to see if it fails.
> > > > 
> > > > Visual artifacts are guaranteed to not appear when this flag is
> > > > not set.
> > > 
> > > That doesn't seem to be true, though. For example setting
> > > HDR_OUTPUT_METADATA for example does result in visual artifacts on my
> > > display no matter if the flag is specified or not because the
> > > artifacts are not the result of a mode set but of the display reacting
> > > to some AVI InfoFrame.
> > 
> > One would need to read the HDMI spec to see if there's anything in
> > there about artifacts on AVI InfoFrame change, then figure out whether
> > this is a bug in the physical screen itself or whether the kernel
> > driver should require ALLOW_MODESET when updating the HDR metadata.
> 
> I'm not even sure if that's the right thing to do. ALLOW_MODESET isn't
> really about if a commit can lead to visual artifacts but about
> keeping the existing links alive (someone with more knowledge on this
> subject probably has a better description for this). An async commit
> can also introduce visual artifacts for example and there are probably
> more cases.

That's certainly not the intent of ALLOW_MODESET.

See the kernel docs for atomic_check:

> This callback also needs to correctly fill out the drm_crtc_state in
> this update to make sure that drm_atomic_crtc_needs_modeset() reflects
> the nature of the possible update and returns true if and only if the
> update cannot be applied without tearing within one vblank on that
> CRTC. The core uses that information to reject updates which require
> a full modeset (i.e. blanking the screen, or at least pausing updates
> for a substantial amount of time) if userspace has disallowed that in
> its request.

I'm sure Daniel Vetter can confirm this as well.

Re: drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Fri, 26 Aug 2022 13:17:09 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Friday, August 26th, 2022 at 15:10, Sebastian Wick <sebastian.wick@redhat.com> wrote:
> 
> > On Fri, Aug 26, 2022 at 2:17 PM Simon Ser contact@emersion.fr wrote:
> >   
> > > On Friday, August 26th, 2022 at 11:49, Sebastian Wick sebastian.wick@redhat.com wrote:
> > >   
> > > > > > +/*
> > > > > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > > > > + *
> > > > > > + * Allow the update to result in visible artifacts such as a black screen.  
> > > > > 
> > > > > Maybe add:
> > > > > 
> > > > > ...temporary or transient visible artifacts while the update is
> > > > > being applied. Applying the update may also take significantly
> > > > > more time than a page flip. The visual artifacts will not
> > > > > appear after the update is completed.
> > > > > 
> > > > > This flag must be set when the KMS update might cause visible
> > > > > artifacts. Without this flag such KMS update will return EINVAL
> > > > > error. What kind of updates may cause visible artifacts depends
> > > > > on the driver and the hardware. Userspace that needs to know
> > > > > beforehand if an update might cause visible artifacts can use
> > > > > DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> > > > > to see if it fails.
> > > > > 
> > > > > Visual artifacts are guaranteed to not appear when this flag is
> > > > > not set.  
> > > > 
> > > > That doesn't seem to be true, though. For example setting
> > > > HDR_OUTPUT_METADATA for example does result in visual artifacts on my
> > > > display no matter if the flag is specified or not because the
> > > > artifacts are not the result of a mode set but of the display reacting
> > > > to some AVI InfoFrame.  
> > > 
> > > One would need to read the HDMI spec to see if there's anything in
> > > there about artifacts on AVI InfoFrame change, then figure out whether
> > > this is a bug in the physical screen itself or whether the kernel
> > > driver should require ALLOW_MODESET when updating the HDR metadata.  
> > 
> > I'm not even sure if that's the right thing to do. ALLOW_MODESET isn't
> > really about if a commit can lead to visual artifacts but about
> > keeping the existing links alive (someone with more knowledge on this
> > subject probably has a better description for this). An async commit
> > can also introduce visual artifacts for example and there are probably
> > more cases.  
> 
> That's certainly not the intent of ALLOW_MODESET.

One could look at this from the perspective of who is allowed to break.
Maybe the monitor is allowed to break any time (because it is out of
our reach anyway, we can only control the electrical signals going into
the cable), but the source hardware (the display block, everything
before "the cable") is allowed to break only with ALLOW_MODESET.

IOW, if the electrical signal is kept "glitch free" in the sense that
the sink (or a human looking at a sink) cannot see artifacts, then
ALLOW_MODESET is not required. One could even change the video mode
without ALLOW_MODESET if it does not result in artifacts, like
switching from one constant refresh rate to another on VRR capable
hardware - or what I heard Intel does with some transparent power
saving by halving the refresh rate when nothing is changing.

So, at the minimum we must require that the electrical signal cannot
"break". Do we include what a monitor does in ALLOW_MODESET or not is
something I'm not sure, since different sinks presumably break on
different changes.

> See the kernel docs for atomic_check:
> 
> > This callback also needs to correctly fill out the drm_crtc_state in
> > this update to make sure that drm_atomic_crtc_needs_modeset() reflects
> > the nature of the possible update and returns true if and only if the
> > update cannot be applied without tearing within one vblank on that
> > CRTC. The core uses that information to reject updates which require
> > a full modeset (i.e. blanking the screen, or at least pausing updates
> > for a substantial amount of time) if userspace has disallowed that in
> > its request.  
> 
> I'm sure Daniel Vetter can confirm this as well.

I guess the new ASYNC flag patches will fix this wording to allow
tearing in that case?


Thanks,
pq

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

Re: drm: document uAPI page-flip flags

[Simon Ser]

CC Ville for the ASYNC bits, see below.

On Friday, August 26th, 2022 at 10:53, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> > +/**
> > + * DRM_MODE_PAGE_FLIP_EVENT
> > + *
> > + * Request that the kernel sends back a vblank event (see
> > + * struct drm_event_vblank) when the page-flip is done.
> 
> ...with type = DRM_EVENT_FLIP_COMPLETE?
> 
> This was a bit new to me, because libdrm abstracts vblank and pageflip
> events into different APIs.

Indeed.

Maybe should clarify what "done" means here? Would "when the page-flip has been
displayed on-screen" be correct?

> > + */
> >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > +/**
> > + * DRM_MODE_PAGE_FLIP_ASYNC
> > + *
> > + * Request that the page-flip is performed as soon as possible, ie. with no
> > + * delay due to waiting for vblank. This may cause tearing to be visible on
> > + * the screen.
> 
> Btw. does the kernel fail the flip if the driver does not support async?
> Or does it silently fall back to sync flip?
> Asking for both legacy and atomic APIs.

Atomic doesn't support this yet, so it's pretty much TBD (see Ville's reply [1]).

For legacy, it seems like drivers do what they want. AFAIU, i915 will reject
(see intel_async_flip_check_uapi etc), and amdgpu will silently fall back to
vsync (see the `acrtc_state->update_type == UPDATE_TYPE_FAST` check in
amdgpu_dm_commit_planes).

Maybe one of the drivers is wrong here and should be fixed?

[1]: https://lore.kernel.org/dri-devel/YwiB%2FxQf6Z6ScU+Z@intel.com/

> > +/**
> > + * DRM_MODE_ATOMIC_NONBLOCK
> > + *
> > + * Do not block while applying the atomic commit.
> 
> Maybe add something like:
> 
> 	The atomic commit ioctl returns immediately instead of waiting
> 	for the changes to be applied in hardware.

Good idea. Also added:

    Note, the driver will still check that the update can be applied before
    retuning.

> > + */
> >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> > +/**
> > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > + *
> > + * Allow the update to result in visible artifacts such as a black screen.
> 
> Maybe add:
> 
> 	...temporary or transient visible artifacts while the update is
> 	being applied. Applying the update may also take significantly
> 	more time than a page flip. The visual artifacts will not
> 	appear after the update is completed.
> 
> 	This flag must be set when the KMS update might cause visible
> 	artifacts. Without this flag such KMS update will return EINVAL
> 	error. What kind of updates may cause visible artifacts depends
> 	on the driver and the hardware. Userspace that needs to know
> 	beforehand if an update might cause visible artifacts can use
> 	DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> 	to see if it fails.
> 
> 	Visual artifacts are guaranteed to not appear when this flag is
> 	not set.
> 
> That "artifacts will not appear after the update is completed" is a bit
> awkward, because when this commit completes and triggers the completion
> event (if requested), the visual artifacts might still be on screen, but
> as soon as the scanout cycle that just started finishes, all artifacts
> are gone for good. Isn't that how it works?
> 
> Or does the kernel wait with the completion event until a good picture
> has been fully scanned out at least once? I'd expect not.

That generally looks like a good description to me, and AFAIK is how things
work indeed.

Re: drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Mon, 29 Aug 2022 15:37:52 +0000
Simon Ser <contact@emersion.fr> wrote:

> CC Ville for the ASYNC bits, see below.
> 
> On Friday, August 26th, 2022 at 10:53, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > > +/**
> > > + * DRM_MODE_PAGE_FLIP_EVENT
> > > + *
> > > + * Request that the kernel sends back a vblank event (see
> > > + * struct drm_event_vblank) when the page-flip is done.  
> > 
> > ...with type = DRM_EVENT_FLIP_COMPLETE?
> > 
> > This was a bit new to me, because libdrm abstracts vblank and pageflip
> > events into different APIs.  
> 
> Indeed.
> 
> Maybe should clarify what "done" means here? Would "when the page-flip has been
> displayed on-screen" be correct?

Good idea, but definition is not quite that AFAIU. I would understand
"displayed" as "turned into light" or at least fully sent to the cable,
when we are talking at this level of detail.

Hence, "has been displayed" is not it because the flip-done event is
emitted before the new FB contents have been scanned out. That scanout
cycle is only starting when the flip is done. The flip timestamp should
correspond to the time when the first real pixel of the new FB hits the
monitor cable.

A flip is done, when it is guaranteed that the next (or on-going, in
case of tearing) scanout cycle will use the new FB, IOW the hardware
programming has been done I believe.

If the flip is sync'd to vblank, the timestamp is as above, but the
actual event might be emitted somewhat before or after the instant of
the timestamp. Some drivers can predict the timestamp so can send the
event early, others don't.

If the flip is tearing, then I'm not sure what the timestamp is or when
the event is emitted.

> > > + */
> > >  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > > +/**
> > > + * DRM_MODE_PAGE_FLIP_ASYNC
> > > + *
> > > + * Request that the page-flip is performed as soon as possible, ie. with no
> > > + * delay due to waiting for vblank. This may cause tearing to be visible on
> > > + * the screen.  
> > 
> > Btw. does the kernel fail the flip if the driver does not support async?
> > Or does it silently fall back to sync flip?
> > Asking for both legacy and atomic APIs.  
> 
> Atomic doesn't support this yet, so it's pretty much TBD (see Ville's reply [1]).

So atomic commit ioctl will fail with EINVAL because userspace is
giving it an unrecognized flag?

[1] is interesting. Apparently the atomic async flag will only be a
hint, "make it tear if possible". That seems fine to me.

> 
> For legacy, it seems like drivers do what they want. AFAIU, i915 will reject
> (see intel_async_flip_check_uapi etc), and amdgpu will silently fall back to
> vsync (see the `acrtc_state->update_type == UPDATE_TYPE_FAST` check in
> amdgpu_dm_commit_planes).
> 
> Maybe one of the drivers is wrong here and should be fixed?
> 
> [1]: https://lore.kernel.org/dri-devel/YwiB%2FxQf6Z6ScU+Z@intel.com/
> 
> > > +/**
> > > + * DRM_MODE_ATOMIC_NONBLOCK
> > > + *
> > > + * Do not block while applying the atomic commit.  
> > 
> > Maybe add something like:
> > 
> > 	The atomic commit ioctl returns immediately instead of waiting
> > 	for the changes to be applied in hardware.  
> 
> Good idea. Also added:
> 
>     Note, the driver will still check that the update can be applied before
>     retuning.

Nice.

> > > + */
> > >  #define DRM_MODE_ATOMIC_NONBLOCK  0x0200
> > > +/**
> > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > + *
> > > + * Allow the update to result in visible artifacts such as a black screen.  
> > 
> > Maybe add:
> > 
> > 	...temporary or transient visible artifacts while the update is
> > 	being applied. Applying the update may also take significantly
> > 	more time than a page flip. The visual artifacts will not
> > 	appear after the update is completed.
> > 
> > 	This flag must be set when the KMS update might cause visible
> > 	artifacts. Without this flag such KMS update will return EINVAL
> > 	error. What kind of updates may cause visible artifacts depends
> > 	on the driver and the hardware. Userspace that needs to know
> > 	beforehand if an update might cause visible artifacts can use
> > 	DRM_MODE_ATOMIC_TEST_ONLY without DRM_MODE_ATOMIC_ALLOW_MODESET
> > 	to see if it fails.
> > 
> > 	Visual artifacts are guaranteed to not appear when this flag is
> > 	not set.
> > 
> > That "artifacts will not appear after the update is completed" is a bit
> > awkward, because when this commit completes and triggers the completion
> > event (if requested), the visual artifacts might still be on screen, but
> > as soon as the scanout cycle that just started finishes, all artifacts
> > are gone for good. Isn't that how it works?
> > 
> > Or does the kernel wait with the completion event until a good picture
> > has been fully scanned out at least once? I'd expect not.  
> 
> That generally looks like a good description to me, and AFAIK is how things
> work indeed.

Yeah, but swick has a good point: should we be talking about literal
visual artifacts or do we only guarantee that the video signal on the
cable is uninterrupted?

HDR_OUTPUT_METADATA is the prime example where that matters. It is
infoframe data, which means changing it will never cause any
interruption in the video signal. However, changing certain fields (and
not with other fields) in the infoframe is likely to cause the monitor
to glitch or blank for a moment.


Thanks,
pq

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

Re: drm: document uAPI page-flip flags

[Michel Dänzer]

On 2022-08-30 10:16, Pekka Paalanen wrote:
> On Mon, 29 Aug 2022 15:37:52 +0000
> Simon Ser <contact@emersion.fr> wrote:
>> On Friday, August 26th, 2022 at 10:53, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>
>>>> +/**
>>>> + * DRM_MODE_PAGE_FLIP_ASYNC
>>>> + *
>>>> + * Request that the page-flip is performed as soon as possible, ie. with no
>>>> + * delay due to waiting for vblank. This may cause tearing to be visible on
>>>> + * the screen.  
>>>
>>> Btw. does the kernel fail the flip if the driver does not support async?
>>> Or does it silently fall back to sync flip?
>>> Asking for both legacy and atomic APIs.  
>>
>> Atomic doesn't support this yet, so it's pretty much TBD (see Ville's reply [1]).
> 
> So atomic commit ioctl will fail with EINVAL because userspace is
> giving it an unrecognized flag?
> 
> [1] is interesting. Apparently the atomic async flag will only be a
> hint, "make it tear if possible". That seems fine to me.

To me it would seem better for the driver to return an error if the async flag is passed in, but the driver can't actually do an async update.

Otherwise it's tricky for a Wayland compositor to decide if it should use an async commit (which needs to be done ASAP to serve the intended purpose) or not (in which case the compositor may want to delay the commit as long as possible for minimal latency).


>> For legacy, it seems like drivers do what they want. AFAIU, i915 will reject
>> (see intel_async_flip_check_uapi etc), and amdgpu will silently fall back to
>> vsync (see the `acrtc_state->update_type == UPDATE_TYPE_FAST` check in
>> amdgpu_dm_commit_planes).
>>
>> Maybe one of the drivers is wrong here and should be fixed?
>>
>> [1]: https://lore.kernel.org/dri-devel/YwiB%2FxQf6Z6ScU+Z@intel.com

-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: drm: document uAPI page-flip flags

[Simon Ser]

On Tuesday, August 30th, 2022 at 10:31, Michel Dänzer <michel.daenzer@mailbox.org> wrote:

> > For the atomic uAPI, we need to pick on of these two approaches:
> >
> > 1. Let the kernel fall back to a sync flip if async isn't possible. This
> >    simplifies user-space, but then user-space has no reliable way to figure out
> >    what really happened (sync or async?). That could be fixed with a new
> >    read-only CRTC prop indicating whether the last flip was async or not.
> >    However, maybe someone will come up in the future with user-space which
> >    needs to only apply an update if async flip is possible, in which case this
> >    approach falls short.
> 
> The future is now. :)
> 
> As I described in the documentation patch discussion, this approach would
> make it tricky for a Wayland compositor to decide if it should use an async
> commit (which needs to be done ASAP to serve the intended purpose) or not (in
> which case the compositor may want to delay the commit as long as possible
> for minimal latency).

Ah right. If an async page-flip is not possible, then a Wayland compositor
could want to wait the "last moment" before the next vblank to see if a more
up-to-date buffer is available.

With that + Xorg usage, I think we have a rather solid case for failing async
flips rather than silently falling back to vsync.

Re: drm: document uAPI page-flip flags

[Ville Syrjälä]

On Tue, Aug 30, 2022 at 11:16:26AM +0300, Pekka Paalanen wrote:
> On Mon, 29 Aug 2022 15:37:52 +0000
> Simon Ser <contact@emersion.fr> wrote:
> 
> > CC Ville for the ASYNC bits, see below.
> > 
> > On Friday, August 26th, 2022 at 10:53, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > 
> > > > +/**
> > > > + * DRM_MODE_PAGE_FLIP_EVENT
> > > > + *
> > > > + * Request that the kernel sends back a vblank event (see
> > > > + * struct drm_event_vblank) when the page-flip is done.  
> > > 
> > > ...with type = DRM_EVENT_FLIP_COMPLETE?
> > > 
> > > This was a bit new to me, because libdrm abstracts vblank and pageflip
> > > events into different APIs.  
> > 
> > Indeed.
> > 
> > Maybe should clarify what "done" means here? Would "when the page-flip has been
> > displayed on-screen" be correct?
> 
> Good idea, but definition is not quite that AFAIU. I would understand
> "displayed" as "turned into light" or at least fully sent to the cable,
> when we are talking at this level of detail.
> 
> Hence, "has been displayed" is not it because the flip-done event is
> emitted before the new FB contents have been scanned out. That scanout
> cycle is only starting when the flip is done. The flip timestamp should
> correspond to the time when the first real pixel of the new FB hits the
> monitor cable.
> 
> A flip is done, when it is guaranteed that the next (or on-going, in
> case of tearing) scanout cycle will use the new FB, IOW the hardware
> programming has been done I believe.
> 
> If the flip is sync'd to vblank, the timestamp is as above, but the
> actual event might be emitted somewhat before or after the instant of
> the timestamp. Some drivers can predict the timestamp so can send the
> event early, others don't.
> 
> If the flip is tearing, then I'm not sure what the timestamp is or when
> the event is emitted.

For i915 we emit the event when the hardware has indicated the
flip has completed (ie. it has really started scanout from the
new fb). After that you can safely reuse the old fb without
accidentally doing frontbuffer rendering. It takes a bit of
time (some smallish number of scanlines) for the hardware to
flush the FIFOs/TLBs/whatever.

And IIRC we just send the last sampled vblank timestamp for
the event. That is, the timestamp should look the same as
for a sync flip submitted during the previous frame. I was
thinking of making it more or less show the current time
of the flip done indication as that's when the scanout
from the new fb starts, but pretty much everyone else was
of the opinion that there is no point in doing that.

-- 
Ville Syrjälä
Intel

Re: drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Tue, 30 Aug 2022 11:41:39 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Tue, Aug 30, 2022 at 11:16:26AM +0300, Pekka Paalanen wrote:

> > Hence, "has been displayed" is not it because the flip-done event is
> > emitted before the new FB contents have been scanned out. That scanout
> > cycle is only starting when the flip is done. The flip timestamp should
> > correspond to the time when the first real pixel of the new FB hits the
> > monitor cable.
> > 
> > A flip is done, when it is guaranteed that the next (or on-going, in
> > case of tearing) scanout cycle will use the new FB, IOW the hardware
> > programming has been done I believe.
> > 
> > If the flip is sync'd to vblank, the timestamp is as above, but the
> > actual event might be emitted somewhat before or after the instant of
> > the timestamp. Some drivers can predict the timestamp so can send the
> > event early, others don't.
> > 
> > If the flip is tearing, then I'm not sure what the timestamp is or when
> > the event is emitted.  
> 
> For i915 we emit the event when the hardware has indicated the
> flip has completed (ie. it has really started scanout from the
> new fb). After that you can safely reuse the old fb without
> accidentally doing frontbuffer rendering. It takes a bit of
> time (some smallish number of scanlines) for the hardware to
> flush the FIFOs/TLBs/whatever.
> 
> And IIRC we just send the last sampled vblank timestamp for
> the event. That is, the timestamp should look the same as
> for a sync flip submitted during the previous frame. I was
> thinking of making it more or less show the current time
> of the flip done indication as that's when the scanout
> from the new fb starts, but pretty much everyone else was
> of the opinion that there is no point in doing that.

Ugh. So we have two different meanings for the completion timestamp:

sync flip: when the first pixel from the new FB is emitted into the
	video signal

async flip: something irrelevant

*sigh*

To me it would make sense for both to use the same definition, if
that's technically possible. The point is to know roughly where the
tear line is which could then answer the question "did it actually
tear" which is related to the silent falling back to sync flips.


Thanks,
pq

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

Re: drm: document uAPI page-flip flags

[Ville Syrjälä]

On Tue, Aug 30, 2022 at 12:00:50PM +0300, Pekka Paalanen wrote:
> On Tue, 30 Aug 2022 11:41:39 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Tue, Aug 30, 2022 at 11:16:26AM +0300, Pekka Paalanen wrote:
> 
> > > Hence, "has been displayed" is not it because the flip-done event is
> > > emitted before the new FB contents have been scanned out. That scanout
> > > cycle is only starting when the flip is done. The flip timestamp should
> > > correspond to the time when the first real pixel of the new FB hits the
> > > monitor cable.
> > > 
> > > A flip is done, when it is guaranteed that the next (or on-going, in
> > > case of tearing) scanout cycle will use the new FB, IOW the hardware
> > > programming has been done I believe.
> > > 
> > > If the flip is sync'd to vblank, the timestamp is as above, but the
> > > actual event might be emitted somewhat before or after the instant of
> > > the timestamp. Some drivers can predict the timestamp so can send the
> > > event early, others don't.
> > > 
> > > If the flip is tearing, then I'm not sure what the timestamp is or when
> > > the event is emitted.  
> > 
> > For i915 we emit the event when the hardware has indicated the
> > flip has completed (ie. it has really started scanout from the
> > new fb). After that you can safely reuse the old fb without
> > accidentally doing frontbuffer rendering. It takes a bit of
> > time (some smallish number of scanlines) for the hardware to
> > flush the FIFOs/TLBs/whatever.
> > 
> > And IIRC we just send the last sampled vblank timestamp for
> > the event. That is, the timestamp should look the same as
> > for a sync flip submitted during the previous frame. I was
> > thinking of making it more or less show the current time
> > of the flip done indication as that's when the scanout
> > from the new fb starts, but pretty much everyone else was
> > of the opinion that there is no point in doing that.
> 
> Ugh. So we have two different meanings for the completion timestamp:
> 
> sync flip: when the first pixel from the new FB is emitted into the
> 	video signal
> 
> async flip: something irrelevant
> 
> *sigh*
> 
> To me it would make sense for both to use the same definition, if
> that's technically possible. The point is to know roughly where the
> tear line is which could then answer the question "did it actually
> tear" which is related to the silent falling back to sync flips.

Yeah, that was pretty much my original thinking as well. Just
no one else was interested in it so I figured it's better to
not deviate from the common behaviour.

At least for i915 it shouldn't be too hard to do it properly.
The biggest complication would be untangling the drm_vblank
midlayer somehow.

-- 
Ville Syrjälä
Intel