Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2

Cc'ing Daniel Stone and Pekka Paalanen, because this relates to wayland.

Wrt. having a new pageflip parameter struct, i wonder if it wouldn't make sense to then already prepare some space in it for specifying an absolute target time, e.g., in u64 microseconds? Or make this part of the atomic pageflip framework?

In Wayland we now have the presentation_feedback extension (maybe it got a new name?), which is great to get feedback when and how presentation was completed, essentially the equivalent of X11's GLX_INTEL_swap_events + some useful extra info / the feedback half of OML_sync_control.

That was supposed to be complemented by a presentation_queue extension which would provide the other half of OML_sync_control, the part where an app can tell the compositor when and how to present surface updates. I remember that a year ago inclusion of that extension was blocked on some other more urgent important blocker bugs? Did this make progress? For timing sensitive applications such an extension is even more important in a wayland world than under X11. On X11 most desktops allow unredirecting fullscreen windows, so an app can drive the flip timing rather direct. At least on Weston as i remember it from my last tests a year ago, that wasn't possible, and an app that doesn't want to present at each video refresh, but at specific target times had to guess what the specific compositors scheduling strategy might be and then "play games" wrt. to timing surface commit's to trick the compositor into sort of doing the right thing - very fragile.

Anyway, the idea of presentation_queue was to specify the requested time of presentation as actual time, not as a target vblank count. This because applications that care about precise presentation timing, like my kind of neuroscience/medical visual stimulation software, or also video players, or e.g., at least the VDPAU video presentation api "think" in absolute time, not in refresh cycles. Also a target vblank count for presentation is less meaningful than a target time for things like variable framerate displays/adaptive sync, or displays which don't have a classic refresh cycle at all. It might also be useful if one thinks about something like VR compositors, where precise timing control could help for some of the tricks ("time warping" iirc?) they use to hide/cope with latency from head tracking -> display.

It would be nice if the kernel could help compositors which would implement presentation_queue or similar to be robust/precise in face of future stuff like Freesync, or of added delays due to Optimus/Prime hybrid-graphics setups. If we wanted to synchronize presentation of multiple displays in a Freesync type display setup, absolute target times could also be helpful.

-mario

On 08/08/2016 09:23 AM, Michel Dänzer wrote:

From: Michel Dänzer michel.daenzer@amd.com

These flags allow userspace to explicitly specify the target vertical blank period when a flip should take effect.

v2:

• Add new struct drm_mode_crtc_page_flip_target instead of modifying struct drm_mode_crtc_page_flip, to make sure all existing userspace code keeps compiling (Daniel Vetter)

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Michel Dänzer michel.daenzer@amd.com

---

drivers/gpu/drm/drm_crtc.c | 48 ++++++++++++++++++++++++++++++++++++++------- drivers/gpu/drm/drm_ioctl.c | 8 ++++++++ include/uapi/drm/drm.h | 1 + include/uapi/drm/drm_mode.h | 39 +++++++++++++++++++++++++++++++++--- 4 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d6491ef..3e1a63d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -5417,15 +5417,23 @@ out: int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_crtc_page_flip *page_flip = data;

• struct drm_mode_crtc_page_flip_target *page_flip = data; struct drm_crtc *crtc; struct drm_framebuffer *fb = NULL; struct drm_pending_vblank_event *e = NULL;

• u32 target_vblank = 0;

• u32 target_vblank = page_flip->sequence; int ret = -EINVAL;

• if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||

•    page_flip->reserved != 0)
  

• if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)

• return -EINVAL;
  

• if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))

• return -EINVAL;
  

• /* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

• * can be specified
  

• */
  

• if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET) return -EINVAL;

  if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)

@@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, return -ENOENT;

