On Tue, 28 Feb 2017 18:35:11 +
Emmanuel Gil Peyrot wrote:
> Weston will not repaint until previous update has been acked by a
> pageflip event coming from the drm driver. However, some buggy drivers
> won’t return those events or will stop sending them at some point and
> Weston output repaints will completely freeze. To ease developers’ task
> in testing their drivers, this patch makes compositor-drm use a timer
> to detect cases where those pageflip events stop coming.
>
> This timeout implementation is software only and includes basic
> features usually found in a watchdog. We simply exit Weston gracefully
> with a log message and an exit code when the timout is reached.
>
> The timeout value can be set via weston.ini by adding a
> pageflip-timeout= entry under a new [compositor-drm]
> section. Setting it to 0 disables the timeout feature.
>
> v2:
> - Made sure we would get both the pageflip and the vblank events before
> stopping the timer.
> - Reordered the error and success cases in
> drm_output_pageflip_timer_create() to be more in line with the rest
> of the code.
>
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83884
> Signed-off-by: Frederic Plourde
> Signed-off-by: Emmanuel Gil Peyrot
> ---
> compositor/main.c | 2 ++
> libweston/compositor-drm.c | 64
> ++
> libweston/compositor-drm.h | 7 +
> man/weston.ini.man | 5
> 4 files changed, 78 insertions(+)
>
Hi,
there are some details to fix, but looking good overall.
> diff --git a/compositor/main.c b/compositor/main.c
> index 72c3cd10..e870dd4a 100644
> --- a/compositor/main.c
> +++ b/compositor/main.c
> @@ -1235,6 +1235,8 @@ load_drm_backend(struct weston_compositor *c,
> weston_config_section_get_string(section,
>"gbm-format", &config.gbm_format,
>NULL);
> + weston_config_section_get_uint(section, "pageflip-timeout",
> +&config.pageflip_timeout, 0);
>
> config.base.struct_version = WESTON_DRM_BACKEND_CONFIG_VERSION;
> config.base.struct_size = sizeof(struct weston_drm_backend_config);
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 7f78699c..e85ebab8 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -119,6 +119,7 @@ struct drm_backend {
> int32_t cursor_height;
>
> uint32_t connector;
> + uint32_t pageflip_timeout;
> };
>
> struct drm_mode {
> @@ -182,6 +183,8 @@ struct drm_output {
>
> struct vaapi_recorder *recorder;
> struct wl_listener recorder_frame_listener;
> +
> + struct wl_event_source *pageflip_timer;
> };
>
> /*
> @@ -225,6 +228,44 @@ to_drm_backend(struct weston_compositor *base)
> return container_of(base->backend, struct drm_backend, base);
> }
>
> +static int
> +pageflip_timeout(void *data) {
> + /*
> + * Our timer just went off, that means we're not receiving drm
> + * page flip events anymore for that output. Let's gracefully exit
> + * weston with a return value so devs can debug what's going on.
> + */
> + struct drm_output *output = data;
> + struct weston_compositor *compositor = output->base.compositor;
> +
> + weston_log("Pageflip timeout reached, your driver is probably "
> +"buggy! Exiting.\n");
While at it, you might print the output name here.
> + weston_compositor_exit_with_code(compositor, EXIT_FAILURE);
> +
> + return -1;
pageflip_timeout() is a timer callback, which is a libwayland-server
event loop dispatch vfunc. Returning -1 here might theoretically
confuse other dispatch vfuncs called from wl_event_loop_dispatch()'s
post_dispatch_check() phase. The post_dispatch_check() is intended to
loop until all dispatch funcs return zero.
However, a source must be explicitly marked to be processed in
post_dispatch_check() by calling wl_event_source_check(), and we don't
do that for timers in Weston. While it is harmless to return -1 in this
case, I would like to see the return value to be 0 to denote there is
no post-check needed.
> +}
> +
> +/* Creates the pageflip timer. Note that it isn't armed by default */
> +static int
> +drm_output_pageflip_timer_create(struct drm_output *output)
> +{
> + struct wl_event_loop *loop = NULL;
> + struct weston_compositor *ec = output->base.compositor;
> +
> + loop = wl_display_get_event_loop(ec->wl_display);
> + assert(loop);
> + output->pageflip_timer = wl_event_loop_add_timer(loop,
> + pageflip_timeout,
> + output);
> +
> + if (output->pageflip_timer == NULL) {
> + weston_log("creating drm pageflip timer failed: %m\n");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static void
> drm_output_set_cursor