| Message ID | 1477921447-18532-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 31, 2016 at 09:44:07AM -0400, Rob Clark wrote: > From: Rob Clark <robclark@freedesktop.org> > > Rather than cut/pasting these couple ioctl wrappers everywhere, just > stuff them as static-inline into a header. > > Signed-off-by: Rob Clark <robclark@freedesktop.org> > --- > This is probably mostly used from mesa, but some drivers, test apps, etc > may also want to use it from libdrm. > > Makefile.sources | 1 + > libsync.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > create mode 100644 libsync.h > > diff --git a/Makefile.sources b/Makefile.sources > index a57036a..10aa1d0 100644 > --- a/Makefile.sources > +++ b/Makefile.sources > @@ -13,6 +13,7 @@ LIBDRM_FILES := \ > util_math.h > > LIBDRM_H_FILES := \ > + libsync.h \ > xf86drm.h \ > xf86drmMode.h > > diff --git a/libsync.h b/libsync.h > new file mode 100644 > index 0000000..fc23b7f > --- /dev/null > +++ b/libsync.h > @@ -0,0 +1,74 @@ > +/* > + * sync abstraction > + * Copyright 2015-2016 Collabora Ltd. > + * > + * Based on the implementation from the Android Open Source Project, > + * > + * Copyright 2012 Google, Inc > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > + * OTHER DEALINGS IN THE SOFTWARE. > + */ > + > +#ifndef _LIBSYNC_H > +#define _LIBSYNC_H > + > +#include <stdint.h> > +#include <string.h> > +#include <sys/ioctl.h> > +#include <sys/poll.h> > + > +// todo prob should just #include <linux/sync_file.h> ? > +struct sync_merge_data { > + char name[32]; > + int32_t fd2; > + int32_t fence; > + uint32_t flags; > + uint32_t pad; > +}; > +#define SYNC_IOC_MAGIC '>' > +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data) > + > + > + > +static inline int sync_wait(int fd, int timeout) > +{ > + struct pollfd fds; > + > + fds.fd = fd; > + fds.events = POLLIN | POLLERR; POLLERR is implied and ignored in fds.events. > + return poll(&fds, 1, timeout); Returns: -1 on error, 0 on timeout, 1 if signaled. This function is horrible wrt to -EINTR for example :) Hmm, mixing poll() and looping over signals until timeout doesn't look as straightforward. > +} > + > +static inline int sync_merge(const char *name, int fd1, int fd2) > +{ > + struct sync_merge_data data = {}; > + int err; What I liked was doing if (fd2 < 0) return dup(fd1); if (fd1 < 0) return dup(fd2); That makes accumulating the fences in the caller much easier (i.e. they start with batch.fence_in = -1; then batch.fence_in = sync_merge(batch.fence_in, fence); finished by execbuf(&batch); close(batch.fence_in); batch.fence_in = -1; -Chris
On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Oct 31, 2016 at 09:44:07AM -0400, Rob Clark wrote: >> From: Rob Clark <robclark@freedesktop.org> >> >> Rather than cut/pasting these couple ioctl wrappers everywhere, just >> stuff them as static-inline into a header. >> >> Signed-off-by: Rob Clark <robclark@freedesktop.org> >> --- >> This is probably mostly used from mesa, but some drivers, test apps, etc >> may also want to use it from libdrm. >> >> Makefile.sources | 1 + >> libsync.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 75 insertions(+) >> create mode 100644 libsync.h >> >> diff --git a/Makefile.sources b/Makefile.sources >> index a57036a..10aa1d0 100644 >> --- a/Makefile.sources >> +++ b/Makefile.sources >> @@ -13,6 +13,7 @@ LIBDRM_FILES := \ >> util_math.h >> >> LIBDRM_H_FILES := \ >> + libsync.h \ >> xf86drm.h \ >> xf86drmMode.h >> >> diff --git a/libsync.h b/libsync.h >> new file mode 100644 >> index 0000000..fc23b7f >> --- /dev/null >> +++ b/libsync.h >> @@ -0,0 +1,74 @@ >> +/* >> + * sync abstraction >> + * Copyright 2015-2016 Collabora Ltd. >> + * >> + * Based on the implementation from the Android Open Source Project, >> + * >> + * Copyright 2012 Google, Inc >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> + * OTHER DEALINGS IN THE SOFTWARE. >> + */ >> + >> +#ifndef _LIBSYNC_H >> +#define _LIBSYNC_H >> + >> +#include <stdint.h> >> +#include <string.h> >> +#include <sys/ioctl.h> >> +#include <sys/poll.h> >> + >> +// todo prob should just #include <linux/sync_file.h> ? >> +struct sync_merge_data { >> + char name[32]; >> + int32_t fd2; >> + int32_t fence; >> + uint32_t flags; >> + uint32_t pad; >> +}; >> +#define SYNC_IOC_MAGIC '>' >> +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data) >> + >> + >> + >> +static inline int sync_wait(int fd, int timeout) >> +{ >> + struct pollfd fds; >> + >> + fds.fd = fd; >> + fds.events = POLLIN | POLLERR; > > POLLERR is implied and ignored in fds.events. > >> + return poll(&fds, 1, timeout); > > Returns: -1 on error, 0 on timeout, 1 if signaled. > > This function is horrible wrt to -EINTR for example :) Hmm, mixing > poll() and looping over signals until timeout doesn't look as > straightforward. hmm, this was just basically cut/paste from android libsync (and iirc removing the legacy ioctls).. although I did just realize there was a newer version which turned this into: ret = poll(..); if (ret > 0) return 0; return ret; perhaps not super-awesome for -EINTR handling.. and tbh I'm 100% of the behavior when this used to be simply "return ioctl(fd, SYNC_IOC_WAIT, &to);".. >> +} >> + >> +static inline int sync_merge(const char *name, int fd1, int fd2) >> +{ >> + struct sync_merge_data data = {}; >> + int err; > > What I liked was doing > > if (fd2 < 0) > return dup(fd1); > > if (fd1 < 0) > return dup(fd2); > > That makes accumulating the fences in the caller much easier (i.e. they > start with > batch.fence_in = -1; > then > batch.fence_in = sync_merge(batch.fence_in, fence); > finished by > execbuf(&batch); > close(batch.fence_in); > batch.fence_in = -1; I guess that would end up ignoring the fence name.. although not sure that I care. But probably we should either make the android version behave the same, or pick new names for these fxns. I think having same names but slightly different behaviour would be confusing. Oh, and now that I'm actually looking at that code, the extra null terminator bit after the strncpy() is kinda unneeded.. should just do: strncpy(data.name, name, sizeof(data.name)); (without the -1) instead BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Oct 31, 2016 at 09:44:07AM -0400, Rob Clark wrote: >> From: Rob Clark <robclark@freedesktop.org> >> >> Rather than cut/pasting these couple ioctl wrappers everywhere, just >> stuff them as static-inline into a header. >> >> Signed-off-by: Rob Clark <robclark@freedesktop.org> >> --- >> This is probably mostly used from mesa, but some drivers, test apps, etc >> may also want to use it from libdrm. >> >> Makefile.sources | 1 + >> libsync.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 75 insertions(+) >> create mode 100644 libsync.h >> >> diff --git a/Makefile.sources b/Makefile.sources >> index a57036a..10aa1d0 100644 >> --- a/Makefile.sources >> +++ b/Makefile.sources >> @@ -13,6 +13,7 @@ LIBDRM_FILES := \ >> util_math.h >> >> LIBDRM_H_FILES := \ >> + libsync.h \ >> xf86drm.h \ >> xf86drmMode.h >> >> diff --git a/libsync.h b/libsync.h >> new file mode 100644 >> index 0000000..fc23b7f >> --- /dev/null >> +++ b/libsync.h >> @@ -0,0 +1,74 @@ >> +/* >> + * sync abstraction >> + * Copyright 2015-2016 Collabora Ltd. >> + * >> + * Based on the implementation from the Android Open Source Project, >> + * >> + * Copyright 2012 Google, Inc >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> + * OTHER DEALINGS IN THE SOFTWARE. >> + */ >> + >> +#ifndef _LIBSYNC_H >> +#define _LIBSYNC_H >> + >> +#include <stdint.h> >> +#include <string.h> >> +#include <sys/ioctl.h> >> +#include <sys/poll.h> >> + >> +// todo prob should just #include <linux/sync_file.h> ? >> +struct sync_merge_data { >> + char name[32]; >> + int32_t fd2; >> + int32_t fence; >> + uint32_t flags; >> + uint32_t pad; >> +}; >> +#define SYNC_IOC_MAGIC '>' >> +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data) >> + >> + >> + >> +static inline int sync_wait(int fd, int timeout) >> +{ >> + struct pollfd fds; >> + >> + fds.fd = fd; >> + fds.events = POLLIN | POLLERR; > > POLLERR is implied and ignored in fds.events. > >> + return poll(&fds, 1, timeout); > > Returns: -1 on error, 0 on timeout, 1 if signaled. > > This function is horrible wrt to -EINTR for example :) Hmm, mixing > poll() and looping over signals until timeout doesn't look as > straightforward. > >> +} >> + >> +static inline int sync_merge(const char *name, int fd1, int fd2) >> +{ >> + struct sync_merge_data data = {}; >> + int err; > > What I liked was doing > > if (fd2 < 0) > return dup(fd1); > > if (fd1 < 0) > return dup(fd2); > > That makes accumulating the fences in the caller much easier (i.e. they > start with > batch.fence_in = -1; > then > batch.fence_in = sync_merge(batch.fence_in, fence); note that if you don't want to leak fd's you'd have to do something more like: int new_fence = sync_merge(batch->fence_in, fence); if (batch->fence_in != -1) close(batch->fence_in); batch->fence_in = new_fence; so it isn't *that* much better.. I guess you could do the close() unconditionally and ignore the error if batch->fence_in==-1.. BR, -R > finished by > execbuf(&batch); > close(batch.fence_in); > batch.fence_in = -1; > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Oct 31, 2016 at 10:30:23AM -0400, Rob Clark wrote: > On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > What I liked was doing > > > > if (fd2 < 0) > > return dup(fd1); > > > > if (fd1 < 0) > > return dup(fd2); > > > > That makes accumulating the fences in the caller much easier (i.e. they > > start with > > batch.fence_in = -1; > > then > > batch.fence_in = sync_merge(batch.fence_in, fence); > > note that if you don't want to leak fd's you'd have to do something more like: > > int new_fence = sync_merge(batch->fence_in, fence); > if (batch->fence_in != -1) > close(batch->fence_in); > batch->fence_in = new_fence; Hmm. so I thought the ioctl was closing the input. > so it isn't *that* much better.. I guess you could do the close() > unconditionally and ignore the error if batch->fence_in==-1.. Yup, realised after writing that a bit more on the input side is required. if (fd2 < 0) return fd1; if (fd1 < 0) return dup(fd2); ret = ioctl(); if (ret < 0) return fd1; close(fd1); return result.fence; Which discards the synchronisation on the new fence if there's an error, are we meant to flag a GL_ERROR? -Chris
On Mon, Oct 31, 2016 at 10:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Oct 31, 2016 at 10:30:23AM -0400, Rob Clark wrote: >> On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > What I liked was doing >> > >> > if (fd2 < 0) >> > return dup(fd1); >> > >> > if (fd1 < 0) >> > return dup(fd2); >> > >> > That makes accumulating the fences in the caller much easier (i.e. they >> > start with >> > batch.fence_in = -1; >> > then >> > batch.fence_in = sync_merge(batch.fence_in, fence); >> >> note that if you don't want to leak fd's you'd have to do something more like: >> >> int new_fence = sync_merge(batch->fence_in, fence); >> if (batch->fence_in != -1) >> close(batch->fence_in); >> batch->fence_in = new_fence; > > Hmm. so I thought the ioctl was closing the input. hmm, the new ioctl is not.. I guess I need to dig up the old code to confirm it's behaviour was the same. In reality I think most things would want to close() the old fence, but I don't want to change the behaviour from an existing android libsync function of the same name. So probably will keep the existing behaviour but add a new function (sync_merge_if() or something like that.. feel free to suggest a better name) which does the dup/close dance. >> so it isn't *that* much better.. I guess you could do the close() >> unconditionally and ignore the error if batch->fence_in==-1.. > > Yup, realised after writing that a bit more on the input side is > required. > > > if (fd2 < 0) > return fd1; > > if (fd1 < 0) > return dup(fd2); > > ret = ioctl(); > if (ret < 0) > return fd1; > > close(fd1); > return result.fence; > > Which discards the synchronisation on the new fence if there's an error, > are we meant to flag a GL_ERROR? The error checking should already be done at the egl level. The same eglCreateSync() has two modes, one where you pass in -1 and are asking the driver to create a fence, and one where you pass in a valid fd which you want to sync on. For at least the gallium bits, these turn into different code paths into the driver. So the fd2<0 case would be an assert(). I'm not entirely sure what behaviour you'd want for i965. BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
2016-10-31 Rob Clark <robdclark@gmail.com>: > On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Mon, Oct 31, 2016 at 09:44:07AM -0400, Rob Clark wrote: > >> From: Rob Clark <robclark@freedesktop.org> > >> > >> Rather than cut/pasting these couple ioctl wrappers everywhere, just > >> stuff them as static-inline into a header. > >> > >> Signed-off-by: Rob Clark <robclark@freedesktop.org> > >> --- > >> This is probably mostly used from mesa, but some drivers, test apps, etc > >> may also want to use it from libdrm. > >> > >> Makefile.sources | 1 + > >> libsync.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 2 files changed, 75 insertions(+) > >> create mode 100644 libsync.h > >> > >> diff --git a/Makefile.sources b/Makefile.sources > >> index a57036a..10aa1d0 100644 > >> --- a/Makefile.sources > >> +++ b/Makefile.sources > >> @@ -13,6 +13,7 @@ LIBDRM_FILES := \ > >> util_math.h > >> > >> LIBDRM_H_FILES := \ > >> + libsync.h \ > >> xf86drm.h \ > >> xf86drmMode.h > >> > >> diff --git a/libsync.h b/libsync.h > >> new file mode 100644 > >> index 0000000..fc23b7f > >> --- /dev/null > >> +++ b/libsync.h > >> @@ -0,0 +1,74 @@ > >> +/* > >> + * sync abstraction > >> + * Copyright 2015-2016 Collabora Ltd. > >> + * > >> + * Based on the implementation from the Android Open Source Project, > >> + * > >> + * Copyright 2012 Google, Inc > >> + * > >> + * Permission is hereby granted, free of charge, to any person obtaining a > >> + * copy of this software and associated documentation files (the "Software"), > >> + * to deal in the Software without restriction, including without limitation > >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >> + * and/or sell copies of the Software, and to permit persons to whom the > >> + * Software is furnished to do so, subject to the following conditions: > >> + * > >> + * The above copyright notice and this permission notice shall be included in > >> + * all copies or substantial portions of the Software. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > >> + * OTHER DEALINGS IN THE SOFTWARE. > >> + */ > >> + > >> +#ifndef _LIBSYNC_H > >> +#define _LIBSYNC_H > >> + > >> +#include <stdint.h> > >> +#include <string.h> > >> +#include <sys/ioctl.h> > >> +#include <sys/poll.h> > >> + > >> +// todo prob should just #include <linux/sync_file.h> ? > >> +struct sync_merge_data { > >> + char name[32]; > >> + int32_t fd2; > >> + int32_t fence; > >> + uint32_t flags; > >> + uint32_t pad; > >> +}; > >> +#define SYNC_IOC_MAGIC '>' > >> +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data) > >> + > >> + > >> + > >> +static inline int sync_wait(int fd, int timeout) > >> +{ > >> + struct pollfd fds; > >> + > >> + fds.fd = fd; > >> + fds.events = POLLIN | POLLERR; > > > > POLLERR is implied and ignored in fds.events. > > > >> + return poll(&fds, 1, timeout); > > > > Returns: -1 on error, 0 on timeout, 1 if signaled. > > > > This function is horrible wrt to -EINTR for example :) Hmm, mixing > > poll() and looping over signals until timeout doesn't look as > > straightforward. > > hmm, this was just basically cut/paste from android libsync (and iirc > removing the legacy ioctls).. although I did just realize there was a > newer version which turned this into: > > ret = poll(..); > if (ret > 0) > return 0; > return ret; > > perhaps not super-awesome for -EINTR handling.. and tbh I'm 100% of > the behavior when this used to be simply "return ioctl(fd, > SYNC_IOC_WAIT, &to);".. The latest version take a few more cases in account and should behave exatly like SYNC_IOC_WAIT. int sync_wait(int fd, int timeout) { struct pollfd fds; int ret; fds.fd = fd; fds.events = POLLIN; ret = poll(&fds, 1, timeout); if (ret > 0) { if (fds.revents & (POLLERR | POLLNVAL)) errno = EINVAL; return -1; return 0; } else if (ret == 0) { errno = ETIME; return -1; } return ret; } > > >> +} > >> + > >> +static inline int sync_merge(const char *name, int fd1, int fd2) > >> +{ > >> + struct sync_merge_data data = {}; > >> + int err; > > > > What I liked was doing > > > > if (fd2 < 0) > > return dup(fd1); > > > > if (fd1 < 0) > > return dup(fd2); > > > > That makes accumulating the fences in the caller much easier (i.e. they > > start with > > batch.fence_in = -1; > > then > > batch.fence_in = sync_merge(batch.fence_in, fence); > > finished by > > execbuf(&batch); > > close(batch.fence_in); > > batch.fence_in = -1; > > I guess that would end up ignoring the fence name.. although not sure > that I care. But probably we should either make the android version > behave the same, or pick new names for these fxns. I think having > same names but slightly different behaviour would be confusing. > > Oh, and now that I'm actually looking at that code, the extra null > terminator bit after the strncpy() is kinda unneeded.. should just > do: > > strncpy(data.name, name, sizeof(data.name)); > > (without the -1) instead > > BR, > -R > > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Oct 31, 2016 at 10:57:16AM -0400, Rob Clark wrote: > On Mon, Oct 31, 2016 at 10:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Which discards the synchronisation on the new fence if there's an error, > > are we meant to flag a GL_ERROR? > > The error checking should already be done at the egl level. The same > eglCreateSync() has two modes, one where you pass in -1 and are asking > the driver to create a fence, and one where you pass in a valid fd > which you want to sync on. For at least the gallium bits, these turn > into different code paths into the driver. So the fd2<0 case would be > an assert(). I'm not entirely sure what behaviour you'd want for > i965. Either path could reasonably err with ENOMEM, ENFILE. Disregarding the fence (so we keep on bumbling on) and logging the error for later inspection. As far as the backend is concerned, it looks like we just have to ensure we don't lose the current fences and return NULL/-1. Check dri2_dup_native_fence_fd() for error handling of its dup(sync->SyncFd), only some paths set the _eglError(). -Chris
On Mon, Oct 31, 2016 at 11:15 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Oct 31, 2016 at 10:57:16AM -0400, Rob Clark wrote: >> On Mon, Oct 31, 2016 at 10:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > Which discards the synchronisation on the new fence if there's an error, >> > are we meant to flag a GL_ERROR? >> >> The error checking should already be done at the egl level. The same >> eglCreateSync() has two modes, one where you pass in -1 and are asking >> the driver to create a fence, and one where you pass in a valid fd >> which you want to sync on. For at least the gallium bits, these turn >> into different code paths into the driver. So the fd2<0 case would be >> an assert(). I'm not entirely sure what behaviour you'd want for >> i965. > > Either path could reasonably err with ENOMEM, ENFILE. Disregarding the > fence (so we keep on bumbling on) and logging the error for later > inspection. As far as the backend is concerned, it looks like we just > have to ensure we don't lose the current fences and return NULL/-1. > > Check dri2_dup_native_fence_fd() for error handling of its > dup(sync->SyncFd), only some paths set the _eglError(). yeah, I guess spec doesn't really define what happens if EMFILE.. although really quite a lot of things will start falling over at that point. I guess we could set EGL_BAD_PARAMETER although that isn't really the perfect error. BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On 31 October 2016 at 13:44, Rob Clark <robdclark@gmail.com> wrote: > From: Rob Clark <robclark@freedesktop.org> > > Rather than cut/pasting these couple ioctl wrappers everywhere, just > stuff them as static-inline into a header. > > Signed-off-by: Rob Clark <robclark@freedesktop.org> > --- > This is probably mostly used from mesa, but some drivers, test apps, etc > may also want to use it from libdrm. > It makes sense imho. To avoid fun experiences we want the header synced in an identical manner (make headers_install) to the include/drm ones. One might as well move it there, so make its "more" obvious. > +static inline int sync_wait(int fd, int timeout) > +{ > + struct pollfd fds; > + > + fds.fd = fd; > + fds.events = POLLIN | POLLERR; IIRC the API does not mention (forbid even) additional members of the pollfd struct. Let's zero init fds, and the compiler will drop it if/where applicable ? Thanks Emil
On Mon, Oct 31, 2016 at 11:25 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 31 October 2016 at 13:44, Rob Clark <robdclark@gmail.com> wrote: >> From: Rob Clark <robclark@freedesktop.org> >> >> Rather than cut/pasting these couple ioctl wrappers everywhere, just >> stuff them as static-inline into a header. >> >> Signed-off-by: Rob Clark <robclark@freedesktop.org> >> --- >> This is probably mostly used from mesa, but some drivers, test apps, etc >> may also want to use it from libdrm. >> > It makes sense imho. To avoid fun experiences we want the header > synced in an identical manner (make headers_install) to the > include/drm ones. One might as well move it there, so make its "more" > obvious. > hmm, not sure I understand, but '#include <libsync.h>' seems to work in either case.. > >> +static inline int sync_wait(int fd, int timeout) >> +{ >> + struct pollfd fds; >> + >> + fds.fd = fd; >> + fds.events = POLLIN | POLLERR; > IIRC the API does not mention (forbid even) additional members of the > pollfd struct. > Let's zero init fds, and the compiler will drop it if/where applicable ? hmm, and I guess if this gets #include'd from c++ code, does it get more fun? I thought there were some different rules about struct initializers in c++? Or maybe that was just with certain compilers? BR, -R
On 31 October 2016 at 16:39, Rob Clark <robdclark@gmail.com> wrote: > On Mon, Oct 31, 2016 at 11:25 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 31 October 2016 at 13:44, Rob Clark <robdclark@gmail.com> wrote: >>> From: Rob Clark <robclark@freedesktop.org> >>> >>> Rather than cut/pasting these couple ioctl wrappers everywhere, just >>> stuff them as static-inline into a header. >>> >>> Signed-off-by: Rob Clark <robclark@freedesktop.org> >>> --- >>> This is probably mostly used from mesa, but some drivers, test apps, etc >>> may also want to use it from libdrm. >>> >> It makes sense imho. To avoid fun experiences we want the header >> synced in an identical manner (make headers_install) to the >> include/drm ones. One might as well move it there, so make its "more" >> obvious. >> > > hmm, not sure I understand, but '#include <libsync.h>' seems to work > in either case.. > The file is from the kernel UAPI, correct ? If so store it alongside the other UAPI ones include/drm/ and import `make headers_install' (see git log -- include/drm for examples). We should add a README with the above specifics to include/drm/ one of these days :-) >> >>> +static inline int sync_wait(int fd, int timeout) >>> +{ >>> + struct pollfd fds; >>> + >>> + fds.fd = fd; >>> + fds.events = POLLIN | POLLERR; >> IIRC the API does not mention (forbid even) additional members of the >> pollfd struct. >> Let's zero init fds, and the compiler will drop it if/where applicable ? > > > hmm, and I guess if this gets #include'd from c++ code, does it get > more fun? I thought there were some different rules about struct > initializers in c++? Or maybe that was just with certain compilers? > Initializers are fun in C++ indeed. Having the extern C wrappers (as per your v2) helps. Thanks Emil
On Mon, Oct 31, 2016 at 1:15 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 31 October 2016 at 16:39, Rob Clark <robdclark@gmail.com> wrote: >> On Mon, Oct 31, 2016 at 11:25 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> On 31 October 2016 at 13:44, Rob Clark <robdclark@gmail.com> wrote: >>>> From: Rob Clark <robclark@freedesktop.org> >>>> >>>> Rather than cut/pasting these couple ioctl wrappers everywhere, just >>>> stuff them as static-inline into a header. >>>> >>>> Signed-off-by: Rob Clark <robclark@freedesktop.org> >>>> --- >>>> This is probably mostly used from mesa, but some drivers, test apps, etc >>>> may also want to use it from libdrm. >>>> >>> It makes sense imho. To avoid fun experiences we want the header >>> synced in an identical manner (make headers_install) to the >>> include/drm ones. One might as well move it there, so make its "more" >>> obvious. >>> >> >> hmm, not sure I understand, but '#include <libsync.h>' seems to work >> in either case.. >> > The file is from the kernel UAPI, correct ? If so store it alongside > the other UAPI ones include/drm/ and import `make headers_install' > (see git log -- include/drm for examples). no, it copy/pastes a few lines from uabi/linux/sync_file.h just to avoid a dependency on kernel headers. I guess I *could* copy sync_file.h into libdrm, but it isn't really a drm uabi header, so I didn't want to do that BR, -R > We should add a README with the above specifics to include/drm/ one of > these days :-) > >>> >>>> +static inline int sync_wait(int fd, int timeout) >>>> +{ >>>> + struct pollfd fds; >>>> + >>>> + fds.fd = fd; >>>> + fds.events = POLLIN | POLLERR; >>> IIRC the API does not mention (forbid even) additional members of the >>> pollfd struct. >>> Let's zero init fds, and the compiler will drop it if/where applicable ? >> >> >> hmm, and I guess if this gets #include'd from c++ code, does it get >> more fun? I thought there were some different rules about struct >> initializers in c++? Or maybe that was just with certain compilers? >> > Initializers are fun in C++ indeed. Having the extern C wrappers (as > per your v2) helps. > > Thanks > Emil
On 31 October 2016 at 17:44, Rob Clark <robdclark@gmail.com> wrote: > On Mon, Oct 31, 2016 at 1:15 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> On 31 October 2016 at 16:39, Rob Clark <robdclark@gmail.com> wrote: >>> On Mon, Oct 31, 2016 at 11:25 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>>> On 31 October 2016 at 13:44, Rob Clark <robdclark@gmail.com> wrote: >>>>> From: Rob Clark <robclark@freedesktop.org> >>>>> >>>>> Rather than cut/pasting these couple ioctl wrappers everywhere, just >>>>> stuff them as static-inline into a header. >>>>> >>>>> Signed-off-by: Rob Clark <robclark@freedesktop.org> >>>>> --- >>>>> This is probably mostly used from mesa, but some drivers, test apps, etc >>>>> may also want to use it from libdrm. >>>>> >>>> It makes sense imho. To avoid fun experiences we want the header >>>> synced in an identical manner (make headers_install) to the >>>> include/drm ones. One might as well move it there, so make its "more" >>>> obvious. >>>> >>> >>> hmm, not sure I understand, but '#include <libsync.h>' seems to work >>> in either case.. >>> >> The file is from the kernel UAPI, correct ? If so store it alongside >> the other UAPI ones include/drm/ and import `make headers_install' >> (see git log -- include/drm for examples). > > no, it copy/pastes a few lines from uabi/linux/sync_file.h just to > avoid a dependency on kernel headers. I guess I *could* copy > sync_file.h into libdrm, but it isn't really a drm uabi header, so I > didn't want to do that > #include <linux/sync_file.h> and drop the copy/pasta ? The latter will come to bite us sooner or later. Just for information: memset should work everywhere. Everything else will produce a warning as you move through GCC versions. Thanks Emil
diff --git a/Makefile.sources b/Makefile.sources index a57036a..10aa1d0 100644 --- a/Makefile.sources +++ b/Makefile.sources @@ -13,6 +13,7 @@ LIBDRM_FILES := \ util_math.h LIBDRM_H_FILES := \ + libsync.h \ xf86drm.h \ xf86drmMode.h diff --git a/libsync.h b/libsync.h new file mode 100644 index 0000000..fc23b7f --- /dev/null +++ b/libsync.h @@ -0,0 +1,74 @@ +/* + * sync abstraction + * Copyright 2015-2016 Collabora Ltd. + * + * Based on the implementation from the Android Open Source Project, + * + * Copyright 2012 Google, Inc + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef _LIBSYNC_H +#define _LIBSYNC_H + +#include <stdint.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/poll.h> + +// todo prob should just #include <linux/sync_file.h> ? +struct sync_merge_data { + char name[32]; + int32_t fd2; + int32_t fence; + uint32_t flags; + uint32_t pad; +}; +#define SYNC_IOC_MAGIC '>' +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data) + + + +static inline int sync_wait(int fd, int timeout) +{ + struct pollfd fds; + + fds.fd = fd; + fds.events = POLLIN | POLLERR; + return poll(&fds, 1, timeout); +} + +static inline int sync_merge(const char *name, int fd1, int fd2) +{ + struct sync_merge_data data = {}; + int err; + + data.fd2 = fd2; + strncpy(data.name, name, sizeof(data.name) - 1); + data.name[sizeof(data.name) - 1] = '\0'; + + err = ioctl(fd1, SYNC_IOC_MERGE, &data); + if (err < 0) + return err; + + return data.fence; +} + +#endif