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

[Yu, Qiang]

Thanks Nicolai, I'll address the gbmint.h and ABI version next version.

Regards,
Qiang
________________________________________
From: Nicolai Hähnle <nhaehnle at gmail.com>
Sent: Tuesday, January 24, 2017 6:12:06 PM
To: Yu, Qiang; mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH] gbm: add support for loading third-party backend

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;
>