[Mesa-dev] [PATCH 2/2] pipe-loader: Add support for render nodes
Tom Stellard
tom at stellard.net
Wed Oct 30 18:24:56 CET 2013
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 <tom at stellard.net> wrote:
> > From: Tom Stellard <thomas.stellard at 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
> >
More information about the mesa-dev
mailing list