[PATCH weston v2 3/4] libweston: Add check and fallback for linux/sync_file.h

[Pekka Paalanen]

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

> The sync file functionality is required by the upcoming GPU render
> timeline work, but it's only available in relatively new linux kernel
> versions (4.7 and above).
> 
> This commit provides an in-tree copy of the required sync file
> definitions.  On systems that don't have the sync file header (due to
> having an older kernel), we will be able to fall back to our own copy
> when building.
> 
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
> 
> Changes in v2:
>   - New patch.
> 
>  Makefile.am                  |  1 +
>  configure.ac                 |  1 +
>  libweston/weston-sync-file.h | 73 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+)
>  create mode 100644 libweston/weston-sync-file.h

> diff --git a/libweston/weston-sync-file.h b/libweston/weston-sync-file.h
> new file mode 100644
> index 00000000..4d179df0
> --- /dev/null
> +++ b/libweston/weston-sync-file.h
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright (C) 2012 Google, Inc.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Hi,

I'm slightly nervous about this note about GPL. I assume we should be
just fine with it, just have the comment below explain how this doesn't
make Weston GPL, but on the other hand it actually looks quite easy to
avoid this completely.

Looking at the code in patch 4, we should be fine with just #ifdeffing
the contents of linux_sync_file_read_timestamp() and return -1 when the
header was not present.

I don't think we would necessarily need to bother with disabling the
GPU timestamping on start-up based on header availability. It will just
always fail on reading the timestamp which it would also do if the
kernel was too old. And maybe EGL already does the kernel runtime
checks for us?

> + *
> + */
> +/* Sync file interface used by Weston, copied from kernel's sync_file.h */

If we really want to keep the fallback definition instead, I would
expand on the comment you added:

/* Sync file kernel UABI definitions used by Weston, copied from
 * the Linux kernel version x.xx include/uapi/linux/sync_file.h
 * This is needed for interfacing with the kernel and does not
 * imply the GNU GPL licence on Weston.
 */


But, do people expect to be able to use kernel features that are not
present in the kernel headers used for building the userspace?

Seeing how simple the #ifdef solution is, personally I would prefer it
over the fallback copy of sync_file.h.

> +
> +#ifndef WESTON_SYNC_FILE_H
> +#define WESTON_SYNC_FILE_H


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/20170926/0e6e6485/attachment-0001.sig>