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

[Alexandros Frantzis]

On Tue, Sep 26, 2017 at 02:07:45PM +0300, Pekka Paalanen wrote:
> 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?

The same question has been implicitly asked for EGL, too,
and the answer there seems to be that we care about building on systems
that don't have all the EGL functionality Weston may use at runtime. I
would expect the same answer to apply to the kernel too, when possible.

All that being said, I understand the reluctance to include a copy of
the kernel userspace API header, since it's a grey area in terms of GPL.
Google did a similar thing for bionic a few years ago, and although this
move caused some concern, no definitive legal answer was given (bionic
still uses a striped down version of the headers without issue today).
It's unfortunate that the situation is not clearer.

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

As mentioned above, I don't think it's ideal from a practical
perspective, but I agree it's probably the simplest way to move forward
(without involving lawyers :)). I will respin the patchset to include
this change (and also some changes discussed for patch 4).

> > +
> > +#ifndef WESTON_SYNC_FILE_H
> > +#define WESTON_SYNC_FILE_H
> 
> 
> Thanks,
> pq

Thanks,
Alexandros