Hi Thierry,
Must admit that I really prefer this idea over modifying existing
applications/users [1] because:
- Most of these setups are tightly coupled (imx+vivante, tegra+nouveau)
and pushing this down to the driver prevents duplication, and hardware
specific details in the users.
- Removes the need (at least temporary) to have an arbiter and policies
about which devices can and how they should be coupled.
Hope your trip through the build/target helpers did not leave you
tearing your hairs out :)
[1] xserver's libglx and everyone else whole deals with gbm or dri
modules directly.
On 27/11/14 16:39, Thierry Reding wrote:
> Tegra K1 and later use a GPU that can be driven by the Nouveau driver.
> But the GPU is a pure render node and has no display engine, hence the
> scanout needs to happen on the Tegra display hardware. The GPU and the
> display engine each have a separate DRM device node exposed by the
> kernel.
>
> To make the setup appear as a single device, this driver instantiates
> a Nouveau screen with each instance of a Tegra screen and forwards GPU
> requests to the Nouveau screen. For purposes of scanout it will import
> buffers created on the GPU into the display driver. Handles that
> userspace requests are those of the display driver so that they can be
> used to create framebuffers.
>
> This has been tested with some GBM test programs, as well as kmscube and
> weston. All of those run without modifications, but I'm sure there is a
> lot that can be improved.
>
> TODO:
> - use Nouveau headers to get at the prototype for creating a screen
I've addressed those inline for you :)
> - implement enough support to seamlessly integrate with X
> - refactor some of the code to be reusable by other drivers
I think this topic will be open for debate for a while. Especially since
tegra is only one driver (for now) that uses this type on
stacking/wrapping thus it's not so easy to predict if others won't need
anything extra.
>
> Signed-off-by: Thierry Reding <tred...@nvidia.com>
> ---
> configure.ac | 12 +-
> src/gallium/Makefile.am | 5 +
> .../auxiliary/target-helpers/inline_drm_helper.h | 30 +
> src/gallium/drivers/tegra/Automake.inc | 11 +
> src/gallium/drivers/tegra/Makefile.am | 17 +
> src/gallium/drivers/tegra/Makefile.sources | 4 +
> src/gallium/drivers/tegra/tegra_context.c | 699
> +++++++++++++++++++++
> src/gallium/drivers/tegra/tegra_context.h | 80 +++
> src/gallium/drivers/tegra/tegra_resource.c | 219 +++++++
> src/gallium/drivers/tegra/tegra_resource.h | 98 +++
> src/gallium/drivers/tegra/tegra_screen.c | 311 +++++++++
> src/gallium/drivers/tegra/tegra_screen.h | 45 ++
> src/gallium/targets/dri/Makefile.am | 2 +
> src/gallium/winsys/tegra/drm/Makefile.am | 11 +
> src/gallium/winsys/tegra/drm/Makefile.sources | 2 +
> src/gallium/winsys/tegra/drm/tegra_drm_public.h | 31 +
> src/gallium/winsys/tegra/drm/tegra_drm_winsys.c | 33 +
> 17 files changed, 1609 insertions(+), 1 deletion(-)
> create mode 100644 src/gallium/drivers/tegra/Automake.inc
> create mode 100644 src/gallium/drivers/tegra/Makefile.am
> create mode 100644 src/gallium/drivers/tegra/Makefile.sources
> create mode 100644 src/gallium/drivers/tegra/tegra_context.c
> create mode 100644 src/gallium/drivers/tegra/tegra_context.h
> create mode 100644 src/gallium/drivers/tegra/tegra_resource.c
> create mode 100644 src/gallium/drivers/tegra/tegra_resource.h
> create mode 100644 src/gallium/drivers/tegra/tegra_screen.c
> create mode 100644 src/gallium/drivers/tegra/tegra_screen.h
> create mode 100644 src/gallium/winsys/tegra/drm/Makefile.am
> create mode 100644 src/gallium/winsys/tegra/drm/Makefile.sources
> create mode 100644 src/gallium/winsys/tegra/drm/tegra_drm_public.h
> create mode 100644 src/gallium/winsys/tegra/drm/tegra_drm_winsys.c
>
> diff --git a/configure.ac b/configure.ac
> index 1d9d015481ec..ae50bec95339 100644
> --- a/configure.ac
> +++ b/configure.ac
[...]
> @@ -1937,6 +1938,12 @@ if test -n "$with_gallium_drivers"; then
> gallium_require_drm "freedreno"
> gallium_require_drm_loader
> ;;
> + xtegra)
> + HAVE_GALLIUM_TEGRA=yes
> + PKG_CHECK_MODULES([TEGRA], [libdrm_tegra >=
> $LIBDRM_TEGRA_REQUIRED])
> + gallium_require_drm "tegra"
> + gallium_require_drm_loader
> + ;;
You might want to have something like the following further down.
Admittedly it's not perfect (it will create a nouveau_dri.so hardlink)
but it'll avoid the manual case for nouveau dependencies.
if test "x$HAVE_GALLIUM_NOUVEAU" != xyes -a
test "x$HAVE_GALLIUM_TEGRA" = xyes; then
AC_ERROR([Building with tegra requires that nouveau])
fi
[...]
> diff --git a/src/gallium/drivers/tegra/Makefile.am
> b/src/gallium/drivers/tegra/Makefile.am
> new file mode 100644
> index 000000000000..eb03df9bb2ed
> --- /dev/null
> +++ b/src/gallium/drivers/tegra/Makefile.am
> @@ -0,0 +1,17 @@
> +AUTOMAKE_OPTIONS = subdir-objects
> +
Don't think you need the AUTOMAKE_OPTIONS here.
> +include Makefile.sources
> +include $(top_srcdir)/src/gallium/Automake.inc
> +
> +AM_CFLAGS = \
> + $(GALLIUM_DRIVER_CFLAGS) \
> + $(LIBUDEV_CFLAGS) \
> + $(TEGRA_CFLAGS)
> +
> +noinst_LTLIBRARIES = libtegra.la
> +
> +libtegra_la_SOURCES = \
> + $(C_SOURCES)
> +
> +libtegra_la_LIBADD = \
> + $(LIBUDEV_LIBS)
Here comes the big question:
Can we do something to avoid the link against libudev ? If we _do_ need
to go through libudev, can we please dlopen/dlsym it.
> diff --git a/src/gallium/drivers/tegra/Makefile.sources
> b/src/gallium/drivers/tegra/Makefile.sources
> new file mode 100644
> index 000000000000..978dd14667f5
> --- /dev/null
> +++ b/src/gallium/drivers/tegra/Makefile.sources
> @@ -0,0 +1,4 @@
> +C_SOURCES := \
> + tegra_context.c \
> + tegra_resource.c \
> + tegra_screen.c
Please add the headers to the above list. It will help automake pick
them up in the distribution tarball.
[...]
> diff --git a/src/gallium/drivers/tegra/tegra_resource.c
> b/src/gallium/drivers/tegra/tegra_resource.c
> new file mode 100644
> index 000000000000..8c5b7d4e41fc
> --- /dev/null
> +++ b/src/gallium/drivers/tegra/tegra_resource.c
[...]
> +#include <drm/tegra_drm.h>
Please drop the "drm/" part.
> diff --git a/src/gallium/drivers/tegra/tegra_screen.c
> b/src/gallium/drivers/tegra/tegra_screen.c
> new file mode 100644
> index 000000000000..aa7bf65cb7ec
> --- /dev/null
> +++ b/src/gallium/drivers/tegra/tegra_screen.c
[...]
> +#ifdef HAVE_LIBUDEV
> +#include <libudev.h>
> +#endif
> +
You might as well drop the #ifdef here. Afaics you're using udev API
explicitly below.
> +#include "util/u_debug.h"
> +
> +#include "tegra/tegra_context.h"
> +#include "tegra/tegra_resource.h"
> +#include "tegra/tegra_screen.h"
> +
> +/* TODO: obtain from include file */
> +struct pipe_screen *nouveau_drm_screen_create(int fd);
> +
#include "nouveau/drm/nouveau_drm_public.h" ?
> +static const char *
> +tegra_get_name(struct pipe_screen *pscreen)
> +{
> + return "tegra";
For nouveau/radeons we add the chipset name so I guess doing similar
thing here wouldn't be a bad idea. Additionally for combined/stacked
drivers we might want to return both the base + gpu's get_name(). It
will provide nice feedback about the actual rendering device + programs
which use device name hacks will continue working :)
> +}
> +
> +static const char *
> +tegra_get_vendor(struct pipe_screen *pscreen)
> +{
> + return "tegra";
Provide both vendors similar to above ? "NVIDIA Corp + nouveau" :P
[...]
> +static int tegra_open_render_node(int fd)
> +{
[...]
> + udev_list_entry_foreach(entry, list) {
> + const char *path = udev_list_entry_get_name(entry);
> + struct udev_device *device, *bus;
> +
> + device = udev_device_new_from_syspath(udev, path);
> + if (!device)
> + continue;
> +
> + path = udev_device_get_devnode(device);
> +
> + parent = udev_device_get_parent(device);
> + if (!parent) {
> + udev_device_unref(device);
> + continue;
> + }
> +
> + /* do not match if the render nodes shares the same parent */
> + if (udev_device_match(parent, display)) {
> + udev_device_unref(parent);
> + udev_device_unref(device);
> + continue;
> + }
> +
> + bus = udev_device_get_root(device);
> + if (!bus) {
> + udev_device_unref(parent);
> + udev_device_unref(device);
> + continue;
> + }
> +
> + /* both devices need to be on the same bus, though */
> + if (udev_device_match(bus, root)) {
> + fd = open(path, O_RDWR);
open(path) only to ignore it and return open("renderD128") below ? Seems
like something is missing here.
> + if (fd < 0)
> + fd = -errno;
> +
> + break;
> + }
> + }
> +
> + udev_enumerate_unref(enumerate);
> + udev_unref(udev);
> +
> + return open("/dev/dri/renderD128", O_RDWR);
> +}
> +
[...]
> diff --git a/src/gallium/winsys/tegra/drm/Makefile.am
> b/src/gallium/winsys/tegra/drm/Makefile.am
> new file mode 100644
> index 000000000000..8e3685ee20e8
> --- /dev/null
> +++ b/src/gallium/winsys/tegra/drm/Makefile.am
> @@ -0,0 +1,11 @@
> +include Makefile.sources
> +include $(top_srcdir)/src/gallium/Automake.inc
> +
> +AM_CFLAGS = \
> + -I$(top_srcdir)/src/gallium/drivers \
> + $(GALLIUM_WINSYS_CFLAGS) \
> + $(FREEDRENO_CFLAGS)
> +
Do we even need FREEDRENO/TEGRA_CFLAGS here ?
> +noinst_LTLIBRARIES = libtegradrm.la
> +
> +libtegradrm_la_SOURCES = $(C_SOURCES)
> diff --git a/src/gallium/winsys/tegra/drm/Makefile.sources
> b/src/gallium/winsys/tegra/drm/Makefile.sources
> new file mode 100644
> index 000000000000..fe0d5c42e72d
> --- /dev/null
> +++ b/src/gallium/winsys/tegra/drm/Makefile.sources
> @@ -0,0 +1,2 @@
> +C_SOURCES := \
> + tegra_drm_winsys.c
Please add tegra_drm_public.h to the list.
[...]
> diff --git a/src/gallium/winsys/tegra/drm/tegra_drm_winsys.c
> b/src/gallium/winsys/tegra/drm/tegra_drm_winsys.c
> new file mode 100644
> index 000000000000..99b0e1649026
> --- /dev/null
> +++ b/src/gallium/winsys/tegra/drm/tegra_drm_winsys.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright © 2014 NVIDIA Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "util/u_debug.h"
> +
> +#include "tegra/tegra_screen.h"
> +
> +struct pipe_screen *tegra_drm_screen_create(int fd);
> +
util/u_debug.h is not be needed so the following should suffice.
#include "tegra_drm_public.h"
#include "tegra/tegra_screen.h"
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
