[PATCH weston 3/6] compositor: set presentation.presented flags
Mario Kleiner
mario.kleiner.de at gmail.com
Thu Jan 15 23:48:21 PST 2015
Hi Pekka,
as you know from our off-list conversation i've spent quite a bit of
time the
last week reviewing and testing this patch series (patches 1-6). I
implemented
a first basic usable Waylang backend into my own software and tested
this patch
series with it wrt. timestamp reliability etc. and it so far looks good
to me, at least
for a first iteration of the presentation_feedback extension.
Tested on x11 backend, wayland backend, and mostly on the native drm
backend.
On the drm backend i tested single- and dual-dispaly, on Intel-kms and
nouveau-kms,
windowed and fullscreen, with/without overlay planes on, opaque or
transparent, etc.
My hardware measurement equipment confirms that timestamps are precise
and reliable wrt. reality as long as the overlay planes are not used, ie.
everything goes through regular kms-pageflip. Precision on the drm backend
is better than 40 microseconds wrt. to the external measurement hardware,
as expected, essentially perfect :)
So fwiw the whole series is
Reviewed-by: Mario Kleiner <mario.kleiner.de at gmail.com>
Tested-by: Mario Kleiner <mario.kleiner.de at gmail.com>
and enthusiastically
Acked-by: Mario Kleiner <mario.kleiner.de at gmail.com>
for inclusion into Wayland/Weston 1.7, which is why i'm cc'ing Bryce for
hopefully getting this in.
See below for one small thing i would probably change, after looking
through the
way the hardware overlay planes are handled in Weston atm.
...snip...
On 12/17/2014 03:20 PM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> DRM:
>
> Page flip is always vsync'd, and always gets the completion timestamp
> from the kernel which should correspond well to hardware. Completion is
> triggered by the kernel/hardware.
>
> Vblank handler is only used with the broken planes path, therefore do
> not report VSYNC, because we cannot guarantee all the planes updated at
> the same time. We cannot set the INVALID, because it would abort the
> compositor if the broken planes path was ever used. This is a hack that
> will get fixed with nuclear pageflip support in the future.
-> I think additionally to not reporting VSYNC i also would not set the
PRESENTATION_FEEDBACK_KIND_HW_COMPLETION flag in the vblank_handler.
I think there are cases where the vblank event associated with a
drmModeSetPlane
could get dispatched only 1 vblank after the plane update and i think
that shouldn't
qualify for the hw completion flag. Essentially i'd rather wait for all
the nuclear pageflip
work in the kernel to mature and be conservative with these flags.
My feeling is that there might be a few more funky/scary things that
could happen
wrt. timestamping when hardware overlays are used, but i just started
looking through
that code, and i'm tired, so maybe i'm wrong. I couldn't test this so
far. I'm just happy
that the overlay planes support is off by default at the moment :)
Anyway, more feedback later. I'll play with this stuff throughout the
next days and weeks,
i just wanted to make sure that you get some timely feedback before
Wayland/Weston 1.7.
thanks,
-mario
> fbdev:
>
> No vsync, update done by copy, no completion event from hardware, and
> completion time is totally fake.
>
> headless:
>
> No real output to update.
>
> RDP:
>
> Guessing that maybe no vsync, fake time, and copy make sense (pixels
> sent over network). Also no event that the pixels have been shown?
>
> RPI:
>
> Presumably Dispmanx updates are vsync'd. We get a completion event from
> the driver, but need to read the clock ourselves, so the completion time
> is somewhat unreliable. Zero-copy flag not implemented though it would
> be theoretically possible with EGL clients (zero-copy is a per-surface
> flag anyway, so in this patch).
>
> Wayland:
>
> No information how the host compositor is doing updates, so make a safe
> guess without assuming vsync or hardware completion event. While we do
> get some timestamp from the host compositor, it is not the completion
> time. Would need to hook to the Presentation extension of the host
> compositor to get more accurate flags.
>
> X11:
>
> No idea about vsync, completion event, or copying. Also the timestamp is
> a fake.
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
> ---
> src/compositor-drm.c | 13 ++++++++++---
> src/compositor-fbdev.c | 7 +++++--
> src/compositor-headless.c | 9 +++++++--
> src/compositor-rdp.c | 8 ++++++--
> src/compositor-rpi.c | 9 +++++++--
> src/compositor-wayland.c | 3 ++-
> src/compositor-x11.c | 7 +++++--
> src/compositor.c | 19 +++++++++++++------
> src/compositor.h | 6 +++++-
> 9 files changed, 60 insertions(+), 21 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 50d664a..7c551e1 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -50,6 +50,7 @@
> #include "libinput-seat.h"
> #include "launcher-util.h"
> #include "vaapi-recorder.h"
> +#include "presentation_timing-server-protocol.h"
>
> #ifndef DRM_CAP_TIMESTAMP_MONOTONIC
> #define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
> @@ -720,7 +721,8 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> finish_frame:
> /* if we cannot page-flip, immediately finish frame */
> clock_gettime(compositor->base.presentation_clock, &ts);
> - weston_output_finish_frame(output_base, &ts);
> + weston_output_finish_frame(output_base, &ts,
> + PRESENTATION_FEEDBACK_INVALID);
> }
>
> static void
> @@ -741,6 +743,8 @@ vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> struct drm_sprite *s = (struct drm_sprite *)data;
> struct drm_output *output = s->output;
> struct timespec ts;
> + uint32_t flags = PRESENTATION_FEEDBACK_KIND_HW_COMPLETION |
> + PRESENTATION_FEEDBACK_KIND_HW_CLOCK;
>
> drm_output_update_msc(output, frame);
> output->vblank_pending = 0;
> @@ -752,7 +756,7 @@ vblank_handler(int fd, unsigned int frame, unsigned int sec, unsigned int usec,
> if (!output->page_flip_pending) {
> ts.tv_sec = sec;
> ts.tv_nsec = usec * 1000;
> - weston_output_finish_frame(&output->base, &ts);
> + weston_output_finish_frame(&output->base, &ts, flags);
> }
> }
>
> @@ -765,6 +769,9 @@ page_flip_handler(int fd, unsigned int frame,
> {
> struct drm_output *output = (struct drm_output *) data;
> struct timespec ts;
> + uint32_t flags = PRESENTATION_FEEDBACK_KIND_VSYNC |
> + PRESENTATION_FEEDBACK_KIND_HW_COMPLETION |
> + PRESENTATION_FEEDBACK_KIND_HW_CLOCK;
>
> drm_output_update_msc(output, frame);
>
> @@ -784,7 +791,7 @@ page_flip_handler(int fd, unsigned int frame,
> else if (!output->vblank_pending) {
> ts.tv_sec = sec;
> ts.tv_nsec = usec * 1000;
> - weston_output_finish_frame(&output->base, &ts);
> + weston_output_finish_frame(&output->base, &ts, flags);
>
> /* We can't call this from frame_notify, because the output's
> * repaint needed flag is cleared just after that */
> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index 65bb035..1e6f6f1 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -44,6 +44,7 @@
> #include "pixman-renderer.h"
> #include "libinput-seat.h"
> #include "gl-renderer.h"
> +#include "presentation_timing-server-protocol.h"
>
> struct fbdev_compositor {
> struct weston_compositor base;
> @@ -117,7 +118,7 @@ fbdev_output_start_repaint_loop(struct weston_output *output)
> struct timespec ts;
>
> clock_gettime(output->compositor->presentation_clock, &ts);
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> }
>
> static void
> @@ -220,8 +221,10 @@ static int
> finish_frame_handler(void *data)
> {
> struct fbdev_output *output = data;
> + struct timespec ts;
>
> - fbdev_output_start_repaint_loop(&output->base);
> + clock_gettime(output->base.compositor->presentation_clock, &ts);
> + weston_output_finish_frame(&output->base, &ts, 0);
>
> return 1;
> }
> diff --git a/src/compositor-headless.c b/src/compositor-headless.c
> index 945c84b..1e6df9a 100644
> --- a/src/compositor-headless.c
> +++ b/src/compositor-headless.c
> @@ -30,6 +30,7 @@
>
> #include "compositor.h"
> #include "pixman-renderer.h"
> +#include "presentation_timing-server-protocol.h"
>
> struct headless_compositor {
> struct weston_compositor base;
> @@ -58,13 +59,17 @@ headless_output_start_repaint_loop(struct weston_output *output)
> struct timespec ts;
>
> clock_gettime(output->compositor->presentation_clock, &ts);
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> }
>
> static int
> finish_frame_handler(void *data)
> {
> - headless_output_start_repaint_loop(data);
> + struct headless_output *output = data;
> + struct timespec ts;
> +
> + clock_gettime(output->base.compositor->presentation_clock, &ts);
> + weston_output_finish_frame(&output->base, &ts, 0);
>
> return 1;
> }
> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> index 2048f8f..1c3f988 100644
> --- a/src/compositor-rdp.c
> +++ b/src/compositor-rdp.c
> @@ -308,7 +308,7 @@ rdp_output_start_repaint_loop(struct weston_output *output)
> struct timespec ts;
>
> clock_gettime(output->compositor->presentation_clock, &ts);
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> }
>
> static int
> @@ -350,7 +350,11 @@ rdp_output_destroy(struct weston_output *output_base)
> static int
> finish_frame_handler(void *data)
> {
> - rdp_output_start_repaint_loop(data);
> + struct rdp_output *output = data;
> + struct timespec ts;
> +
> + clock_gettime(output->base.compositor->presentation_clock, &ts);
> + weston_output_finish_frame(&output->base, &ts, 0);
>
> return 1;
> }
> diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> index 150e9e1..ec8ffff 100644
> --- a/src/compositor-rpi.c
> +++ b/src/compositor-rpi.c
> @@ -47,6 +47,7 @@
> #include "rpi-renderer.h"
> #include "launcher-util.h"
> #include "libinput-seat.h"
> +#include "presentation_timing-server-protocol.h"
>
> #if 0
> #define DBG(...) \
> @@ -215,8 +216,9 @@ rpi_output_start_repaint_loop(struct weston_output *output)
> {
> struct timespec ts;
>
> + /* XXX: do a phony dispmanx update and trigger on its completion? */
> clock_gettime(output->compositor->presentation_clock, &ts);
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> }
>
> static int
> @@ -250,10 +252,13 @@ static void
> rpi_output_update_complete(struct rpi_output *output,
> const struct timespec *stamp)
> {
> + uint32_t flags = PRESENTATION_FEEDBACK_KIND_VSYNC |
> + PRESENTATION_FEEDBACK_KIND_HW_COMPLETION;
> +
> DBG("frame update complete(%ld.%09ld)\n",
> (long)stamp->tv_sec, (long)stamp->tv_nsec);
> rpi_renderer_finish_frame(&output->base);
> - weston_output_finish_frame(&output->base, stamp);
> + weston_output_finish_frame(&output->base, stamp, flags);
> }
>
> static void
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 65bce39..04a2331 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -43,6 +43,7 @@
> #include "../shared/os-compatibility.h"
> #include "../shared/cairo-util.h"
> #include "fullscreen-shell-client-protocol.h"
> +#include "presentation_timing-server-protocol.h"
>
> #define WINDOW_TITLE "Weston Compositor"
>
> @@ -313,7 +314,7 @@ frame_done(void *data, struct wl_callback *callback, uint32_t time)
> /* XXX: use the presentation extension for proper timings */
> ts.tv_sec = time / 1000;
> ts.tv_nsec = (time % 1000) * 1000000;
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, 0);
> }
>
> static const struct wl_callback_listener frame_listener = {
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index a760f33..dea89c3 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -51,6 +51,7 @@
> #include "pixman-renderer.h"
> #include "../shared/config-parser.h"
> #include "../shared/image-loader.h"
> +#include "presentation_timing-server-protocol.h"
>
> #define DEFAULT_AXIS_STEP_DISTANCE wl_fixed_from_int(10)
>
> @@ -341,7 +342,7 @@ x11_output_start_repaint_loop(struct weston_output *output)
> struct timespec ts;
>
> clock_gettime(output->compositor->presentation_clock, &ts);
> - weston_output_finish_frame(output, &ts);
> + weston_output_finish_frame(output, &ts, PRESENTATION_FEEDBACK_INVALID);
> }
>
> static int
> @@ -450,8 +451,10 @@ static int
> finish_frame_handler(void *data)
> {
> struct x11_output *output = data;
> + struct timespec ts;
>
> - x11_output_start_repaint_loop(&output->base);
> + clock_gettime(output->base.compositor->presentation_clock, &ts);
> + weston_output_finish_frame(&output->base, &ts, 0);
>
> return 1;
> }
> diff --git a/src/compositor.c b/src/compositor.c
> index b84658a..8085b05 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -495,12 +495,12 @@ weston_presentation_feedback_present(
> struct weston_output *output,
> uint32_t refresh_nsec,
> const struct timespec *ts,
> - uint64_t seq)
> + uint64_t seq,
> + uint32_t flags)
> {
> struct wl_client *client = wl_resource_get_client(feedback->resource);
> struct wl_resource *o;
> uint64_t secs;
> - uint32_t flags = 0;
>
> wl_resource_for_each(o, &output->resource_list) {
> if (wl_resource_get_client(o) != client)
> @@ -524,13 +524,18 @@ weston_presentation_feedback_present_list(struct wl_list *list,
> struct weston_output *output,
> uint32_t refresh_nsec,
> const struct timespec *ts,
> - uint64_t seq)
> + uint64_t seq,
> + uint32_t flags)
> {
> struct weston_presentation_feedback *feedback, *tmp;
>
> + assert(!(flags & PRESENTATION_FEEDBACK_INVALID) ||
> + wl_list_empty(list));
> +
> wl_list_for_each_safe(feedback, tmp, list, link)
> weston_presentation_feedback_present(feedback, output,
> - refresh_nsec, ts, seq);
> + refresh_nsec, ts, seq,
> + flags);
> }
>
> static void
> @@ -2083,7 +2088,8 @@ weston_compositor_read_input(int fd, uint32_t mask, void *data)
>
> WL_EXPORT void
> weston_output_finish_frame(struct weston_output *output,
> - const struct timespec *stamp)
> + const struct timespec *stamp,
> + uint32_t presented_flags)
> {
> struct weston_compositor *compositor = output->compositor;
> struct wl_event_loop *loop =
> @@ -2097,7 +2103,8 @@ weston_output_finish_frame(struct weston_output *output,
> refresh_nsec = 1000000000000UL / output->current_mode->refresh;
> weston_presentation_feedback_present_list(&output->feedback_list,
> output, refresh_nsec, stamp,
> - output->msc);
> + output->msc,
> + presented_flags);
>
> output->frame_time = stamp->tv_sec * 1000 + stamp->tv_nsec / 1000000;
>
> diff --git a/src/compositor.h b/src/compositor.h
> index 3f7ed4a..3a26635 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -1066,9 +1066,13 @@ weston_compositor_stack_plane(struct weston_compositor *ec,
> struct weston_plane *plane,
> struct weston_plane *above);
>
> +/* An invalid flag in presented_flags to catch logic errors. */
> +#define PRESENTATION_FEEDBACK_INVALID (1U << 31)
> +
> void
> weston_output_finish_frame(struct weston_output *output,
> - const struct timespec *stamp);
> + const struct timespec *stamp,
> + uint32_t presented_flags);
> void
> weston_output_schedule_repaint(struct weston_output *output);
> void
More information about the wayland-devel
mailing list