[Mesa-dev] [PATCH 21/40] pipe-loader: wire up the 'static' drm pipe-loader

[Emil Velikov]

On 22 October 2015 at 15:07, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 18.10.2015 00:57, Emil Velikov wrote:
>>
>> Add a list of driver descriptors and select one from the list, during
>> probe time.
>>
>> As we'll need to have all the driver pipe_foo_screen_create() functions
>> provided externally (i.e. from another static lib) we need a separate
>> (non-inline) drm_helper, which contains the function declarations.
>>
>> XXX: More than happy to rename things - header/functions/etc.
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>   src/gallium/auxiliary/pipe-loader/Makefile.am      |   6 +-
>>   .../auxiliary/pipe-loader/pipe_loader_drm.c        | 119
>> ++++++++++++++++++++-
>>   .../auxiliary/target-helpers/drm_helper_public.h   |  34 ++++++
>>   3 files changed, 154 insertions(+), 5 deletions(-)
>>   create mode 100644
>> src/gallium/auxiliary/target-helpers/drm_helper_public.h
>>
>> diff --git a/src/gallium/auxiliary/pipe-loader/Makefile.am
>> b/src/gallium/auxiliary/pipe-loader/Makefile.am
>> index 6a4a667..7db4190 100644
>> --- a/src/gallium/auxiliary/pipe-loader/Makefile.am
>> +++ b/src/gallium/auxiliary/pipe-loader/Makefile.am
>> @@ -34,12 +34,12 @@ AM_CFLAGS += \
>>   libpipe_loader_static_la_SOURCES += \
>>         $(DRM_SOURCES)
>>
>> -libpipe_loader_dynamic_la_SOURCES += \
>> -       $(DRM_SOURCES)
>> -
>>   libpipe_loader_static_la_LIBADD = \
>>         $(top_builddir)/src/loader/libloader.la
>>
>> +libpipe_loader_dynamic_la_SOURCES += \
>> +       $(DRM_SOURCES)
>> +
>>   libpipe_loader_dynamic_la_LIBADD = \
>>         $(top_builddir)/src/loader/libloader.la
>>
>> diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c
>> b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c
>> index 33274de..97e9dcb 100644
>> --- a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c
>> +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c
>> @@ -36,6 +36,7 @@
>>   #include <unistd.h>
>>
>>   #include "loader.h"
>> +#include "target-helpers/drm_helper_public.h"
>>   #include "state_tracker/drm_driver.h"
>>   #include "pipe_loader_priv.h"
>>
>> @@ -51,7 +52,9 @@
>>   struct pipe_loader_drm_device {
>>      struct pipe_loader_device base;
>>      const struct drm_driver_descriptor *dd;
>> +#ifndef GALLIUM_STATIC_TARGETS
>>      struct util_dl_library *lib;
>> +#endif
>>      int fd;
>>   };
>>
>> @@ -59,6 +62,103 @@ struct pipe_loader_drm_device {
>>
>>   static const struct pipe_loader_ops pipe_loader_drm_ops;
>>
>> +#ifdef GALLIUM_STATIC_TARGETS
>> +static const struct drm_conf_ret throttle_ret = {
>> +   DRM_CONF_INT,
>> +   {2},
>> +};
>> +
>> +static const struct drm_conf_ret share_fd_ret = {
>> +   DRM_CONF_BOOL,
>> +   {true},
>> +};
>> +
>> +static inline const struct drm_conf_ret *
>> +configuration_query(enum drm_conf conf)
>> +{
>> +   switch (conf) {
>> +   case DRM_CONF_THROTTLE:
>> +      return &throttle_ret;
>> +   case DRM_CONF_SHARE_FD:
>> +      return &share_fd_ret;
>> +   default:
>> +      break;
>> +   }
>> +   return NULL;
>> +}
>> +
>> +static const struct drm_driver_descriptor driver_descriptors[] = {
>> +    {
>> +        .name = "i915",
>> +        .driver_name = "i915",
>> +        .create_screen = pipe_i915_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "i965",
>> +        .driver_name = "i915",
>> +        .create_screen = pipe_ilo_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "nouveau",
>> +        .driver_name = "nouveau",
>> +        .create_screen = pipe_nouveau_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "r300",
>> +        .driver_name = "radeon",
>> +        .create_screen = pipe_r300_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "r600",
>> +        .driver_name = "radeon",
>> +        .create_screen = pipe_r600_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "radeonsi",
>> +        .driver_name = "radeon",
>> +        .create_screen = pipe_radeonsi_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "vmwgfx",
>> +        .driver_name = "vmwgfx",
>> +        .create_screen = pipe_vmwgfx_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "kgsl",
>> +        .driver_name = "freedreno",
>> +        .create_screen = pipe_freedreno_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "msm",
>> +        .driver_name = "freedreno",
>> +        .create_screen = pipe_freedreno_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>> +    {
>> +        .name = "vc4",
>> +        .driver_name = "vc4",
>> +        .create_screen = pipe_vc4_create_screen,
>> +        .configuration = configuration_query,
>> +    },
>
>
> I believe these should be guarded by the respective #if
> defined(GALLIUM_xxx).
>
> I see that in patch 25 (target-helpers: add a non-inline drm_helper.h) you
> change the pipe_XXX_create_screen functions so that they return NULL if the
> corresponding driver has not been configured.
>
I'm afraid that adding the GALLIUM_$DRIVER define guards is not what
we want here. The idea being that the pipe-loader is a component
linked alongside the state-tracker, thus all the pipe_*_create_screen
should be available, with the appropriate implementation for each
function dependant on the target.

For example: pipe-loader should always list pipe_r300_create_screen
symbol as 'requirement', which will be resolved as { return NULL } for
OMX or { radeon_specifics } for DRI.

> However, using #if guards here instead is bound to provide a clearer
> distinction between the "create_screen failed" and "driver missing" failure
> modes.
>
If you want I can add the fprintf(stderr, "foo: driver missing\n") in
the dummy (return NULL) cases for patch 25 ?

Thanks for having a look Nicolai and welcome back to mesa :-)
-Emil