| Message ID | 20220928094149.118943-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm: document uAPI page-flip flags | expand |
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
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.)
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.
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
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 |\
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(-)