[PATCH weston v2 4/4] gl-renderer: Emit GPU rendering begin and end timeline timepoints

[Pekka Paalanen]

On Tue, 19 Sep 2017 14:59:11 +0300
Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:

> Use EGL fence sync objects to emit timepoints for the beginning and the
> end of rendering on the GPU. The timepoints are emitted asynchronously
> using the sync file fds associated with the fence sync objects. The sync
> file fds are acquired using the facilities provided by the
> EGL_ANDROID_native_fence_sync extension.
> 
> The asynchronous timepoint submissions are stored in a list in
> gl_output_state until they are executed, and any pending submissions
> that remain at output destruction time are cleaned up.
> 
> If timelining is inactive or the required EGL extensions are not
> present, then GPU timepoint processing and emission are skipped.
> 
> Note that the GPU timestamps returned by sync files are in the
> CLOCK_MONOTONIC clock domain, and are thus compatible with the
> timeline timestamps (which also are in the CLOCK_MONOTONIC domain).
> 
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
> 
> Changes in v2:
>  - Added Signed-off-by.
>  - Used fallback sync_file.h header if required.
>  - Improved error checking.
>  - Used enum timeline_render_point_type instead of int.
>  - Stored pending timeline submissions in a list in gl_output_state.
>  - Renamed various struct and function to improve consistency and clarity.
>  - Added fallbacks for newly used EGL definitions.
>  - Added log messages for render gpu related warnings.
> 
>  libweston/gl-renderer.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++++
>  shared/weston-egl-ext.h |  12 ++++
>  2 files changed, 174 insertions(+)

Hi,

patches 1, 2 and 4 are:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Furthermore, patch 4 was already previously reviewed by Daniel on the
sync bits:
https://lists.freedesktop.org/archives/wayland-devel/2017-September/035031.html

I do have a few minor adjustments for this patch below that I have
already done privately.

> 
> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> index c2e88a65..62a36205 100644
> --- a/libweston/gl-renderer.c
> +++ b/libweston/gl-renderer.c
> @@ -39,6 +39,16 @@
>  #include <assert.h>
>  #include <linux/input.h>
>  #include <drm_fourcc.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +
> +#ifdef HAVE_LINUX_SYNC_FILE

configure.ac generates this for me:

/* Define to 1 if you have the <linux/sync_file.h> header file. */
/* #undef HAVE_LINUX_SYNC_FILE_H */

So I think it's missing the _H. I can fix that.

> +#include <linux/sync_file.h>
> +#else
> +#include "weston-sync-file.h"
> +#endif
> +
> +#include "timeline.h"
>  
>  #include "gl-renderer.h"
>  #include "vertex-clipping.h"
> @@ -47,6 +57,7 @@
>  
>  #include "shared/helpers.h"
>  #include "shared/platform.h"
> +#include "shared/timespec-util.h"
>  #include "weston-egl-ext.h"
>  
>  struct gl_shader {
> @@ -87,6 +98,8 @@ struct gl_output_state {
>  	enum gl_border_status border_status;
>  
>  	struct weston_matrix output_matrix;
> +
> +	struct wl_list timeline_render_point_list;

We usually have a comment saying which struct and link field are stored
in this list.

>  };
>  
>  enum buffer_type {
> @@ -238,6 +251,20 @@ struct gl_renderer {
>  	PFNEGLDUPNATIVEFENCEFDANDROIDPROC dup_native_fence_fd;
>  };
>  
> +enum timeline_render_point_type {
> +	TIMELINE_RENDER_POINT_TYPE_BEGIN,
> +	TIMELINE_RENDER_POINT_TYPE_END
> +};
> +
> +struct timeline_render_point {
> +	struct wl_list link;

We often have a comment saying which list the link is being used for.

> +
> +	enum timeline_render_point_type type;
> +	int fd;
> +	struct weston_output *output;
> +	struct wl_event_source *event_source;
> +};
> +
>  static PFNEGLGETPLATFORMDISPLAYEXTPROC get_platform_display = NULL;
>  
>  static inline const char *
> @@ -274,6 +301,115 @@ get_renderer(struct weston_compositor *ec)
>  	return (struct gl_renderer *)ec->renderer;
>  }
>  
> +static int
> +linux_sync_file_read_timestamp(int fd, uint64_t *ts)
> +{
> +	struct sync_file_info file_info = { 0 };
> +	struct sync_fence_info fence_info = { 0 };

For me, these generate compiler warnings.

/home/pq/git/weston/libweston/gl-renderer.c: In function ‘linux_sync_file_read_timestamp’:
/home/pq/git/weston/libweston/gl-renderer.c:307:9: warning: missing braces around initializer [-Wmissing-braces]
  struct sync_file_info file_info = { 0 };
         ^
/home/pq/git/weston/libweston/gl-renderer.c:307:9: warning: (near initialization for ‘file_info.name’) [-Wmissing-braces]
/home/pq/git/weston/libweston/gl-renderer.c:308:9: warning: missing braces around initializer [-Wmissing-braces]
  struct sync_fence_info fence_info = { 0 };
         ^

I suppose it's a gcc version thing, looks like I'm still on 4.9.3.

I think just dropping the zero there is enough to fix it. I can do that
when merging the patches.

> +
> +	assert(ts != NULL);
> +
> +	file_info.sync_fence_info = (uint64_t)(uintptr_t)&fence_info;
> +	file_info.num_fences = 1;
> +
> +	if (ioctl(fd, SYNC_IOC_FILE_INFO, &file_info) < 0)
> +		return -1;
> +
> +	*ts = fence_info.timestamp_ns;
> +
> +	return 0;
> +}


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170925/95a34fb1/attachment.sig>