if (crtc->funcs->page_flip_target) {

• u32 current_vblank;
  

  int r;

  r = drm_crtc_vblank_get(crtc); if (r) return r;

• target_vblank = drm_crtc_vblank_count(crtc) +
  

• 	!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
  

• } else if (crtc->funcs->page_flip == NULL) {

• current_vblank = drm_crtc_vblank_count(crtc);
  

• switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
  

• case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
  

• 	if ((int)(target_vblank - current_vblank) > 1) {
  

• 		DRM_DEBUG("Invalid absolute flip target %u, "
  

• 			  "must be <= %u\n", target_vblank,
  

• 			  current_vblank + 1);
  

• 		drm_crtc_vblank_put(crtc);
  

• 		return -EINVAL;
  

• 	}
  

• 	break;
  

• case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
  

• 	if (target_vblank != 0 && target_vblank != 1) {
  

• 		DRM_DEBUG("Invalid relative flip target %u, "
  

• 			  "must be 0 or 1\n", target_vblank);
  

• 		drm_crtc_vblank_put(crtc);
  

• 		return -EINVAL;
  

• 	}
  

• 	target_vblank += current_vblank;
  

• 	break;
  

• default:
  

• 	target_vblank = current_vblank +
  

• 		!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
  

• 	break;
  

• }
  

• } else if (crtc->funcs->page_flip == NULL ||

•    (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) {
  

  return -EINVAL; }

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 33af4a5..0099d2a 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data, static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_get_cap *req = data;

• struct drm_crtc *crtc;

  req->value = 0; switch (req->capability) {

@@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_ case DRM_CAP_ASYNC_PAGE_FLIP: req->value = dev->mode_config.async_page_flip; break;

• case DRM_CAP_PAGE_FLIP_TARGET:

• req->value = 1;
  

• drm_for_each_crtc(crtc, dev) {
  

• 	if (!crtc->funcs->page_flip_target)
  

• 		req->value = 0;
  

• }
  

• break;
  

  case DRM_CAP_CURSOR_WIDTH: if (dev->mode_config.cursor_width) req->value = dev->mode_config.cursor_width;

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 452675f..b2c5284 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -646,6 +646,7 @@ struct drm_gem_open { #define DRM_CAP_CURSOR_WIDTH 0x8 #define DRM_CAP_CURSOR_HEIGHT 0x9 #define DRM_CAP_ADDFB2_MODIFIERS 0x10 +#define DRM_CAP_PAGE_FLIP_TARGET 0x11

/** DRM_IOCTL_GET_CAP ioctl argument type */ struct drm_get_cap { diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 49a7265..df0e350 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -520,7 +520,13 @@ struct drm_color_lut {

#define DRM_MODE_PAGE_FLIP_EVENT 0x01 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02 -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC) +#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)
  

+#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \

• 		  DRM_MODE_PAGE_FLIP_ASYNC | \
  

• 		  DRM_MODE_PAGE_FLIP_TARGET)
  
  

/*

• Request a page flip on the specified crtc.

@@ -543,8 +549,7 @@ struct drm_color_lut {

• 'as soon as possible', meaning that it not delay waiting for vblank.
• This may cause tearing on the screen.

• • The reserved field must be zero until we figure out something
• • clever to use it for.

• • The reserved field must be zero.
  */

struct drm_mode_crtc_page_flip { @@ -555,6 +560,34 @@ struct drm_mode_crtc_page_flip { __u64 user_data; };

+/*

• • Request a page flip on the specified crtc.
• • Same as struct drm_mode_crtc_page_flip, but supports new flags and
• • re-purposes the reserved field:
• • The sequence field must be zero unless either of the
• • DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
• • the ABSOLUTE flag is specified, the sequence field denotes the absolute
• • vblank sequence when the flip should take effect. When the RELATIVE
• • flag is specified, the sequence field denotes the relative (to the
• • current one when the ioctl is called) vblank sequence when the flip
• • should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
• • make sure the vblank sequence before the target one has passed before
• • calling this ioctl. The purpose of the
• • DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
• • the target for when code dealing with a page flip runs during a
• • vertical blank period.
• */

+struct drm_mode_crtc_page_flip_target {

• __u32 crtc_id;
• __u32 fb_id;
• __u32 flags;
• __u32 sequence;
• __u64 user_data;

+};

/* create a dumb scanout buffer */ struct drm_mode_create_dumb { __u32 height;