[FFmpeg-devel] Patch: Enable OpenCL with Win32 threads
Michael Niedermayer
michaelni at gmx.at
Fri May 2 06:11:20 CEST 2014
On Fri, May 02, 2014 at 12:58:29PM +1000, Matt Oliver wrote:
> On 2 May 2014 04:27, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> > On Thu, May 01, 2014 at 07:23:15PM +1000, Matt Oliver wrote:
> > > On 18 April 2014 11:25, Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > > > On Thu, Apr 17, 2014 at 01:46:36PM +1000, Matt Oliver wrote:
> > > > > >
> > > > > > Could this just be integrated in the pthread_mutex_lock win32
> > wrapper
> > > > > > code to emulate PTHREAD_MUTEX_INITIALIZER?
> > > > >
> > > > >
> > > > > It probably shouldn't as it adds extra checks that are not necessary
> > if
> > > > you
> > > > > did the appropriate _init calls anyway (also I did it wrong ;) ).
> > Win32
> > > > > already does an internal check on the -1 waiters value which is what
> > I
> > > > used
> > > > > in the first patch that added a static initialisation for win32.
> > > > >
> > > > >
> > > > > > this is not thread safe at all
> > > > > >
> > > > > > thread #1 avpriv_atomic_cas
> > > > > > thread #2 avpriv_atomic_cas (if skiped)
> > LOCK_OPENCL
> > > > use
> > > > > > of uninitialized mutex
> > > > > >
> > > > > > please see the existing code, that sets up global mutexes
> > > > > > in the lock manager for example
> > > > > >
> > > > >
> > > > > This is one of those things were I knew that and knew better but for
> > some
> > > > > reason did it anyway (good reason not to submit patches late at
> > night).
> > > > It
> > > > > needed a second atomic and wait to work properly. Anyway attached is
> > a
> > > > > patch that initialises in the same way the existing lock manager does
> > > > which
> > > > > should be more acceptable and consistent with existing code. Its a
> > little
> > > > > more complicated than static initialisation but if the first static
> > patch
> > > > > with the win32 static initializer isnt suitable then this seems the
> > most
> > > > > consistent way to support all native thread implementations.
> > > >
> > > > > opencl.c | 63
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
> > > > > opencl.h | 1 +
> > > > > 2 files changed, 51 insertions(+), 13 deletions(-)
> > > > > 1b908cff3181f07226e68c0ed2fa647517dcfae7
> > > > opencl-add-support-for-non-pthread-locking.patch
> > > > > From 7644090cf14982a700859b6ae23724f3a1800ace Mon Sep 17 00:00:00
> > 2001
> > > > > From: Matt Oliver <protogonoi at gmail.com>
> > > > > Date: Thu, 17 Apr 2014 13:21:48 +1000
> > > > > Subject: [PATCH] opencl: add support for non-pthread locking
> > > > >
> > > > > ---
> > > > > libavutil/opencl.c | 63
> > > > +++++++++++++++++++++++++++++++++++++++++++-----------
> > > > > libavutil/opencl.h | 1 +
> > > > > 2 files changed, 51 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/libavutil/opencl.c b/libavutil/opencl.c
> > > > > index 142c6b0..5a3b85f 100644
> > > > > --- a/libavutil/opencl.c
> > > > > +++ b/libavutil/opencl.c
> > > > > @@ -27,15 +27,21 @@
> > > > > #include "avassert.h"
> > > > > #include "opt.h"
> > > > >
> > > > > +#if HAVE_THREADS
> > > > > #if HAVE_PTHREADS
> > > > > -
> > > > > #include <pthread.h>
> > > > > -static pthread_mutex_t atomic_opencl_lock =
> > PTHREAD_MUTEX_INITIALIZER;
> > > > > -
> > > > > -#define LOCK_OPENCL pthread_mutex_lock(&atomic_opencl_lock)
> > > > > -#define UNLOCK_OPENCL pthread_mutex_unlock(&atomic_opencl_lock)
> > > > > +#elif HAVE_W32THREADS
> > > > > +#include "compat/w32pthreads.h"
> > > > > +#elif HAVE_OS2THREADS
> > > > > +#include "compat/os2threads.h"
> > > > > +#endif
> > > > > +#include "atomic.h"
> > > > >
> > > > > -#elif !HAVE_THREADS
> > > > > +#define MUTEX_OPENCL pthread_mutex_t *atomic_opencl_lock;
> > > > > +#define LOCK_OPENCL
> > pthread_mutex_lock(opencl_ctx.atomic_opencl_lock)
> > > > > +#define UNLOCK_OPENCL
> > > > pthread_mutex_unlock(opencl_ctx.atomic_opencl_lock)
> > > > > +#else
> > > > > +#define MUTEX_OPENCL
> > > > > #define LOCK_OPENCL
> > > > > #define UNLOCK_OPENCL
> > > > > #endif
> > > > > @@ -74,6 +80,7 @@ typedef struct {
> > > > > int kernel_code_count;
> > > > > KernelCode kernel_code[MAX_KERNEL_CODE_NUM];
> > > > > AVOpenCLDeviceList device_list;
> > > > > + MUTEX_OPENCL
> > > > > } OpenclContext;
> > > > >
> > > > > #define OFFSET(x) offsetof(OpenclContext, x)
> > > > > @@ -321,14 +328,44 @@ void
> > av_opencl_free_device_list(AVOpenCLDeviceList
> > > > **device_list)
> > > > > av_freep(device_list);
> > > > > }
> > > > >
> > > > > -int av_opencl_set_option(const char *key, const char *val)
> > > > > +inline int init_opencl_ctx(void)
> > > > > {
> > > > > - int ret = 0;
> > > > > - LOCK_OPENCL;
> > > > > +#if HAVE_THREADS
> > > >
> > > > > + if (!opencl_ctx.atomic_opencl_lock) {
> > > >
> > > > Thread #1 if(1), Thread #2 if(1)
> > > >
> > > > > + pthread_mutex_t *tmp = av_malloc(sizeof(pthread_mutex_t));
> > > > > + int err;
> > > > > + if (!tmp)
> > > > > + return AVERROR(ENOMEM);
> > > > > + if ((err = pthread_mutex_init(tmp, NULL))) {
> > > > > + av_free(tmp);
> > > > > + return AVERROR(err);
> > > > > + }
> > > >
> > > > > + if (!avpriv_atomic_ptr_cas(opencl_ctx.atomic_opencl_lock,
> > NULL,
> > > > tmp)) {
> > > >
> > > > Thread #1 if(1), Thread #2 if(0)
> > > > Thread #1 stops here, by choice of the scheduler
> > > >
> > > > > + LOCK_OPENCL;
> > > > > + av_opt_set_defaults(&opencl_ctx);
> > > > > + opencl_ctx.opt_init_flag = 1;
> > > > > + UNLOCK_OPENCL;
> > > > > + }
> > > > > + else {
> > > > > + pthread_mutex_destroy(tmp);
> > > > > + av_free(tmp);
> > > > > + }
> > > >
> > > > Thread #2 continues on without av_opt_set_defaults() having been
> > > > executed
> > > >
> > > >
> > > > [...]
> > > >
> > > > --
> > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > > >
> > > > I have never wished to cater to the crowd; for what I know they do not
> > > > approve, and what they approve I do not know. -- Epicurus
> > > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel at ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > >
> > > Well this would be pretty rare as any code that calls the init_ctx
> > function
> > > immediately follows it with a mutex lock. So the scheduler would have to
> > > pause thread #1 exactly after the atomic function and before the
> > following
> > > Lock statement for there to be a problem. But you are correct in that it
> > is
> > > possible so attached is a patch that rectifies this. As a result the
> > > changes are far less and this patch just adds the function to detect that
> > > the mutex has been initialised. Constantly checking is a little ugly but
> > > short of static init or a shared global function for all mutex
> > > initialisation then this is the only way. The checks are only done when
> > > getting set options or when actually initialising opencl which is only
> > > really done once so its not actually that much of an issue.
> > >
> > > Matt
> >
> > > opencl.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
> > > opencl.h | 1 +
> > > 2 files changed, 42 insertions(+), 9 deletions(-)
> > > 99edce8779dbdcc1ec884c54a0e96123bf576f10
> > opencl-add-support-for-non-pthread-locking.patch
> > > From db36cb0349527abcab76cb589008b19f53c7ea1f Mon Sep 17 00:00:00 2001
> > > From: Matt Oliver <protogonoi at gmail.com>
> > > Date: Thu, 1 May 2014 19:21:29 +1000
> > > Subject: [PATCH] opencl: add support for non-pthread locking
> >
> > applied
> > thx
> >
> > [...]
> >
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker.
> > User
> > questions about the command line tools should be sent to the ffmpeg-user
> > ML.
> > And questions about how to use libav* should be sent to the libav-user ML.
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> Heres the patch to enable it in configure for other thread implementations.
> configure | 2 --
> 1 file changed, 2 deletions(-)
> 81becfc7006d7c143610aefa116148ee9ed32cb3 Enable-opencl-wihtout-pthreads.patch
> From 8f993e666b13cf08fdc54dbda7131fc7076e000d Mon Sep 17 00:00:00 2001
> From: Matt Oliver <protogonoi at gmail.com>
> Date: Fri, 2 May 2014 12:49:23 +1000
> Subject: [PATCH] Enable opencl wihtout pthreads.
applied
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140502/fc1abccc/attachment.asc>
More information about the ffmpeg-devel
mailing list