[FFmpeg-devel] [PATCH] lavu/opencl: ticket #2422 opencl using dynamic mutex

[James Almer]

On 28/06/13 7:17 AM, Wei Gao wrote:
> Hi
> 
> The attachment is about the opencl using dynamic mutex, I have tested on my
> comoputer in pthread, I can't compile a win32 version, can anyone help to
> test?
> 
> Thanks
> Best regards

> diff --git a/libavutil/opencl.c b/libavutil/opencl.c
> index 5887b50..e5de2a7 100644
> --- a/libavutil/opencl.c
> +++ b/libavutil/opencl.c
> @@ -25,18 +25,24 @@
>  #include "log.h"
>  #include "avassert.h"
>  #include "opt.h"
> +#include "atomic.h"
>  
>  #if HAVE_PTHREADS
> -
>  #include <pthread.h>
> -static pthread_mutex_t atomic_opencl_lock = PTHREAD_MUTEX_INITIALIZER;
> +#elif HAVE_W32THREADS
> +#include "compat/w32pthreads.h"
> +#elif HAVE_OS2THREADS
> +#include "compat/os2threads.h"
> +#endif

I don't think this is necessary as it was established that OpenCL is not available on OS2.


>  
> -#define LOCK_OPENCL pthread_mutex_lock(&atomic_opencl_lock)
> -#define UNLOCK_OPENCL pthread_mutex_unlock(&atomic_opencl_lock)
>  
> -#elif !HAVE_THREADS
> +#if !HAVE_THREADS
>  #define LOCK_OPENCL
>  #define UNLOCK_OPENCL
> +#else
> +static pthread_mutex_t atomic_opencl_lock = NULL;

Needs to be a pointer.
The idea is that it will point to the first mutex that gets allocated and initialized in 
init_opencl_mutex() below.


> +#define LOCK_OPENCL pthread_mutex_lock(&atomic_opencl_lock)
> +#define UNLOCK_OPENCL pthread_mutex_unlock(&atomic_opencl_lock)
>  #endif

Thus no need for & here.


> @@ -62,6 +68,7 @@ typedef struct {
>      int platform_idx;
>      int device_idx;
>      char *build_options;
> +    pthread_mutex_t *opencl_mutex;

This is not necessary. atomic_opencl_lock is enough.


> +static int init_opencl_mutex(void)
> +{
> +#if HAVE_THREADS
> +    pthread_mutex_t *opencl_mutex;
> +    opencl_mutex = av_mallocz(sizeof(pthread_mutex_t));
> +    if (!opencl_mutex) {
> +        return AVERROR(ENOMEM);
> +    }
> +    pthread_mutex_init(opencl_mutex, NULL);
> +    if (avpriv_atomic_ptr_cas((void * volatile *)&atomic_opencl_lock, NULL, opencl_mutex)) {

This line is ok, assuming atomic_opencl_lock is a pointer.


> +    if (opencl_mutex)
> +        opencl_ctx.opencl_mutex = opencl_mutex;

No need for this as i mentioned earlier.


> +    return 0;
> +#else
> +    return 0;
> +#endif

You can simplify this into
#endif
    return 0;

	
> +static void uninit_opencl_mutex(void)
> +{
> +#if HAVE_THREADS
> +    if (atomic_opencl_lock) {
> +        pthread_mutex_destroy(&atomic_opencl_lock);

No need for & here either.


> +        av_freep(&opencl_ctx.opencl_mutex);
> +    }
> +#endif

av_freep(&atomic_opencl_lock);


> @@ -588,6 +630,10 @@ static int compile_kernel_file(OpenclContext *opencl_ctx)
>  int av_opencl_init(AVOpenCLExternalEnv *ext_opencl_env)
>  {
>      int ret = 0;
> +    //check whether the mutex has been inited

Initialized.