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

[Emil Velikov]

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.

-Emil