Re: [RFC] tegra: Initial support

[Emil Velikov <emil.l.velikov@gmail.com>]

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 <treding@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