[Mesa-dev] [Mesa-stable] [PATCH 2/2] egl: move gbm link-time dependency to runtime dependency

[Emil Velikov]

On 12 November 2017 at 07:15, sguttula <suresh.guttula at amd.com> wrote:
> This patch will fix build error when we build with_platforms=drm
> and without enabling gbm.
>
> Chromiumos uses surfaceless platform and minigbm[not mesa-gbm]
> and drm platform needed for va.When we enable drm platform ,
> it is failing for dependency on gbm[as we disable mesa-gbm]
> at compile time.To fix this moved link dependency to runtime.
>
> Cc: mesa-stable at lists.freedesktop.org
> Cc: Emil Velikov <emil.velikov at collabora.com>
> Cc: Deepak Sharma <deepak.sharma at amd.com>
>
> Signed-off-by: sguttula <suresh.guttula at amd.com>
> Reviewed-by: Deepak Sharma <deepak.sharma at amd.com>
> ---
>  src/egl/Makefile.am                 |   2 +-
>  src/egl/drivers/dri2/platform_drm.c | 103 +++++++++++++++++++++++++++++++-----
>  src/egl/main/egldisplay.c           |   5 +-
>  src/gbm/main/gbm.c                  |  11 ----
>  4 files changed, 95 insertions(+), 26 deletions(-)
>  mode change 100644 => 100755 src/egl/Makefile.am
>  mode change 100644 => 100755 src/egl/drivers/dri2/platform_drm.c
>  mode change 100644 => 100755 src/egl/main/egldisplay.c
>  mode change 100644 => 100755 src/gbm/main/gbm.c
>
Hmm are you developing a Linux driver on Windows? These mode changes
should not be here.

In a TL;DR: I'd suggest:
 - picking something like my series [1] as a base
 - doing the dladdr trick as 1/X, updating gbm_device::dummy and
related documentation (suggest making it an ABI check)
 - 2/X - add separate file which provides wrappers - add small prefix
say "foo_gbm...", call "dlsym_all" from dri2_initialize_drm,
"close_the_handle at teardown
 - 3/X update existing codebase to use ^^, drop linking
 - ...
 - profit

There are a few small suggestions below.

[1] https://patchwork.freedesktop.org/series/33716/
Reviews will be appreciated


> diff --git a/src/egl/Makefile.am b/src/egl/Makefile.am
> old mode 100644
> new mode 100755
> index eeb745f..3bad977
> --- a/src/egl/Makefile.am
> +++ b/src/egl/Makefile.am
> @@ -94,7 +94,7 @@ dri2_backend_FILES += \
>  endif
>
>  if HAVE_PLATFORM_DRM
> -libEGL_common_la_LIBADD += $(top_builddir)/src/gbm/libgbm.la
> +
There is a similar hunk in src/egl/meson.build. You want to drop that
one as well.
Namely:
link_for_egl += libgbm


>  dri2_backend_FILES += drivers/dri2/platform_drm.c
>  endif
>
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> old mode 100644
> new mode 100755
> index 9005f5d..f489881
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -39,7 +39,19 @@
>  #include "egl_dri2.h"
>  #include "egl_dri2_fallbacks.h"
>  #include "loader.h"
> +static void* gbm_handle =NULL;
>
Nit: Please follow surrounding coding style - space on each side of =

> +/** Destroy the gbm device and free all resources associated with it.
> + *
> + * \param gbm The device created using gbm_create_device()
> + */
> +GBM_EXPORT void
> +gbm_device_destroy(struct gbm_device *gbm)
> +{
> +   gbm->refcount--;
> +   if (gbm->refcount == 0)
> +      gbm->destroy(gbm);
> +}
This is a _very_ bad idea. You're breaking GBM instead of moving the
call from egl_dri2.c here.
See my series for an example.


>  static struct gbm_bo *
>  lock_front_buffer(struct gbm_surface *_surf)
>  {
> @@ -169,11 +181,18 @@ dri2_drm_destroy_surface(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
>
> +   void (* gbm_fp)();
>     dri2_dpy->core->destroyDrawable(dri2_surf->dri_drawable);
>
>     for (unsigned i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> -      if (dri2_surf->color_buffers[i].bo)
> -        gbm_bo_destroy(dri2_surf->color_buffers[i].bo);
> +      if (dri2_surf->color_buffers[i].bo){
> +         gbm_fp = dlsym(gbm_handle,"gbm_bo_destroy");
This will (and below) lead to a lot of unnecessary dlsym calls. You
can do it once, as mentioned above.


> @@ -660,6 +713,15 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>
>     dri2_dpy->fd = -1;
>     disp->DriverData = (void *) dri2_dpy;
> +#if defined(__x86_64__)
> +   gbm_handle = dlopen("/usr/lib64/libgbm.so.1", RTLD_LAZY);
> +#else
> +   gbm_handle = dlopen("/usr/lib/libgbm.so.1", RTLD_LAZY);
Why are you hard coding the path? Please don't use LAZY -> LOCAL | NOW
is a wiser move.


> @@ -124,7 +126,8 @@ _eglNativePlatformDetectNativeDisplay(void *nativeDisplay)
>
>  #ifdef HAVE_DRM_PLATFORM
>        /* gbm has a pointer to its constructor as first element. */
Nit: Comment should be fixed up.

Thanks
Emil