On Wed, Oct 30, 2013 at 08:15:43AM +0100, David Herrmann wrote: > Hi Tom > > On Tue, Oct 29, 2013 at 9:00 PM, Tom Stellard <t...@stellard.net> wrote: > > From: Tom Stellard <thomas.stell...@amd.com> > > > > You can use the --enable-pipe-loader-render-nodes configure flag to > > make the pipe-loader use render nodes for talking with the device. > > --- > > configure.ac | 6 ++ > > src/gallium/auxiliary/pipe-loader/Makefile.am | 5 ++ > > .../auxiliary/pipe-loader/pipe_loader_drm.c | 80 > > ++++++++++++++++++++-- > > src/gallium/winsys/radeon/drm/Makefile.am | 4 ++ > > 4 files changed, 91 insertions(+), 4 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > index 91b9871..19c4b1b 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -599,6 +599,11 @@ AC_ARG_ENABLE([opencl_icd], > > @<:@default=no@:>@])], > > [enable_opencl_icd="$enableval"], > > [enable_opencl_icd=no]) > > +AC_ARG_ENABLE([pipe-loader-render-nodes], > > + [AS_HELP_STRING([--enable-pipe-loader-render-nodes], > > + [Enable the pipe-loader to use render nodes to interact with > > devices. @<:@default=no@:>@])], > > + [enable_pipe_loader_render_nodes="$enableval"], > > + [enable_pipe_loader_render_nodes=no]) > > Why exactly make this optional? If there's a render-node, try using > it, otherwise fall back to the legacy node. I'm not really convinced > making this a compile-time option gives us anything. >
I wanted to have this option to make it easier for testers to test with and without render-nodes. I wasn't sure whether to enable it by default and have an option to disable it or disable it by default and have an option to enable it. I went with the conservative approach of disabling by default to minimize the impact, but I wouldn't mind enabling it by default it other developers think this is OK. > > AC_ARG_ENABLE([xlib-glx], > > [AS_HELP_STRING([--enable-xlib-glx], > > [make GLX library Xlib-based instead of DRI-based > > @<:@default=disabled@:>@])], > > @@ -1376,6 +1381,7 @@ if test "x$enable_opencl" = xyes; then > > fi > > AM_CONDITIONAL(HAVE_CLOVER, test "x$enable_opencl" = xyes) > > AM_CONDITIONAL(HAVE_CLOVER_ICD, test "x$enable_opencl_icd" = xyes) > > +AM_CONDITIONAL(HAVE_PIPE_LOADER_RENDER_NODES, test > > "x$enable_pipe_loader_render_nodes" = xyes) > > AC_SUBST([OPENCL_LIBNAME]) > > > > dnl > > diff --git a/src/gallium/auxiliary/pipe-loader/Makefile.am > > b/src/gallium/auxiliary/pipe-loader/Makefile.am > > index 9a8094f..a32b519 100644 > > --- a/src/gallium/auxiliary/pipe-loader/Makefile.am > > +++ b/src/gallium/auxiliary/pipe-loader/Makefile.am > > @@ -23,3 +23,8 @@ libpipe_loader_la_SOURCES += pipe_loader_drm.c > > AM_CFLAGS = $(LIBDRM_CFLAGS) > > endif > > endif > > + > > +if HAVE_PIPE_LOADER_RENDER_NODES > > +AM_CPPFLAGS+= -DUSE_RENDER_NODES > > +endif > > Your 1/2 patch depends on this, doesn't it? Just trying to understand > where the first patch gets USE_RENDER_NODES from. > Patch 1 will compile fine and is actually a no-op without the USE_RENDER_NODES definition. I wanted to keep the two patches separate, so I added the work around in patch 1, but didn't actually activate it until patch 2. > > + > > diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c > > b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c > > index 339d7bf..79b7237 100644 > > --- a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c > > +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c > > @@ -51,6 +51,11 @@ > > #define DRIVER_MAP_GALLIUM_ONLY > > #include "pci_ids/pci_id_driver_map.h" > > > > +#define DRM_RENDER_NODE_DEV_NAME "%s/renderD%d" > > Not sure what conventions are, but I'd rather name this "_NAME_FORMAT" > or something like this. It's not obvious that it contains > printf-specifiers. > I can fix this. > > +#define DRM_RENDER_NODE_MIN_MINOR 128 > > +#define DRM_RENDER_NODE_MAX_MINOR (DRM_RENDER_NODE_MIN_MINOR + > > DRM_MAX_MINOR) > > Linux actually allows 63 render-nodes while DRM_MAX_MINOR is defined > as 16. Don't know where DRM_MAX_MINOR really originated, it's not used > in the kernel at all. But it's fine. > Will the maximum number of render-nodes ever change? Is it codified somewhere? > > +#define DRM_RENDER_NODE_MAX_NODES (DRM_RENDER_NODE_MAX_MINOR - > > DRM_RENDER_NODE_MIN_MINOR) > > + > > struct pipe_loader_drm_device { > > struct pipe_loader_device base; > > struct util_dl_library *lib; > > @@ -216,22 +221,89 @@ open_drm_minor(int minor) > > return open(path, O_RDWR, 0); > > } > > > > +#ifdef USE_RENDER_NODES > > + > > +static int > > +open_drm_render_node_minor(int minor) > > +{ > > + char path[PATH_MAX]; > > + snprintf(path, sizeof(path), DRM_RENDER_NODE_DEV_NAME, DRM_DIR_NAME, > > minor); > > + return open(path, O_RDWR, 0); > > O_CLOEXEC? Or don't we use that in mesa? The trailing "0" is not > needed, btw. Only if you pass O_CREAT. > I'm not sure, I used O_RDWR, because the existing function open_drm_node_minor() also used O_RDWR. > > +} > > + > > +#endif > > + > > int > > pipe_loader_drm_probe(struct pipe_loader_device **devs, int ndev) > > { > > - int i, j, fd; > > + int i, k, fd, num_render_node_devs; > > + int j = 0; > > + > > + struct { > > + unsigned vendor_id; > > + unsigned chip_id; > > + } render_node_devs[DRM_RENDER_NODE_MAX_NODES]; > > + > > +#ifdef USE_RENDER_NODES > > + /* Look for render nodes first */ > > + for (i = DRM_RENDER_NODE_MIN_MINOR, j = 0; > > + i <= DRM_RENDER_NODE_MAX_MINOR; i++) { > > + fd = open_drm_render_node_minor(i); > > + struct pipe_loader_device *dev; > > + if (fd < 0) > > + continue; > > + > > + if (!pipe_loader_drm_probe_fd(&dev, fd)) { > > + close(fd); > > + continue; > > + } > > > > - for (i = 0, j = 0; i < DRM_MAX_MINOR; i++) { > > + render_node_devs[j].vendor_id = dev->u.pci.vendor_id; > > + render_node_devs[j].chip_id = dev->u.pci.chip_id; > > + > > + if (j < ndev) { > > + devs[j] = dev; > > + } else { > > + dev->ops->release(&dev); > > + } > > + j++; > > + } > > +#endif > > + num_render_node_devs = j; > > + > > + /* Look for a drm device. */ > > + for (i = 0; i < DRM_MAX_MINOR; i++) { > > + struct pipe_loader_device *dev; > > + boolean duplicate = FALSE; > > fd = open_drm_minor(i); > > if (fd < 0) > > continue; > > > > - if (j >= ndev || !pipe_loader_drm_probe_fd(&devs[j], fd)) > > + if (!pipe_loader_drm_probe_fd(&dev, fd)) { > > close(fd); > > + continue; > > + } > > + > > + for (k = 0; k < num_render_node_devs; k++) { > > + if (dev->u.pci.vendor_id == render_node_devs[k].vendor_id && > > + dev->u.pci.chip_id == render_node_devs[k].chip_id) { > > + close(fd); > > + duplicate = TRUE; > > If you call pipe_loader_drm_probe_fd() twice, do you get a shared > pipe_loader_device? Or what's the reason you don't call > dev->ops->release(&dev) here? > I should call release here, I'll fix this too. > > + break; > > + } > > + } > > + > > + if (duplicate) > > + continue; > > + > > + if (j < ndev) { > > + devs[j] = dev; > > + } else { > > + dev->ops->release(&dev); > > + } > > > > j++; > > } > > - > > return j; > > } > > Looks good to me. Using udev would be simpler, but BSD doesn't have that. > > Thanks > David > > > > > diff --git a/src/gallium/winsys/radeon/drm/Makefile.am > > b/src/gallium/winsys/radeon/drm/Makefile.am > > index d5c5474..ef08064 100644 > > --- a/src/gallium/winsys/radeon/drm/Makefile.am > > +++ b/src/gallium/winsys/radeon/drm/Makefile.am > > @@ -7,6 +7,10 @@ AM_CFLAGS = \ > > $(RADEON_CFLAGS) \ > > $(VISIBILITY_CFLAGS) > > > > +if HAVE_PIPE_LOADER_RENDER_NODES > > +AM_CFLAGS+= -DUSE_RENDER_NODES > > +endif > > + > > noinst_LTLIBRARIES = libradeonwinsys.la > > > > libradeonwinsys_la_SOURCES = $(C_SOURCES) > > -- > > 1.8.1.5 > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev