[Mesa-dev] [PATCH 2/2] pipe-loader: Add support for render nodes
David Herrmann
dh.herrmann at gmail.com
Wed Oct 30 18:34:46 CET 2013
Hi
On Wed, Oct 30, 2013 at 6:24 PM, Tom Stellard <tom at stellard.net> wrote:
> 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.
I'm no mesa developer, so your decision. I just wondered why we need
it. Render-nodes shouldn't have any negative impact, theoretically.
>> > 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?
Nope.
Minors are statically assigned in the kernel. 63 (or 64?) for each
group, legacy nodes starting at 1. control-nodes starting at 64,
render-nodes starting at 128.
>> > +#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.
Ok.
Thanks
David
>> > +}
>> > +
>> > +#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