[PATCH v3] drm: document uAPI page-flip flags

[PATCH v3] drm: document uAPI page-flip flags

[Simon Ser]

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

v2 (Pekka):
- Mention DRM_EVENT_FLIP_COMPLETE in DRM_MODE_PAGE_FLIP_EVENT docs.
- Expand DRM_MODE_ATOMIC_NONBLOCK and DRM_MODE_ATOMIC_ALLOW_MODESET
  description.
v3:
- Fix struct field ref syntax (Daniel)
- Clarify when artifacts are no longer displayed (Daniel)
- Add note about sinks deciding to show artifacts on their own (Pekka, Daniel)

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
---
 include/uapi/drm/drm_mode.h | 63 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index fa953309d9ce..9b10327b9d21 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -935,12 +935,31 @@ struct hdr_output_metadata {
 	};
 };
 
+/**
+ * DRM_MODE_PAGE_FLIP_EVENT
+ *
+ * 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.
+ */
 #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 +1053,53 @@ 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. The &DRM_IOCTL_MODE_ATOMIC
+ * IOCTL returns immediately instead of waiting for the changes to be applied
+ * in hardware. 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 temporary or transient visible artifacts while
+ * the update is being applied. Applying the update may also take significantly
+ * more time than a page flip. All visual artifacts will disappear by the time
+ * the update is completed, as signalled throught the vblank event's timestamp
+ * (see struct drm_event_vblank).
+ *
+ * This flag must be set when the KMS update might cause visible artifacts.
+ * Without this flag such KMS update will return a EINVAL error. What kind of
+ * update may cause visible artifacts depends on the driver and the hardware.
+ * User-space 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.
+ *
+ * To the best of the driver's knowledge, visual artifacts are guaranteed to
+ * not appear when this flag is not set. Some sinks might display visual
+ * artifacts outside of the driver's control.
+ */
 #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.3

Re: [PATCH v3] drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Wed, 28 Sep 2022 09:41:57 +0000
Simon Ser <contact@emersion.fr> wrote:

> Document flags accepted by the page-flip and atomic IOCTLs.
> 
> v2 (Pekka):
> - Mention DRM_EVENT_FLIP_COMPLETE in DRM_MODE_PAGE_FLIP_EVENT docs.
> - Expand DRM_MODE_ATOMIC_NONBLOCK and DRM_MODE_ATOMIC_ALLOW_MODESET
>   description.
> v3:
> - Fix struct field ref syntax (Daniel)
> - Clarify when artifacts are no longer displayed (Daniel)
> - Add note about sinks deciding to show artifacts on their own (Pekka, Daniel)
> 

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>

Only nitpicks below, you can ignore them.

> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 63 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index fa953309d9ce..9b10327b9d21 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -935,12 +935,31 @@ struct hdr_output_metadata {
>  	};
>  };
>  
> +/**
> + * DRM_MODE_PAGE_FLIP_EVENT
> + *
> + * 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.
> + */
>  #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.

Should this mention also drm_mode_crtc_page_flip.flags?

UAPI header defines both structs.

> + */
>  #define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
>  				  DRM_MODE_PAGE_FLIP_ASYNC | \
>  				  DRM_MODE_PAGE_FLIP_TARGET)
> @@ -1034,11 +1053,53 @@ 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. The &DRM_IOCTL_MODE_ATOMIC
> + * IOCTL returns immediately instead of waiting for the changes to be applied
> + * in hardware. 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 temporary or transient visible artifacts while
> + * the update is being applied. Applying the update may also take significantly
> + * more time than a page flip. All visual artifacts will disappear by the time
> + * the update is completed, as signalled throught the vblank event's timestamp

typo: throught

> + * (see struct drm_event_vblank).
> + *
> + * This flag must be set when the KMS update might cause visible artifacts.
> + * Without this flag such KMS update will return a EINVAL error. What kind of
> + * update may cause visible artifacts depends on the driver and the hardware.
> + * User-space 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.
> + *
> + * To the best of the driver's knowledge, visual artifacts are guaranteed to
> + * not appear when this flag is not set. Some sinks might display visual
> + * artifacts outside of the driver's control.

