[Mesa-dev] [PATCH] st/dri: Add fence extension to SW path

[Chad Versace]

When you resubmit the patch again, please add (v3) to the subject.

On Mon 08 May 2017, Gurchetan Singh wrote:
> Use the same fence implementation for drisw.c as dri2.c by making
> dri2FenceExtension an external variable. Since the fence implementation
> is not dri2.c specific, put it in a separate file. This is desirable for
> synchronization in virtual machines.
> 
> v2: Don't depend on dri2.c for extensions (Emil)
> ---
>  src/gallium/state_trackers/dri/Makefile.sources |   2 +
>  src/gallium/state_trackers/dri/dri2.c           | 203 +--------------------
>  src/gallium/state_trackers/dri/dri_extensions.c | 229 ++++++++++++++++++++++++
>  src/gallium/state_trackers/dri/dri_extensions.h |  30 ++++
>  src/gallium/state_trackers/dri/drisw.c          |   2 +
>  5 files changed, 264 insertions(+), 202 deletions(-)
>  create mode 100644 src/gallium/state_trackers/dri/dri_extensions.c
>  create mode 100644 src/gallium/state_trackers/dri/dri_extensions.h


Usually, to make review easier, and to make the git log easier to
understand, it's better to break patches like this into two: patch
1 moves the code, and patch 2 makes the actual changes.

> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> index ed6004f836..2d95e668f8 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c


> -static bool
> -dri2_is_opencl_interop_loaded_locked(struct dri_screen *screen)
> -{
> -   return screen->opencl_dri_event_add_ref &&
> -          screen->opencl_dri_event_release &&
> -          screen->opencl_dri_event_wait &&
> -          screen->opencl_dri_event_get_fence;
> -}
> -
> -static bool
> -dri2_load_opencl_interop(struct dri_screen *screen)
> -{
> -#if defined(RTLD_DEFAULT)
> -   bool success;

There's a bug here.  Pre-patch, RTLD_DEFAULT is defined here and this
code gets built.  I verified this with #error. Post-patch, in the
equivalent code in the new file, RTLD_DEFAULT is undefined, and this
code never gets built.

> -
> -   mtx_lock(&screen->opencl_func_mutex);
> -
> -   if (dri2_is_opencl_interop_loaded_locked(screen)) {
> -      mtx_unlock(&screen->opencl_func_mutex);
> -      return true;
> -   }
> -
> -   screen->opencl_dri_event_add_ref =
> -      dlsym(RTLD_DEFAULT, "opencl_dri_event_add_ref");
> -   screen->opencl_dri_event_release =
> -      dlsym(RTLD_DEFAULT, "opencl_dri_event_release");
> -   screen->opencl_dri_event_wait =
> -      dlsym(RTLD_DEFAULT, "opencl_dri_event_wait");
> -   screen->opencl_dri_event_get_fence =
> -      dlsym(RTLD_DEFAULT, "opencl_dri_event_get_fence");
> -
> -   success = dri2_is_opencl_interop_loaded_locked(screen);
> -   mtx_unlock(&screen->opencl_func_mutex);
> -   return success;
> -#else
> -   return false;
> -#endif
> -}


> diff --git a/src/gallium/state_trackers/dri/dri_extensions.c b/src/gallium/state_trackers/dri/dri_extensions.c
> new file mode 100644
> index 0000000000..2fa7233aab
> --- /dev/null
> +++ b/src/gallium/state_trackers/dri/dri_extensions.c


> +static bool
> +dri2_is_opencl_interop_loaded_locked(struct dri_screen *screen)
> +{
> +   return screen->opencl_dri_event_add_ref &&
> +          screen->opencl_dri_event_release &&
> +          screen->opencl_dri_event_wait &&
> +          screen->opencl_dri_event_get_fence;
> +}
> +
> +static bool
> +dri2_load_opencl_interop(struct dri_screen *screen)
> +{
> +#if defined(RTLD_DEFAULT)
> +   bool success;
> +
> +   pipe_mutex_lock(screen->opencl_func_mutex);

pipe_mutex_lock/unlock need to be replaced with mtx_lock/unlock, because
the pipe_mutex wrappers no longer exist. See this commit.

    commit e000b17f87bd960c4ce1c0892017023d4dc59609
    Author: Brian Paul <brianp at vmware.com>
    Date:   Wed Apr 5 15:15:27 2017 -0600

        winsys/svga: use c11 thread types/functions

        Gallium no longer has wrappers for mutexes and conditi

> +
> +   if (dri2_is_opencl_interop_loaded_locked(screen)) {
> +      pipe_mutex_unlock(screen->opencl_func_mutex);
> +      return true;
> +   }
> +
> +   screen->opencl_dri_event_add_ref =
> +      dlsym(RTLD_DEFAULT, "opencl_dri_event_add_ref");
> +   screen->opencl_dri_event_release =
> +      dlsym(RTLD_DEFAULT, "opencl_dri_event_release");
> +   screen->opencl_dri_event_wait =
> +      dlsym(RTLD_DEFAULT, "opencl_dri_event_wait");
> +   screen->opencl_dri_event_get_fence =
> +      dlsym(RTLD_DEFAULT, "opencl_dri_event_get_fence");
> +
> +   success = dri2_is_opencl_interop_loaded_locked(screen);
> +   pipe_mutex_unlock(screen->opencl_func_mutex);
> +   return success;
> +#else
> +   return false;
> +#endif
> +}

Those are the only issues I found. But, I don't know this code well, so
you should probably seek the review of someone who touched it recently,
like Rob Herring, Rob Clark, or Tim Arceri.