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

[Nicolai Hähnle]

On 22.10.2015 17:32, Emil Velikov wrote:

On 22 October 2015 at 15:07, Nicolai Hähnle <nhaeh...@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.veli...@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.

Oh I see now, thanks for the explanation.

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 ?

That works well for me, yes.

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

Thanks :)

Cheers,
Nicolai
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev