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

[Emil Velikov]

On 22 October 2015 at 16:32, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.
>
Or on other words - as is you'll build a static/dynamic combo once.
Following your suggestion one will need to build one pair for each
state-tracker/target.

And yes, it's not that too obvious of a design as is. Suggestions on
the commit message wording and/or documentation are greatly
appreciated.

-Emil