Ok, so we kept the "visual artifacts" semantics and allow monitors to
do otherwise.

I'm still not sure what this means for things like infoframe data where
changing a certain field (e.g. HDR_OUTPUT_METADATA structure's EOTF
field) has a high risk of causing a visual glitch. I cannot imagine why
a monitor manufacturer would not be able to avoid the glitch if they
wanted to. The glitch might or might not happen, and we cannot know in
advance or afterwards whether it did happen, but it is probable that
many monitors will glitch.

I think "To the best of driver's knowledge" means that if someone
reports a monitor to glitch, the driver/kernel would need to add that
field to the "needs modeset" set. But doing so can regress other
monitors that didn't glitch, so it needs to be a monitor quirk.

This is not something for this patch, but would it be possible to agree
on the kernel development policy here? Should glitches be reported and
added to monitor quirks list? Or should drivers be defensive from the
start and require modeset if the field is known to cause glitches on
some monitors?

> + */
>  #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: [PATCH v3] drm: document uAPI page-flip flags

[Simon Ser]

On Wednesday, September 28th, 2022 at 12:06, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> > +/**
> > + * DRM_MODE_PAGE_FLIP_FLAGS
> > + *
> > + * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.
> 
> Should this mention also drm_mode_crtc_page_flip.flags?
> 
> UAPI header defines both structs.

drm_mode_crtc_page_flip is "v1", drm_mode_crtc_page_flip_target is "v2". The
latter just replaces a reserved field with a new one. So I figured "v1" is
mostly kept around for backwards compat and everybody should use "v2" for
simplicity's sake.

> > +/**
> > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > + *
> > + * Allow the update to result in temporary or transient visible artifacts while
> > + * the update is being applied. Applying the update may also take significantly
> > + * more time than a page flip. All visual artifacts will disappear by the time
> > + * the update is completed, as signalled throught the vblank event's timestamp
> 
> typo: throught

Indeed!

> > + * (see struct drm_event_vblank).
> > + *
> > + * This flag must be set when the KMS update might cause visible artifacts.
> > + * Without this flag such KMS update will return a EINVAL error. What kind of
> > + * update may cause visible artifacts depends on the driver and the hardware.
> > + * User-space 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.
> > + *
> > + * To the best of the driver's knowledge, visual artifacts are guaranteed to
> > + * not appear when this flag is not set. Some sinks might display visual
> > + * artifacts outside of the driver's control.
> 
> Ok, so we kept the "visual artifacts" semantics and allow monitors to
> do otherwise.
> 
> I'm still not sure what this means for things like infoframe data where
> changing a certain field (e.g. HDR_OUTPUT_METADATA structure's EOTF
> field) has a high risk of causing a visual glitch. I cannot imagine why
> a monitor manufacturer would not be able to avoid the glitch if they
> wanted to. The glitch might or might not happen, and we cannot know in
> advance or afterwards whether it did happen, but it is probable that
> many monitors will glitch.
> 
> I think "To the best of driver's knowledge" means that if someone
> reports a monitor to glitch, the driver/kernel would need to add that
> field to the "needs modeset" set. But doing so can regress other
> monitors that didn't glitch, so it needs to be a monitor quirk.
> 
> This is not something for this patch, but would it be possible to agree
> on the kernel development policy here? Should glitches be reported and
> added to monitor quirks list? Or should drivers be defensive from the
> start and require modeset if the field is known to cause glitches on
> some monitors?

Good question. I am not sure there is even a desire to have a quirks table for
this among driver developers.

This reminds me of some VRR displays showing visual artifacts when user-space
changes its page-flip rate. If we took this definition by the letter, the
kernel should require ALLOW_MODESET at each page-flip... (The better solution
would be to just to have a slew rate implemented in the kernel. I think it's
implemented to some extent in amdgpu/i915 but still some monitors are having
issues.)

Re: [PATCH v3] drm: document uAPI page-flip flags

[Sebastian Wick]

On Thu, Sep 29, 2022 at 8:11 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Wednesday, September 28th, 2022 at 12:06, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> > > +/**
> > > + * DRM_MODE_PAGE_FLIP_FLAGS
> > > + *
> > > + * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.
> >
> > Should this mention also drm_mode_crtc_page_flip.flags?
> >
> > UAPI header defines both structs.
>
> drm_mode_crtc_page_flip is "v1", drm_mode_crtc_page_flip_target is "v2". The
> latter just replaces a reserved field with a new one. So I figured "v1" is
> mostly kept around for backwards compat and everybody should use "v2" for
> simplicity's sake.
>
> > > +/**
> > > + * DRM_MODE_ATOMIC_ALLOW_MODESET
> > > + *
> > > + * Allow the update to result in temporary or transient visible artifacts while
> > > + * the update is being applied. Applying the update may also take significantly
> > > + * more time than a page flip. All visual artifacts will disappear by the time
> > > + * the update is completed, as signalled throught the vblank event's timestamp
> >
> > typo: throught
>
> Indeed!
>
> > > + * (see struct drm_event_vblank).
> > > + *
> > > + * This flag must be set when the KMS update might cause visible artifacts.
> > > + * Without this flag such KMS update will return a EINVAL error. What kind of
> > > + * update may cause visible artifacts depends on the driver and the hardware.
> > > + * User-space 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.
> > > + *
> > > + * To the best of the driver's knowledge, visual artifacts are guaranteed to
> > > + * not appear when this flag is not set. Some sinks might display visual
> > > + * artifacts outside of the driver's control.
> >
> > Ok, so we kept the "visual artifacts" semantics and allow monitors to
> > do otherwise.
> >
> > I'm still not sure what this means for things like infoframe data where
> > changing a certain field (e.g. HDR_OUTPUT_METADATA structure's EOTF
> > field) has a high risk of causing a visual glitch. I cannot imagine why
> > a monitor manufacturer would not be able to avoid the glitch if they
> > wanted to. The glitch might or might not happen, and we cannot know in
> > advance or afterwards whether it did happen, but it is probable that
> > many monitors will glitch.
> >
> > I think "To the best of driver's knowledge" means that if someone
> > reports a monitor to glitch, the driver/kernel would need to add that
> > field to the "needs modeset" set. But doing so can regress other
> > monitors that didn't glitch, so it needs to be a monitor quirk.
> >
> > This is not something for this patch, but would it be possible to agree
> > on the kernel development policy here? Should glitches be reported and
> > added to monitor quirks list? Or should drivers be defensive from the
> > start and require modeset if the field is known to cause glitches on
> > some monitors?
>
> Good question. I am not sure there is even a desire to have a quirks table for
> this among driver developers.
>
> This reminds me of some VRR displays showing visual artifacts when user-space
> changes its page-flip rate. If we took this definition by the letter, the
> kernel should require ALLOW_MODESET at each page-flip... (The better solution
> would be to just to have a slew rate implemented in the kernel. I think it's
> implemented to some extent in amdgpu/i915 but still some monitors are having
> issues.)
>

Honestly the right thing to do here is give up on the "no visual
artifacts" phrashing. This is really about not having to interrupt the
stream which definitely results in visual artifacts but avoiding that
doesn't guarantee no artifacts.

Re: [PATCH v3] drm: document uAPI page-flip flags

[Pekka Paalanen]

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

On Thu, 29 Sep 2022 18:10:28 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Wednesday, September 28th, 2022 at 12:06, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > > +/**
> > > + * DRM_MODE_PAGE_FLIP_FLAGS
> > > + *
> > > + * Bitmask of flags suitable for &drm_mode_crtc_page_flip_target.flags.  
> > 
> > Should this mention also drm_mode_crtc_page_flip.flags?
> > 
> > UAPI header defines both structs.  
> 
> drm_mode_crtc_page_flip is "v1", drm_mode_crtc_page_flip_target is "v2". The
> latter just replaces a reserved field with a new one. So I figured "v1" is
> mostly kept around for backwards compat and everybody should use "v2" for
> simplicity's sake.

Makes sense after one finds the doc that says this is a v2 of that.
Backward compat makes sense, because someone might have been setting
.reserved=0 explicitly.

FWIW, libdrm does not use _target for drmModePageFlip().


Thanks,
pq

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