Re: [PATCH weston v2] compositor-drm: pageflip timeout implementation

[Pekka Paalanen]

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", _format,
>NULL);
> + weston_config_section_get_uint(section, "pageflip-timeout",
> +_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;
> +}
> +
>  

Re: [PATCH weston v2] compositor-drm: pageflip timeout implementation

[Emmanuel Gil Peyrot]

On Tue, Feb 28, 2017 at 06:35:11PM +, 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 

Err, I forgot to include this:
Reviewed-by: Daniel Stone 

-- 
Emmanuel Gil Peyrot
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v2] compositor-drm: pageflip timeout implementation

[Daniel Stone]

Hi Emmanuel,

On 6 January 2017 at 21:17, Emmanuel Gil Peyrot
 wrote:
> @@ -858,6 +907,10 @@ vblank_handler(int fd, unsigned int frame, unsigned int 
> sec, unsigned int usec,
> s->current = s->next;
> s->next = NULL;
>
> +   /* Stop the pageflip timer instead of rearming it here */
> +   if (output->pageflip_timer)
> +   wl_event_source_timer_update(output->pageflip_timer, 0);
> +
> if (!output->page_flip_pending) {
> ts.tv_sec = sec;
> ts.tv_nsec = usec * 1000;
> @@ -891,6 +944,10 @@ page_flip_handler(int fd, unsigned int frame,
>
> output->page_flip_pending = 0;
>
> +   /* Stop the pageflip timer instead of rearming it here */
> +   if (output->pageflip_timer)
> +   wl_event_source_timer_update(output->pageflip_timer, 0);
> +
> if (output->destroy_pending)
> drm_output_destroy(>base);
> else if (output->disable_pending)

Can you please move both of these into the bottom negative conditional
where we call weston_output_finish_frame? i.e. in vblank_handler, we
should only disarm the timer if (!output->page_flip_pending), and in
the pageflip handler, we should only disarm if
(!output->vblank_pending). This makes sure that if using legacy plane
updates (sigh), we wait for both of the pageflip event for the primary
plane, and the vblank events for the planes, to arrive before
disarming the timer. Without doing that, we could get a false
negative: if the pageflip completion arrives but not the vblank event,
we'll disarm the event, but not actually resume repaint.

With that fixed:
Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel