[Mesa-dev] [PATCH] gbm: add support for loading third-party backend

[Nicolai Hähnle]

On 24.01.2017 04:36, Qiang Yu wrote:
> Third-party can put their backend to a directory configured with
> '--with-gbm-backenddir' and create a /etc/gbm.conf.d/*.conf file
> which contains the backend so file name to overwrite the default
> builtin DRI backend.
>
> The /etc/gbm.conf.d/*.conf will be sorted and the backends added
> will be tried one-by-one until one can successfully create a gbm
> device. The default DRI backend is tried at last.
>
> People can still use GBM_BACKEND to overwrite the backend try
> order.

This is a very good idea. I mostly like the code, but I think some 
high-level points should be addressed immediately to avoid problems 
later on.


ABI Compatibility
=================
This change necessarily makes all (most?) of gbmint.h into a public 
binary interface. This raises two questions:

1. Does all of gbmint.h really have to be in the public interface? If 
not, it would be good to make the split clear.

2. There needs to be a way for the GBM backend to declare the ABI 
version it supports. The simplest way would be to add an int abi_version 
as the first field of struct gbm_backend, and have load_backend check it.


Config Files
============
Right now, the config files seem like a bit of a pointless indirection. 
After all, we could just put symlinks directly into /etc/gbm.conf.d/.

On the other hand, we might want to follow the idea of Vulkan ICD JSON 
files, at least in concept. By putting additional data into the config 
file, we could skip loading a backend if the config file already 
indicates that it has the wrong ABI version, for example.

I don't know how valuable that really is, and would like to hear other 
opinions. I also don't think we should over-complicate things, so I 
think either of the following two options are fine:

a) Skip config files altogether, and just expect people to put symlinks 
into /etc/gbm.conf.d/.

b) Keep the config files, but for future extensibility, use strncmp to 
only look at lines of the form:

lib: <full path here>

Then we have the freedom to add lines like "abi_version: NNN" later. I 
lean slightly towards (b).

Related, I think GBM_BACKEND_DIR and the corresponding configure option 
are not needed. We can just require the config files to contain a full 
path. This would allow e.g. distribution-provided backends in 
/usr/lib/gbm, while amdgpu-pro would install itself in /opt/amdgpu/lib/gbm.

If anything, people might want to configure the CONFIG_DIR, but in the 
interest of supportability, I don't think we should let them ;-)


Some comments on the code itself:

[snip]
> @@ -57,17 +67,28 @@ load_backend(const struct backend_desc *backend)
>     if (backend->builtin) {
>        init = backend->builtin;
>     }
> +   else {
> +      char path[PATH_MAX];
> +      void *module;
> +
> +      snprintf(path, PATH_MAX, "%s/%s", GBM_BACKEND_DIR, backend->name);
> +      module = dlopen(path, RTLD_NOW | RTLD_GLOBAL);
> +      if (module) {
> +         backend->builtin = dlsym(module, "gbm_backend");
> +         init = backend->builtin;
> +      }
> +   }

load_backend might be called multiple times for the same backend 
(consider the case where the dlsym lookup fails). I think it would be 
good to store the module pointer in the backend_desc, and skip this code 
when the module has already been loaded.


>
>     return init;
>  }
>
> -static const struct backend_desc *
> +static struct backend_desc *
>  find_backend(const char *name)
>  {
> -   const struct backend_desc *backend = NULL;
> +   struct backend_desc *backend = NULL;
>     unsigned i;
>
> -   for (i = 0; i < ARRAY_SIZE(backends); ++i) {
> +   for (i = 0; i < num_backends; ++i) {
>        if (strcmp(backends[i].name, name) == 0) {
>           backend = &backends[i];
>           break;
> @@ -77,6 +98,50 @@ find_backend(const char *name)
>     return backend;
>  }
>
> +static int
> +scandir_filter(const struct dirent *ent)
> +{
> +    if (ent->d_type != DT_REG && ent->d_type != DT_LNK &&
> +        ent->d_type != DT_UNKNOWN)
> +       return 0;
> +
> +    if (fnmatch("*.conf", ent->d_name, 0))
> +       return 0;
> +
> +    return 1;
> +}
> +
> +static void
> +init_backends(void)
> +{
> +   int i, count;
> +   struct dirent **entries = NULL;
> +
> +   count = scandir(CONFIG_DIR, &entries, scandir_filter, alphasort);
> +   for (i = 0; i < count; i++) {
> +      char path[PATH_MAX];
> +      FILE *file;
> +
> +      snprintf(path, PATH_MAX, "%s/%s", CONFIG_DIR, entries[i]->d_name);
> +      if ((file = fopen(path, "r"))) {
> +         while (fgets(path, PATH_MAX, file)) {
> +            int n = strlen(path);
> +            if (path[n - 1] == '\n')
> +               path[n - 1] = '\0';
> +            if (!fnmatch("*.so", path, 0) &&
> +                num_backends < MAX_BACKENDS - ARRAY_SIZE(builtin_backends)) {
> +               backends[num_backends].name = strdup(path);
> +               backends[num_backends++].builtin = NULL;

Please put the num_backends++ in a line of its own.


> +            }
> +         }
> +         fclose(file);
> +      }
> +   }
> +
> +   memcpy(backends + num_backends, builtin_backends, sizeof(builtin_backends));
> +   num_backends += ARRAY_SIZE(builtin_backends);
> +}
> +
>  struct gbm_device *
>  _gbm_create_device(int fd)
>  {
> @@ -85,6 +150,9 @@ _gbm_create_device(int fd)
>     unsigned i;
>     const char *b;
>
> +   if (!num_backends)
> +      init_backends();
> +

This isn't thread-safe. I think you can just use pthread_once, see the 
example at 
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_once.html

Thanks,
Nicolai


>     b = getenv("GBM_BACKEND");
>     if (b)
>        backend = load_backend(find_backend(b));
> @@ -92,7 +160,7 @@ _gbm_create_device(int fd)
>     if (backend)
>        dev = backend->create_device(fd);
>
> -   for (i = 0; i < ARRAY_SIZE(backends) && dev == NULL; ++i) {
> +   for (i = 0; i < num_backends && dev == NULL; ++i) {
>        backend = load_backend(&backends[i]);
>        if (backend == NULL)
>           continue;
>