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.
> 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.
> +
> 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.
> +#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.
> +#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.
> +}
> +
> +#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?
> + 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
