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

[Alexandros Frantzis]

On Thu, Sep 28, 2017 at 11:44:14AM +0300, Pekka Paalanen wrote:
> On Wed, 27 Sep 2017 15:01:55 +0100
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 
> > On 27 September 2017 at 13:26, Alexandros Frantzis
> > <alexandros.frantzis at collabora.com> wrote:
> > > On Mon, Sep 25, 2017 at 05:40:36PM +0300, Pekka Paalanen wrote:  
> > >> 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>  
> > >
> > > Hi Pekka,
> > >
> > > <snip>  
> > >> > +
> > >> > +   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.  
> > >
> > > The issue here is that the first struct member is an array, and
> > > this seems to confuse older gcc versions when using '= { 0 }'.
> > >
> > > I considered '= {}' but this seems to be a GCC extension, and wasn't
> > > used anywhere else in the code base, so in v3 I opted for the standard
> > > compliant solution '= { { 0 } }'.
> > >  
> > FWIW, some versions of GCC/Clang will report a warning on '{ 0 }'
> > while others - '{ { 0 } }'.
> > The GCC extension '{}' is supported by both, for ages. Not sure about
> > other compilers.
> > 
> > Regardless, staying consistent with the existing code sounds like the
> > best option IMHO.
> 
> Hm, weston already has 19 instances of {}, but 12 instances of {{0}}.

You are right, I was grepping in 'libweston' not the whole source tree.

> Whatever, I don't mind as long as the compiler doesn't mind.
> 
> Emil, thanks for confirming the {} syntax should essentially be safe.

Since '= {}' seems to be widely supported for the compilers and version
we care for, feel free to change it if you feel it's more consistent (I
am fine either way, too).

Thanks,
Alexandros