[PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2
Mario Kleiner
mario.kleiner.de at gmail.com
Tue Aug 16 01:32:51 UTC 2016
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 at 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 at ffwll.ch>
> Signed-off-by: Michel Dänzer <michel.daenzer at 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;
>
More information about the amd-gfx
mailing list