* [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:15 ` Erik Faye-Lund
2014-04-09 11:40 ` [PATCH v2 libdrm 2/7] libdrm: Add NVIDIA Tegra support Thierry Reding
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
Checks whether or not the compiler supports the -fvisibility option. If
so it sets the VISIBILITY_CFLAGS variable which can be added to the per
directory AM_CFLAGS where appropriate.
By default all symbols will be hidden via the VISIBILITY_CFLAGS. The
drm_public macro can be used to mark symbols that should be exported.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
configure.ac | 20 ++++++++++++++++++++
libdrm.h | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
create mode 100644 libdrm.h
diff --git a/configure.ac b/configure.ac
index b7eef9673a20..22b45cc7560d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -366,6 +366,26 @@ AC_ARG_WITH([kernel-source],
[kernel_source="$with_kernel_source"])
AC_SUBST(kernel_source)
+dnl Add flags for gcc and g++
+if test "x$GCC" = xyes; then
+ # Enable -fvisibility=hidden if using a gcc that supports it
+ save_CFLAGS="$CFLAGS"
+ AC_MSG_CHECKING([whether $CC supports -fvisibility=hidden])
+ VISIBILITY_CFLAGS="-fvisibility=hidden"
+ CFLAGS="$CFLAGS $VISIBILITY_CFLAGS"
+ AC_LINK_IFELSE([AC_LANG_PROGRAM()], AC_MSG_RESULT([yes]),
+ [VISIBILITY_CFLAGS=""; AC_MSG_RESULT([no])]);
+
+ # Restore CFLAGS; VISIBILITY_CFLAGS are added to it where needed.
+ CFLAGS=$save_CFLAGS
+
+ if test "x$VISIBILITY_CFLAGS" != x; then
+ AC_DEFINE(HAVE_VISIBILITY, 1, [Compiler has -fvisibility support])
+ fi
+
+ AC_SUBST([VISIBILITY_CFLAGS])
+fi
+
AC_SUBST(WARN_CFLAGS)
AC_CONFIG_FILES([
Makefile
diff --git a/libdrm.h b/libdrm.h
new file mode 100644
index 000000000000..23926e6f6741
--- /dev/null
+++ b/libdrm.h
@@ -0,0 +1,34 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef LIBDRM_LIBDRM_H
+#define LIBDRM_LIBDRM_H
+
+#if defined(HAVE_VISIBILITY)
+# define drm_private __attribute__((visibility("hidden")))
+# define drm_public __attribute__((visibility("default")))
+#else
+# define drm_private
+# define drm_public
+#endif
+
+#endif
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available
2014-04-09 11:40 ` [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available Thierry Reding
@ 2014-04-10 17:15 ` Erik Faye-Lund
2014-05-02 14:12 ` Thierry Reding
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-10 17:15 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> diff --git a/libdrm.h b/libdrm.h
> new file mode 100644
> index 000000000000..23926e6f6741
> --- /dev/null
> +++ b/libdrm.h
> @@ -0,0 +1,34 @@
> +/*
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> + */
> +
> +#ifndef LIBDRM_LIBDRM_H
> +#define LIBDRM_LIBDRM_H
LIBDRM_LIBDRM_H sounds a bit clunky to me. Why LIBDRM twice? The other
headers don't seem to prefix LIBDRM_ to their header-guards. In fact,
many of them don't even have header-guards.
Also, does these macro really warrant making a top-level, generically
named header?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available
2014-04-10 17:15 ` Erik Faye-Lund
@ 2014-05-02 14:12 ` Thierry Reding
2014-05-02 14:59 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:12 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 2327 bytes --]
On Thu, Apr 10, 2014 at 07:15:14PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > diff --git a/libdrm.h b/libdrm.h
> > new file mode 100644
> > index 000000000000..23926e6f6741
> > --- /dev/null
> > +++ b/libdrm.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> > + */
> > +
> > +#ifndef LIBDRM_LIBDRM_H
> > +#define LIBDRM_LIBDRM_H
>
> LIBDRM_LIBDRM_H sounds a bit clunky to me. Why LIBDRM twice? The other
> headers don't seem to prefix LIBDRM_ to their header-guards. In fact,
> many of them don't even have header-guards.
This was with the intention of marking it as an internal header file. So
the LIBDRM_ prefix could be used consistently for all files that are not
installed. xf86atomic.h uses that prefix as well.
> Also, does these macro really warrant making a top-level, generically
> named header?
There isn't really another header file where this would fit. Others are
either installed (and therefore shouldn't be exposing these macros) or
have a very specific purpose (xf86atomic.h).
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available
2014-05-02 14:12 ` Thierry Reding
@ 2014-05-02 14:59 ` Erik Faye-Lund
0 siblings, 0 replies; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 14:59 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Fri, May 2, 2014 at 4:12 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Apr 10, 2014 at 07:15:14PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > diff --git a/libdrm.h b/libdrm.h
>> > new file mode 100644
>> > index 000000000000..23926e6f6741
>> > --- /dev/null
>> > +++ b/libdrm.h
>> > @@ -0,0 +1,34 @@
>> > +/*
>> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
>> > + */
>> > +
>> > +#ifndef LIBDRM_LIBDRM_H
>> > +#define LIBDRM_LIBDRM_H
>>
>> LIBDRM_LIBDRM_H sounds a bit clunky to me. Why LIBDRM twice? The other
>> headers don't seem to prefix LIBDRM_ to their header-guards. In fact,
>> many of them don't even have header-guards.
>
> This was with the intention of marking it as an internal header file. So
> the LIBDRM_ prefix could be used consistently for all files that are not
> installed. xf86atomic.h uses that prefix as well.
If you look at the history of xf86atomic.h, it seems this strange
header-guard is the result of a slightly careless replace. It used to
be called intel_atomics.h, and have INTEL_ATOMICS_H as the
header-guard. So I wouldn't lake that set too much of a precedence.
>> Also, does these macro really warrant making a top-level, generically
>> named header?
>
> There isn't really another header file where this would fit. Others are
> either installed (and therefore shouldn't be exposing these macros) or
> have a very specific purpose (xf86atomic.h).
I guess this is a matter of taste, and it would be great with some
input from the other libdrm-people on this. I don't care too much
either way...
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 libdrm 2/7] libdrm: Add NVIDIA Tegra support
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
2014-04-09 11:40 ` [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:19 ` Erik Faye-Lund
2014-04-09 11:40 ` [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open() Thierry Reding
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <thierry.reding@avionic-design.de>
Add the libdrm_tegra helper library to encapsulate Tegra-specific
interfaces to the DRM.
Furthermore, Tegra is added to the list of supported chips in the
modetest and vbltest programs.
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Makefile.am | 6 +-
configure.ac | 15 ++-
include/drm/Makefile.am | 1 +
include/drm/tegra_drm.h | 155 ++++++++++++++++++++++++++++
tegra/.gitignore | 1 +
tegra/Makefile.am | 20 ++++
tegra/libdrm_tegra.pc.in | 11 ++
tegra/private.h | 51 ++++++++++
tegra/tegra.c | 253 ++++++++++++++++++++++++++++++++++++++++++++++
tegra/tegra.h | 47 +++++++++
tests/modetest/modetest.c | 2 +-
tests/vbltest/vbltest.c | 2 +-
12 files changed, 560 insertions(+), 4 deletions(-)
create mode 100644 include/drm/tegra_drm.h
create mode 100644 tegra/.gitignore
create mode 100644 tegra/Makefile.am
create mode 100644 tegra/libdrm_tegra.pc.in
create mode 100644 tegra/private.h
create mode 100644 tegra/tegra.c
create mode 100644 tegra/tegra.h
diff --git a/Makefile.am b/Makefile.am
index 826c30d0c0d9..14c402dce74f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -51,7 +51,11 @@ if HAVE_FREEDRENO
FREEDRENO_SUBDIR = freedreno
endif
-SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR) $(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) $(FREEDRENO_SUBDIR) tests include man
+if HAVE_TEGRA
+TEGRA_SUBDIR = tegra
+endif
+
+SUBDIRS = . $(LIBKMS_SUBDIR) $(INTEL_SUBDIR) $(NOUVEAU_SUBDIR) $(RADEON_SUBDIR) $(OMAP_SUBDIR) $(EXYNOS_SUBDIR) $(FREEDRENO_SUBDIR) $(TEGRA_SUBDIR) tests include man
libdrm_la_LTLIBRARIES = libdrm.la
libdrm_ladir = $(libdir)
diff --git a/configure.ac b/configure.ac
index 22b45cc7560d..4cb6810bd6a8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -98,6 +98,11 @@ AC_ARG_ENABLE(freedreno-experimental-api,
[Enable support for freedreno's experimental API (default: disabled)]),
[FREEDRENO=$enableval], [FREEDRENO=no])
+AC_ARG_ENABLE(tegra-experimental-api,
+ AS_HELP_STRING([--enable-tegra-experimental-api],
+ [Enable support for Tegra's experimental API (default: disabled)]),
+ [TEGRA=$enableval], [TEGRA=no])
+
AC_ARG_ENABLE(install-test-programs,
AS_HELP_STRING([--enable-install-test-programs],
[Install test programs (default: no)]),
@@ -219,6 +224,11 @@ if test "x$FREEDRENO" = xyes; then
AC_DEFINE(HAVE_FREEDRENO, 1, [Have freedreno support])
fi
+AM_CONDITIONAL(HAVE_TEGRA, [test "x$TEGRA" = xyes])
+if test "x$TEGRA" = xyes; then
+ AC_DEFINE(HAVE_TEGRA, 1, [Have Tegra support])
+fi
+
AM_CONDITIONAL(HAVE_INSTALL_TESTS, [test "x$INSTALL_TESTS" = xyes])
if test "x$INSTALL_TESTS" = xyes; then
AC_DEFINE(HAVE_INSTALL_TESTS, 1, [Install test programs])
@@ -270,7 +280,7 @@ else
fi
AM_CONDITIONAL([HAVE_MANPAGES_STYLESHEET], [test "x$HAVE_MANPAGES_STYLESHEET" = "xyes"])
-if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != "xno" -o "x$OMAP" != "xno" -o "x$FREEDRENO" != "xno"; then
+if test "x$INTEL" != "xno" -o "x$RADEON" != "xno" -o "x$NOUVEAU" != "xno" -o "x$OMAP" != "xno" -o "x$FREEDRENO" != "xno" -o "x$TEGRA" != "xno"; then
# Check for atomic intrinsics
AC_CACHE_CHECK([for native atomic primitives], drm_cv_atomic_primitives,
[
@@ -403,6 +413,8 @@ AC_CONFIG_FILES([
exynos/libdrm_exynos.pc
freedreno/Makefile
freedreno/libdrm_freedreno.pc
+ tegra/Makefile
+ tegra/libdrm_tegra.pc
tests/Makefile
tests/modeprint/Makefile
tests/modetest/Makefile
@@ -427,4 +439,5 @@ echo " Nouveau API $NOUVEAU"
echo " OMAP API $OMAP"
echo " EXYNOS API $EXYNOS"
echo " Freedreno API $FREEDRENO"
+echo " Tegra API $TEGRA"
echo ""
diff --git a/include/drm/Makefile.am b/include/drm/Makefile.am
index 2bc34d2ffd9f..f591abc45152 100644
--- a/include/drm/Makefile.am
+++ b/include/drm/Makefile.am
@@ -35,6 +35,7 @@ klibdrminclude_HEADERS = \
radeon_drm.h \
savage_drm.h \
sis_drm.h \
+ tegra_drm.h \
via_drm.h \
mach64_drm.h \
qxl_drm.h
diff --git a/include/drm/tegra_drm.h b/include/drm/tegra_drm.h
new file mode 100644
index 000000000000..1b8b7c1edef7
--- /dev/null
+++ b/include/drm/tegra_drm.h
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2012-2013, NVIDIA CORPORATION. All rights reserved.
+ *
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef _UAPI_TEGRA_DRM_H_
+#define _UAPI_TEGRA_DRM_H_
+
+#include <drm.h>
+
+#define DRM_TEGRA_GEM_CREATE_TILED (1 << 0)
+#define DRM_TEGRA_GEM_CREATE_BOTTOM_UP (1 << 1)
+
+struct drm_tegra_gem_create {
+ __u64 size;
+ __u32 flags;
+ __u32 handle;
+};
+
+struct drm_tegra_gem_mmap {
+ __u32 handle;
+ __u32 offset;
+};
+
+struct drm_tegra_syncpt_read {
+ __u32 id;
+ __u32 value;
+};
+
+struct drm_tegra_syncpt_incr {
+ __u32 id;
+ __u32 pad;
+};
+
+struct drm_tegra_syncpt_wait {
+ __u32 id;
+ __u32 thresh;
+ __u32 timeout;
+ __u32 value;
+};
+
+#define DRM_TEGRA_NO_TIMEOUT (0xffffffff)
+
+struct drm_tegra_open_channel {
+ __u32 client;
+ __u32 pad;
+ __u64 context;
+};
+
+struct drm_tegra_close_channel {
+ __u64 context;
+};
+
+struct drm_tegra_get_syncpt {
+ __u64 context;
+ __u32 index;
+ __u32 id;
+};
+
+struct drm_tegra_get_syncpt_base {
+ __u64 context;
+ __u32 syncpt;
+ __u32 id;
+};
+
+struct drm_tegra_syncpt {
+ __u32 id;
+ __u32 incrs;
+};
+
+struct drm_tegra_cmdbuf {
+ __u32 handle;
+ __u32 offset;
+ __u32 words;
+ __u32 pad;
+};
+
+struct drm_tegra_reloc {
+ struct {
+ __u32 handle;
+ __u32 offset;
+ } cmdbuf;
+ struct {
+ __u32 handle;
+ __u32 offset;
+ } target;
+ __u32 shift;
+ __u32 pad;
+};
+
+struct drm_tegra_waitchk {
+ __u32 handle;
+ __u32 offset;
+ __u32 syncpt;
+ __u32 thresh;
+};
+
+struct drm_tegra_submit {
+ __u64 context;
+ __u32 num_syncpts;
+ __u32 num_cmdbufs;
+ __u32 num_relocs;
+ __u32 num_waitchks;
+ __u32 waitchk_mask;
+ __u32 timeout;
+ __u32 pad;
+ __u64 syncpts;
+ __u64 cmdbufs;
+ __u64 relocs;
+ __u64 waitchks;
+ __u32 fence; /* Return value */
+
+ __u32 reserved[5]; /* future expansion */
+};
+
+#define DRM_TEGRA_GEM_CREATE 0x00
+#define DRM_TEGRA_GEM_MMAP 0x01
+#define DRM_TEGRA_SYNCPT_READ 0x02
+#define DRM_TEGRA_SYNCPT_INCR 0x03
+#define DRM_TEGRA_SYNCPT_WAIT 0x04
+#define DRM_TEGRA_OPEN_CHANNEL 0x05
+#define DRM_TEGRA_CLOSE_CHANNEL 0x06
+#define DRM_TEGRA_GET_SYNCPT 0x07
+#define DRM_TEGRA_SUBMIT 0x08
+#define DRM_TEGRA_GET_SYNCPT_BASE 0x09
+
+#define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_CREATE, struct drm_tegra_gem_create)
+#define DRM_IOCTL_TEGRA_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP, struct drm_tegra_gem_mmap)
+#define DRM_IOCTL_TEGRA_SYNCPT_READ DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_SYNCPT_READ, struct drm_tegra_syncpt_read)
+#define DRM_IOCTL_TEGRA_SYNCPT_INCR DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_SYNCPT_INCR, struct drm_tegra_syncpt_incr)
+#define DRM_IOCTL_TEGRA_SYNCPT_WAIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_SYNCPT_WAIT, struct drm_tegra_syncpt_wait)
+#define DRM_IOCTL_TEGRA_OPEN_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_OPEN_CHANNEL, struct drm_tegra_open_channel)
+#define DRM_IOCTL_TEGRA_CLOSE_CHANNEL DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_CLOSE_CHANNEL, struct drm_tegra_open_channel)
+#define DRM_IOCTL_TEGRA_GET_SYNCPT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GET_SYNCPT, struct drm_tegra_get_syncpt)
+#define DRM_IOCTL_TEGRA_SUBMIT DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_SUBMIT, struct drm_tegra_submit)
+#define DRM_IOCTL_TEGRA_GET_SYNCPT_BASE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GET_SYNCPT_BASE, struct drm_tegra_get_syncpt_base)
+
+#endif
diff --git a/tegra/.gitignore b/tegra/.gitignore
new file mode 100644
index 000000000000..74cfe47a2592
--- /dev/null
+++ b/tegra/.gitignore
@@ -0,0 +1 @@
+libdrm_tegra.pc
diff --git a/tegra/Makefile.am b/tegra/Makefile.am
new file mode 100644
index 000000000000..1b83145b120d
--- /dev/null
+++ b/tegra/Makefile.am
@@ -0,0 +1,20 @@
+AM_CPPFLAGS = \
+ -I$(top_srcdir) \
+ -I$(top_srcdir)/include/drm
+
+AM_CFLAGS = \
+ $(VISIBILITY_CFLAGS)
+
+libdrm_tegra_ladir = $(libdir)
+libdrm_tegra_la_LTLIBRARIES = libdrm_tegra.la
+libdrm_tegra_la_LDFLAGS = -version-number 0:0:0 -no-undefined
+libdrm_tegra_la_LIBADD = ../libdrm.la @PTHREADSTUBS_LIBS@
+
+libdrm_tegra_la_SOURCES = \
+ tegra.c
+
+libdrm_tegraincludedir = ${includedir}/libdrm
+libdrm_tegrainclude_HEADERS = tegra.h
+
+pkgconfigdir = @pkgconfigdir@
+pkgconfig_DATA = libdrm_tegra.pc
diff --git a/tegra/libdrm_tegra.pc.in b/tegra/libdrm_tegra.pc.in
new file mode 100644
index 000000000000..2e06f49c6fba
--- /dev/null
+++ b/tegra/libdrm_tegra.pc.in
@@ -0,0 +1,11 @@
+prefix=@prefix@
+exec_prefix=@exec_prefix@
+libdir=@libdir@
+includedir=@includedir@
+
+Name: libdrm_tegra
+Description: Userspace interface to Tegra kernel DRM services
+Version: @PACKAGE_VERSION@
+Libs: -L${libdir} -ldrm_tegra
+Cflags: -I${includedir} -I${includedir}/libdrm
+Requires.private: libdrm
diff --git a/tegra/private.h b/tegra/private.h
new file mode 100644
index 000000000000..9b6bc9395d23
--- /dev/null
+++ b/tegra/private.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef __DRM_TEGRA_PRIVATE_H__
+#define __DRM_TEGRA_PRIVATE_H__ 1
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include <libdrm.h>
+#include <xf86atomic.h>
+
+#include "tegra.h"
+
+struct drm_tegra {
+ bool close;
+ int fd;
+};
+
+struct drm_tegra_bo {
+ struct drm_tegra *drm;
+ uint32_t handle;
+ uint32_t offset;
+ uint32_t flags;
+ uint32_t size;
+ atomic_t ref;
+ void *map;
+};
+
+#endif /* __DRM_TEGRA_PRIVATE_H__ */
diff --git a/tegra/tegra.c b/tegra/tegra.c
new file mode 100644
index 000000000000..bf6d035453d5
--- /dev/null
+++ b/tegra/tegra.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+
+#include <sys/mman.h>
+
+#include <xf86drm.h>
+
+#include <tegra_drm.h>
+
+#include "private.h"
+
+static inline struct tegra_bo *tegra_bo(struct drm_tegra_bo *bo)
+{
+ return (struct tegra_bo *)bo;
+}
+
+static void drm_tegra_bo_free(struct drm_tegra_bo *bo)
+{
+ struct drm_tegra *drm = bo->drm;
+ struct drm_gem_close args;
+
+ if (bo->map)
+ munmap(bo->map, bo->size);
+
+ memset(&args, 0, sizeof(args));
+ args.handle = bo->handle;
+
+ drmIoctl(drm->fd, DRM_IOCTL_GEM_CLOSE, &args);
+
+ free(bo);
+}
+
+static int drm_tegra_wrap(struct drm_tegra **drmp, int fd, bool close)
+{
+ struct drm_tegra *drm;
+ int err;
+
+ if (fd < 0 || !drmp)
+ return -EINVAL;
+
+ drm = calloc(1, sizeof(*drm));
+ if (!drm)
+ return -ENOMEM;
+
+ drm->close = close;
+ drm->fd = fd;
+
+ *drmp = drm;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_new(struct drm_tegra **drmp, int fd)
+{
+ bool supported = false;
+ drmVersionPtr version;
+
+ version = drmGetVersion(fd);
+ if (!version)
+ return -ENOMEM;
+
+ if (!strncmp(version->name, "tegra", version->name_len))
+ supported = true;
+
+ drmFreeVersion(version);
+
+ if (!supported)
+ return -ENOTSUP;
+
+ return drm_tegra_wrap(drmp, fd, false);
+}
+
+drm_public
+void drm_tegra_close(struct drm_tegra *drm)
+{
+ if (!drm)
+ return;
+
+ if (drm->close)
+ close(drm->fd);
+
+ free(drm);
+}
+
+drm_public
+int drm_tegra_bo_new(struct drm_tegra_bo **bop, struct drm_tegra *drm,
+ uint32_t flags, uint32_t size)
+{
+ struct drm_tegra_gem_create args;
+ struct drm_tegra_bo *bo;
+ int err;
+
+ if (!drm || size == 0 || !bop)
+ return -EINVAL;
+
+ bo = calloc(1, sizeof(*bo));
+ if (!bo)
+ return -ENOMEM;
+
+ atomic_set(&bo->ref, 1);
+ bo->flags = flags;
+ bo->size = size;
+ bo->drm = drm;
+
+ memset(&args, 0, sizeof(args));
+ args.flags = flags;
+ args.size = size;
+
+ err = drmCommandWriteRead(drm->fd, DRM_TEGRA_GEM_CREATE, &args,
+ sizeof(args));
+ if (err < 0) {
+ err = -errno;
+ free(bo);
+ return err;
+ }
+
+ bo->handle = args.handle;
+
+ *bop = bo;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_bo_wrap(struct drm_tegra_bo **bop, struct drm_tegra *drm,
+ uint32_t handle, uint32_t flags, uint32_t size)
+{
+ struct drm_tegra_bo *bo;
+
+ if (!drm || !bop)
+ return -EINVAL;
+
+ bo = calloc(1, sizeof(*bo));
+ if (!bo)
+ return -ENOMEM;
+
+ atomic_set(&bo->ref, 1);
+ bo->handle = handle;
+ bo->flags = flags;
+ bo->size = size;
+ bo->drm = drm;
+
+ *bop = bo;
+
+ return 0;
+}
+
+drm_public
+struct drm_tegra_bo *drm_tegra_bo_get(struct drm_tegra_bo *bo)
+{
+ if (bo)
+ atomic_inc(&bo->ref);
+
+ return bo;
+}
+
+drm_public
+void drm_tegra_bo_put(struct drm_tegra_bo *bo)
+{
+ if (bo && atomic_dec_and_test(&bo->ref))
+ drm_tegra_bo_free(bo);
+}
+
+drm_public
+int drm_tegra_bo_get_handle(struct drm_tegra_bo *bo, uint32_t *handle)
+{
+ if (!bo || !handle)
+ return -EINVAL;
+
+ *handle = bo->handle;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_bo_map(struct drm_tegra_bo *bo, void **ptr)
+{
+ struct drm_tegra *drm = bo->drm;
+
+ if (!bo->map) {
+ struct drm_tegra_gem_mmap args;
+ int err;
+
+ memset(&args, 0, sizeof(args));
+ args.handle = bo->handle;
+
+ err = drmCommandWriteRead(drm->fd, DRM_TEGRA_GEM_MMAP, &args,
+ sizeof(args));
+ if (err < 0)
+ return -errno;
+
+ bo->offset = args.offset;
+
+ bo->map = mmap(0, bo->size, PROT_READ | PROT_WRITE, MAP_SHARED,
+ drm->fd, bo->offset);
+ if (bo->map == MAP_FAILED) {
+ bo->map = NULL;
+ return -errno;
+ }
+ }
+
+ if (ptr)
+ *ptr = bo->map;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_bo_unmap(struct drm_tegra_bo *bo)
+{
+ if (!bo)
+ return -EINVAL;
+
+ if (!bo->map)
+ return 0;
+
+ if (munmap(bo->map, bo->size))
+ return -errno;
+
+ bo->map = NULL;
+
+ return 0;
+}
diff --git a/tegra/tegra.h b/tegra/tegra.h
new file mode 100644
index 000000000000..0731cb3bd4dc
--- /dev/null
+++ b/tegra/tegra.h
@@ -0,0 +1,47 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef __DRM_TEGRA_H__
+#define __DRM_TEGRA_H__ 1
+
+#include <stdint.h>
+#include <stdlib.h>
+
+struct drm_tegra_bo;
+struct drm_tegra;
+
+int drm_tegra_new(struct drm_tegra **drmp, int fd);
+void drm_tegra_close(struct drm_tegra *drm);
+
+int drm_tegra_bo_new(struct drm_tegra_bo **bop, struct drm_tegra *drm,
+ uint32_t flags, uint32_t size);
+int drm_tegra_bo_wrap(struct drm_tegra_bo **bop, struct drm_tegra *drm,
+ uint32_t handle, uint32_t flags, uint32_t size);
+struct drm_tegra_bo *drm_tegra_bo_get(struct drm_tegra_bo *bo);
+void drm_tegra_bo_put(struct drm_tegra_bo *bo);
+int drm_tegra_bo_get_handle(struct drm_tegra_bo *bo, uint32_t *handle);
+int drm_tegra_bo_map(struct drm_tegra_bo *bo, void **ptr);
+int drm_tegra_bo_unmap(struct drm_tegra_bo *bo);
+
+#endif /* __DRM_TEGRA_H__ */
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index bc9c9988dec7..4212963f8ff0 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -1386,7 +1386,7 @@ int main(int argc, char **argv)
int encoders = 0, connectors = 0, crtcs = 0, planes = 0, framebuffers = 0;
int drop_master = 0;
int test_vsync = 0;
- const char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos", "tilcdc", "msm" };
+ const char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos", "tilcdc", "msm", "tegra" };
char *device = NULL;
char *module = NULL;
unsigned int i;
diff --git a/tests/vbltest/vbltest.c b/tests/vbltest/vbltest.c
index 2a09d28ed00a..e4b200bf0c3c 100644
--- a/tests/vbltest/vbltest.c
+++ b/tests/vbltest/vbltest.c
@@ -103,7 +103,7 @@ static void usage(char *name)
int main(int argc, char **argv)
{
int i, c, fd, ret;
- char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "exynos", "omapdrm", "tilcdc", "msm" };
+ char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "exynos", "omapdrm", "tilcdc", "msm", "tegra" };
drmVBlank vbl;
drmEventContext evctx;
struct vbl_info handler_info;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
2014-04-09 11:40 ` [PATCH v2 libdrm 1/7] configure: Support symbol visibility when available Thierry Reding
2014-04-09 11:40 ` [PATCH v2 libdrm 2/7] libdrm: Add NVIDIA Tegra support Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:20 ` Erik Faye-Lund
2014-04-09 11:40 ` [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs Thierry Reding
` (3 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
This test opens a device, dumps the version information and checks that
a Tegra DRM context can be opened on it.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- fix in-tree build failure
configure.ac | 1 +
tests/Makefile.am | 4 ++++
tests/tegra/.gitignore | 1 +
tests/tegra/Makefile.am | 22 +++++++++++++++++
tests/tegra/openclose.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 91 insertions(+)
create mode 100644 tests/tegra/.gitignore
create mode 100644 tests/tegra/Makefile.am
create mode 100644 tests/tegra/openclose.c
diff --git a/configure.ac b/configure.ac
index 4cb6810bd6a8..e4cf70b1520b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -422,6 +422,7 @@ AC_CONFIG_FILES([
tests/radeon/Makefile
tests/vbltest/Makefile
tests/exynos/Makefile
+ tests/tegra/Makefile
include/Makefile
include/drm/Makefile
man/Makefile
diff --git a/tests/Makefile.am b/tests/Makefile.am
index cd1149130214..0a3d21f2d99f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -24,6 +24,10 @@ if HAVE_EXYNOS
SUBDIRS += exynos
endif
+if HAVE_TEGRA
+SUBDIRS += tegra
+endif
+
if HAVE_LIBUDEV
check_LTLIBRARIES = libdrmtest.la
diff --git a/tests/tegra/.gitignore b/tests/tegra/.gitignore
new file mode 100644
index 000000000000..5c5216c5c5e6
--- /dev/null
+++ b/tests/tegra/.gitignore
@@ -0,0 +1 @@
+openclose
diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
new file mode 100644
index 000000000000..8b481bde4f11
--- /dev/null
+++ b/tests/tegra/Makefile.am
@@ -0,0 +1,22 @@
+AM_CPPFLAGS = \
+ -I$(top_srcdir)/include/drm \
+ -I$(top_srcdir)/tegra \
+ -I$(top_srcdir)
+
+AM_CFLAGS = -Wall -Werror
+
+LDADD = \
+ ../../tegra/libdrm_tegra.la \
+ ../../libdrm.la
+
+TESTS = \
+ openclose \
+
+if HAVE_INSTALL_TESTS
+testdir = $(libexecdir)/libdrm/tests/tegra
+test_PROGRAMS = \
+ $(TESTS)
+else
+noinst_PROGRAMS = $(TESTS)
+check_PROGRAMS = $(TESTS)
+endif
diff --git a/tests/tegra/openclose.c b/tests/tegra/openclose.c
new file mode 100644
index 000000000000..5b4230c774f6
--- /dev/null
+++ b/tests/tegra/openclose.c
@@ -0,0 +1,63 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <unistd.h>
+
+#include "xf86drm.h"
+#include "tegra.h"
+
+int main(int argc, char *argv[])
+{
+ struct drm_tegra *tegra;
+ drmVersionPtr version;
+ int err, fd;
+
+ fd = open(argv[1], O_RDWR);
+ if (fd < 0)
+ return 1;
+
+ version = drmGetVersion(fd);
+ if (version) {
+ printf("Version: %d.%d.%d\n", version->version_major,
+ version->version_minor, version->version_patchlevel);
+ printf(" Name: %s\n", version->name);
+ printf(" Date: %s\n", version->date);
+ printf(" Description: %s\n", version->desc);
+
+ drmFreeVersion(version);
+ }
+
+ err = drm_tegra_new(&tegra, fd);
+ if (err < 0)
+ return 1;
+
+ drm_tegra_close(tegra);
+ close(fd);
+
+ return 0;
+}
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()
2014-04-09 11:40 ` [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open() Thierry Reding
@ 2014-04-10 17:20 ` Erik Faye-Lund
2014-04-28 19:49 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-10 17:20 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This test opens a device, dumps the version information and checks that
> a Tegra DRM context can be opened on it.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Looks good!
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()
2014-04-10 17:20 ` Erik Faye-Lund
@ 2014-04-28 19:49 ` Erik Faye-Lund
2014-05-02 14:17 ` Thierry Reding
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-28 19:49 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Thu, Apr 10, 2014 at 7:20 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> This test opens a device, dumps the version information and checks that
>> a Tegra DRM context can be opened on it.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>
> Looks good!
Actually, "make check" doesn't work here:
kusma@localhost:~/src/libdrm$ make -C tests/tegra/ check
make: Entering directory `/home/kusma/src/libdrm/tests/tegra'
make openclose gr2d-fill
make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
make[1]: `openclose' is up to date.
make[1]: `gr2d-fill' is up to date.
make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
make check-TESTS
make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
FAIL: openclose
failed to open DRM device (null): Bad address
FAIL: gr2d-fill
=======================================================================
2 of 2 tests failed
Please report to https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
=======================================================================
make[1]: *** [check-TESTS] Error 1
make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
make: *** [check-am] Error 2
make: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()
2014-04-28 19:49 ` Erik Faye-Lund
@ 2014-05-02 14:17 ` Thierry Reding
0 siblings, 0 replies; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:17 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 1656 bytes --]
On Mon, Apr 28, 2014 at 09:49:48PM +0200, Erik Faye-Lund wrote:
> On Thu, Apr 10, 2014 at 7:20 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> > On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> From: Thierry Reding <treding@nvidia.com>
> >>
> >> This test opens a device, dumps the version information and checks that
> >> a Tegra DRM context can be opened on it.
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> > Looks good!
>
> Actually, "make check" doesn't work here:
>
> kusma@localhost:~/src/libdrm$ make -C tests/tegra/ check
> make: Entering directory `/home/kusma/src/libdrm/tests/tegra'
> make openclose gr2d-fill
> make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
> make[1]: `openclose' is up to date.
> make[1]: `gr2d-fill' is up to date.
> make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
> make check-TESTS
> make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
> FAIL: openclose
> failed to open DRM device (null): Bad address
> FAIL: gr2d-fill
> =======================================================================
> 2 of 2 tests failed
> Please report to https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
> =======================================================================
> make[1]: *** [check-TESTS] Error 1
> make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
> make: *** [check-am] Error 2
> make: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
I've modified the tests to default to /dev/dri/card0 if no device was
specified on the command-line.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
` (2 preceding siblings ...)
2014-04-09 11:40 ` [PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open() Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:13 ` Erik Faye-Lund
2014-04-09 11:40 ` [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Thierry Reding
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
These functions can be used to open channels to engines, manage job
submissions, create push buffers to store command streams in and wait
until jobs have been completed.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- automatically allocate buffer objects as required by pushbuffers
- pushbuffers can now have more than one associated buffer object
- add drm_tegra_pushbuf_prepare() function
tegra/Makefile.am | 4 ++
tegra/channel.c | 127 +++++++++++++++++++++++++++++++++
tegra/fence.c | 71 +++++++++++++++++++
tegra/job.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++
tegra/private.h | 72 +++++++++++++++++++
tegra/pushbuf.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
tegra/tegra.c | 2 +
tegra/tegra.h | 52 ++++++++++++++
8 files changed, 713 insertions(+)
create mode 100644 tegra/channel.c
create mode 100644 tegra/fence.c
create mode 100644 tegra/job.c
create mode 100644 tegra/pushbuf.c
diff --git a/tegra/Makefile.am b/tegra/Makefile.am
index 1b83145b120d..c73587e8661e 100644
--- a/tegra/Makefile.am
+++ b/tegra/Makefile.am
@@ -11,6 +11,10 @@ libdrm_tegra_la_LDFLAGS = -version-number 0:0:0 -no-undefined
libdrm_tegra_la_LIBADD = ../libdrm.la @PTHREADSTUBS_LIBS@
libdrm_tegra_la_SOURCES = \
+ channel.c \
+ fence.c \
+ job.c \
+ pushbuf.c \
tegra.c
libdrm_tegraincludedir = ${includedir}/libdrm
diff --git a/tegra/channel.c b/tegra/channel.c
new file mode 100644
index 000000000000..3ab1d578f8e5
--- /dev/null
+++ b/tegra/channel.c
@@ -0,0 +1,127 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <string.h>
+
+#include "private.h"
+
+static int drm_tegra_channel_setup(struct drm_tegra_channel *channel)
+{
+ struct drm_tegra *drm = channel->drm;
+ struct drm_tegra_get_syncpt args;
+ int err;
+
+ memset(&args, 0, sizeof(args));
+ args.context = channel->context;
+ args.index = 0;
+
+ err = ioctl(drm->fd, DRM_IOCTL_TEGRA_GET_SYNCPT, &args);
+ if (err < 0)
+ return -errno;
+
+ channel->syncpt = args.id;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_channel_open(struct drm_tegra_channel **channelp,
+ struct drm_tegra *drm,
+ enum drm_tegra_class client)
+{
+ struct drm_tegra_open_channel args;
+ struct drm_tegra_channel *channel;
+ enum host1x_class class;
+ int err;
+
+ switch (client) {
+ case DRM_TEGRA_GR2D:
+ class = HOST1X_CLASS_GR2D;
+ break;
+
+ case DRM_TEGRA_GR3D:
+ class = HOST1X_CLASS_GR3D;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ channel = calloc(1, sizeof(*channel));
+ if (!channel)
+ return -ENOMEM;
+
+ channel->drm = drm;
+
+ memset(&args, 0, sizeof(args));
+ args.client = class;
+
+ err = ioctl(drm->fd, DRM_IOCTL_TEGRA_OPEN_CHANNEL, &args);
+ if (err < 0) {
+ free(channel);
+ return -errno;
+ }
+
+ channel->context = args.context;
+ channel->class = class;
+
+ err = drm_tegra_channel_setup(channel);
+ if (err < 0) {
+ free(channel);
+ return err;
+ }
+
+ *channelp = channel;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_channel_close(struct drm_tegra_channel *channel)
+{
+ struct drm_tegra_close_channel args;
+ struct drm_tegra *drm;
+ int err;
+
+ if (!channel)
+ return -EINVAL;
+
+ drm = channel->drm;
+
+ memset(&args, 0, sizeof(args));
+ args.context = channel->context;
+
+ err = ioctl(drm->fd, DRM_IOCTL_TEGRA_CLOSE_CHANNEL, &args);
+ if (err < 0)
+ return -errno;
+
+ free(channel);
+
+ return 0;
+}
diff --git a/tegra/fence.c b/tegra/fence.c
new file mode 100644
index 000000000000..f58725ca8472
--- /dev/null
+++ b/tegra/fence.c
@@ -0,0 +1,71 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <string.h>
+
+#include "private.h"
+
+drm_public
+int drm_tegra_fence_wait_timeout(struct drm_tegra_fence *fence,
+ unsigned long timeout)
+{
+ struct drm_tegra_syncpt_wait args;
+ int err;
+
+ memset(&args, 0, sizeof(args));
+ args.id = fence->syncpt;
+ args.thresh = fence->value;
+ args.timeout = timeout;
+
+ while (true) {
+ err = ioctl(fence->drm->fd, DRM_IOCTL_TEGRA_SYNCPT_WAIT, &args);
+ if (err < 0) {
+ if (errno == EINTR)
+ continue;
+
+ return -errno;
+ }
+
+ break;
+ }
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_fence_wait(struct drm_tegra_fence *fence)
+{
+ return drm_tegra_fence_wait_timeout(fence, -1);
+}
+
+drm_public
+void drm_tegra_fence_free(struct drm_tegra_fence *fence)
+{
+ free(fence);
+}
diff --git a/tegra/job.c b/tegra/job.c
new file mode 100644
index 000000000000..8df643c9a9bb
--- /dev/null
+++ b/tegra/job.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "private.h"
+
+int drm_tegra_job_add_reloc(struct drm_tegra_job *job,
+ const struct drm_tegra_reloc *reloc)
+{
+ struct drm_tegra_reloc *relocs;
+ size_t size;
+
+ size = (job->num_relocs + 1) * sizeof(*reloc);
+
+ relocs = realloc(job->relocs, size);
+ if (!reloc)
+ return -ENOMEM;
+
+ job->relocs = relocs;
+
+ job->relocs[job->num_relocs++] = *reloc;
+
+ return 0;
+}
+
+int drm_tegra_job_add_cmdbuf(struct drm_tegra_job *job,
+ const struct drm_tegra_cmdbuf *cmdbuf)
+{
+ struct drm_tegra_cmdbuf *cmdbufs;
+ size_t size;
+
+ size = (job->num_cmdbufs + 1) * sizeof(*cmdbuf);
+
+ cmdbufs = realloc(job->cmdbufs, size);
+ if (!cmdbufs)
+ return -ENOMEM;
+
+ cmdbufs[job->num_cmdbufs++] = *cmdbuf;
+ job->cmdbufs = cmdbufs;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_job_new(struct drm_tegra_job **jobp,
+ struct drm_tegra_channel *channel)
+{
+ struct drm_tegra_job *job;
+
+ job = calloc(1, sizeof(*job));
+ if (!job)
+ return -ENOMEM;
+
+ DRMINITLISTHEAD(&job->pushbufs);
+ job->channel = channel;
+
+ *jobp = job;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_job_free(struct drm_tegra_job *job)
+{
+ struct drm_tegra_pushbuf_private *pushbuf;
+
+ if (!job)
+ return -EINVAL;
+
+ DRMLISTFOREACHENTRY(pushbuf, &job->pushbufs, list)
+ drm_tegra_pushbuf_free(&pushbuf->base);
+
+ free(job->cmdbufs);
+ free(job->relocs);
+ free(job);
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_job_submit(struct drm_tegra_job *job,
+ struct drm_tegra_fence **fencep)
+{
+ struct drm_tegra *drm = job->channel->drm;
+ struct drm_tegra_pushbuf_private *pushbuf;
+ struct drm_tegra_fence *fence = NULL;
+ struct drm_tegra_cmdbuf *cmdbufs;
+ struct drm_tegra_syncpt *syncpts;
+ struct drm_tegra_submit args;
+ unsigned int i;
+ int err;
+
+ /*
+ * Make sure the current command stream buffer is queued for
+ * submission.
+ */
+ err = drm_tegra_pushbuf_queue(job->pushbuf);
+ if (err < 0)
+ return err;
+
+ job->pushbuf = NULL;
+
+ if (fencep) {
+ fence = calloc(1, sizeof(*fence));
+ if (!fence)
+ return -ENOMEM;
+ }
+
+ syncpts = calloc(1, sizeof(*syncpts));
+ if (!syncpts) {
+ free(cmdbufs);
+ free(fence);
+ return -ENOMEM;
+ }
+
+ syncpts[0].id = job->syncpt;
+ syncpts[0].incrs = job->increments;
+
+ memset(&args, 0, sizeof(args));
+ args.context = job->channel->context;
+ args.num_syncpts = 1;
+ args.num_cmdbufs = job->num_cmdbufs;
+ args.num_relocs = job->num_relocs;
+ args.num_waitchks = 0;
+ args.waitchk_mask = 0;
+ args.timeout = 1000;
+
+ args.syncpts = (uintptr_t)syncpts;
+ args.cmdbufs = (uintptr_t)job->cmdbufs;
+ args.relocs = (uintptr_t)job->relocs;
+ args.waitchks = 0;
+
+ err = ioctl(drm->fd, DRM_IOCTL_TEGRA_SUBMIT, &args);
+ if (err < 0) {
+ free(syncpts);
+ free(cmdbufs);
+ free(fence);
+ return -errno;
+ }
+
+ if (fence) {
+ fence->syncpt = job->syncpt;
+ fence->value = args.fence;
+ fence->drm = drm;
+ *fencep = fence;
+ }
+
+ free(syncpts);
+ free(cmdbufs);
+
+ return 0;
+}
diff --git a/tegra/private.h b/tegra/private.h
index 9b6bc9395d23..fc74fb56b58d 100644
--- a/tegra/private.h
+++ b/tegra/private.h
@@ -26,13 +26,31 @@
#define __DRM_TEGRA_PRIVATE_H__ 1
#include <stdbool.h>
+#include <stddef.h>
#include <stdint.h>
#include <libdrm.h>
+#include <libdrm_lists.h>
#include <xf86atomic.h>
+#include "tegra_drm.h"
#include "tegra.h"
+#define container_of(ptr, type, member) ({ \
+ const typeof(((type *)0)->member) *__mptr = (ptr); \
+ (type *)((char *)__mptr - offsetof(type, member)); \
+ })
+
+#define align(offset, align) \
+ (((offset) + (align) - 1) & ~((align) - 1))
+
+enum host1x_class {
+ HOST1X_CLASS_HOST1X = 0x01,
+ HOST1X_CLASS_GR2D = 0x51,
+ HOST1X_CLASS_GR2D_SB = 0x52,
+ HOST1X_CLASS_GR3D = 0x60,
+};
+
struct drm_tegra {
bool close;
int fd;
@@ -40,6 +58,7 @@ struct drm_tegra {
struct drm_tegra_bo {
struct drm_tegra *drm;
+ drmMMListHead list;
uint32_t handle;
uint32_t offset;
uint32_t flags;
@@ -48,4 +67,57 @@ struct drm_tegra_bo {
void *map;
};
+struct drm_tegra_channel {
+ struct drm_tegra *drm;
+ enum host1x_class class;
+ uint64_t context;
+ uint32_t syncpt;
+};
+
+struct drm_tegra_fence {
+ struct drm_tegra *drm;
+ uint32_t syncpt;
+ uint32_t value;
+};
+
+struct drm_tegra_pushbuf_private {
+ struct drm_tegra_pushbuf base;
+ struct drm_tegra_job *job;
+ drmMMListHead list;
+ drmMMListHead bos;
+
+ struct drm_tegra_bo *bo;
+ uint32_t *start;
+ uint32_t *end;
+};
+
+static inline struct drm_tegra_pushbuf_private *
+pushbuf_priv(struct drm_tegra_pushbuf *pb)
+{
+ return container_of(pb, struct drm_tegra_pushbuf_private, base);
+}
+
+int drm_tegra_pushbuf_queue(struct drm_tegra_pushbuf_private *pushbuf);
+
+struct drm_tegra_job {
+ struct drm_tegra_channel *channel;
+
+ unsigned int increments;
+ uint32_t syncpt;
+
+ struct drm_tegra_reloc *relocs;
+ unsigned int num_relocs;
+
+ struct drm_tegra_cmdbuf *cmdbufs;
+ unsigned int num_cmdbufs;
+
+ struct drm_tegra_pushbuf_private *pushbuf;
+ drmMMListHead pushbufs;
+};
+
+int drm_tegra_job_add_reloc(struct drm_tegra_job *job,
+ const struct drm_tegra_reloc *reloc);
+int drm_tegra_job_add_cmdbuf(struct drm_tegra_job *job,
+ const struct drm_tegra_cmdbuf *cmdbuf);
+
#endif /* __DRM_TEGRA_PRIVATE_H__ */
diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
new file mode 100644
index 000000000000..178d5cd57541
--- /dev/null
+++ b/tegra/pushbuf.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright © 2012, 2013 Thierry Reding
+ * Copyright © 2013 Erik Faye-Lund
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "private.h"
+
+#define HOST1X_OPCODE_NONINCR(offset, count) \
+ ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
+
+int drm_tegra_pushbuf_queue(struct drm_tegra_pushbuf_private *pushbuf)
+{
+ struct drm_tegra_cmdbuf cmdbuf;
+ int err;
+
+ if (!pushbuf || !pushbuf->bo)
+ return 0;
+
+ /* unmap buffer object since it won't be accessed anymore */
+ drm_tegra_bo_unmap(pushbuf->bo);
+
+ /* add buffer object as command buffers for this job */
+ memset(&cmdbuf, 0, sizeof(cmdbuf));
+ cmdbuf.words = pushbuf->base.ptr - pushbuf->start;
+ cmdbuf.handle = pushbuf->bo->handle;
+ cmdbuf.offset = 0;
+
+ err = drm_tegra_job_add_cmdbuf(pushbuf->job, &cmdbuf);
+ if (err < 0)
+ return err;
+
+ return 0;
+}
+
+static inline unsigned long
+drm_tegra_pushbuf_get_offset(struct drm_tegra_pushbuf *pushbuf)
+{
+ struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
+
+ return (unsigned long)pushbuf->ptr - (unsigned long)priv->start;
+}
+
+drm_public
+int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
+ struct drm_tegra_job *job)
+{
+ struct drm_tegra_pushbuf_private *pushbuf;
+ void *ptr;
+ int err;
+
+ pushbuf = calloc(1, sizeof(*pushbuf));
+ if (!pushbuf)
+ return -ENOMEM;
+
+ DRMINITLISTHEAD(&pushbuf->list);
+ DRMINITLISTHEAD(&pushbuf->bos);
+ pushbuf->job = job;
+
+ *pushbufp = &pushbuf->base;
+
+ DRMLISTADD(&pushbuf->list, &job->pushbufs);
+ job->pushbuf = pushbuf;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf)
+{
+ struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
+ struct drm_tegra_bo *bo, *tmp;
+
+ if (!pushbuf)
+ return -EINVAL;
+
+ drm_tegra_bo_unmap(priv->bo);
+
+ DRMLISTFOREACHENTRYSAFE(bo, tmp, &priv->bos, list)
+ drm_tegra_bo_put(priv->bo);
+
+ DRMLISTDEL(&priv->list);
+ free(priv);
+
+ return 0;
+}
+
+/**
+ * drm_tegra_pushbuf_prepare() - prepare push buffer for a series of pushes
+ * @pushbuf: push buffer
+ * @words: maximum number of words in series of pushes to follow
+ */
+drm_public
+int drm_tegra_pushbuf_prepare(struct drm_tegra_pushbuf *pushbuf,
+ unsigned int words)
+{
+ struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
+ struct drm_tegra_channel *channel = priv->job->channel;
+ struct drm_tegra_bo *bo;
+ void *ptr;
+ int err;
+
+ if (priv->bo && (pushbuf->ptr + words < priv->end))
+ return 0;
+
+ /*
+ * Align to full pages, since buffer object allocations are page
+ * granular anyway.
+ */
+ words = align(words, 1024);
+
+ err = drm_tegra_bo_new(&bo, channel->drm, 0, words * sizeof(uint32_t));
+ if (err < 0)
+ return err;
+
+ err = drm_tegra_bo_map(bo, &ptr);
+ if (err < 0) {
+ drm_tegra_bo_put(bo);
+ return err;
+ }
+
+ /* queue current command stream buffer for submission */
+ err = drm_tegra_pushbuf_queue(priv);
+ if (err < 0) {
+ drm_tegra_bo_unmap(bo);
+ drm_tegra_bo_put(bo);
+ return err;
+ }
+
+ DRMLISTADD(&bo->list, &priv->bos);
+
+ priv->start = priv->base.ptr = ptr;
+ priv->end = priv->start + bo->size;
+ priv->bo = bo;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_pushbuf_relocate(struct drm_tegra_pushbuf *pushbuf,
+ struct drm_tegra_bo *target,
+ unsigned long offset,
+ unsigned long shift)
+{
+ struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
+ struct drm_tegra_reloc reloc;
+ int err;
+
+ memset(&reloc, 0, sizeof(reloc));
+ reloc.cmdbuf.handle = priv->bo->handle;
+ reloc.cmdbuf.offset = drm_tegra_pushbuf_get_offset(pushbuf);
+ reloc.target.handle = target->handle;
+ reloc.target.offset = offset;
+ reloc.shift = shift;
+
+ err = drm_tegra_job_add_reloc(priv->job, &reloc);
+ if (err < 0)
+ return err;
+
+ *pushbuf->ptr++ = 0xdeadbeef;
+
+ return 0;
+}
+
+drm_public
+int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
+ enum drm_tegra_syncpt_cond cond)
+{
+ struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
+
+ if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
+ return -EINVAL;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
+ *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;
+ priv->job->increments++;
+
+ return 0;
+}
diff --git a/tegra/tegra.c b/tegra/tegra.c
index bf6d035453d5..1f7b9e7345b6 100644
--- a/tegra/tegra.c
+++ b/tegra/tegra.c
@@ -56,6 +56,7 @@ static void drm_tegra_bo_free(struct drm_tegra_bo *bo)
drmIoctl(drm->fd, DRM_IOCTL_GEM_CLOSE, &args);
+ DRMLISTDEL(&bo->list);
free(bo);
}
@@ -127,6 +128,7 @@ int drm_tegra_bo_new(struct drm_tegra_bo **bop, struct drm_tegra *drm,
if (!bo)
return -ENOMEM;
+ DRMINITLISTHEAD(&bo->list);
atomic_set(&bo->ref, 1);
bo->flags = flags;
bo->size = size;
diff --git a/tegra/tegra.h b/tegra/tegra.h
index 0731cb3bd4dc..4cf3b49a3e3e 100644
--- a/tegra/tegra.h
+++ b/tegra/tegra.h
@@ -28,6 +28,13 @@
#include <stdint.h>
#include <stdlib.h>
+#include <tegra_drm.h>
+
+enum drm_tegra_class {
+ DRM_TEGRA_GR2D,
+ DRM_TEGRA_GR3D,
+};
+
struct drm_tegra_bo;
struct drm_tegra;
@@ -44,4 +51,49 @@ int drm_tegra_bo_get_handle(struct drm_tegra_bo *bo, uint32_t *handle);
int drm_tegra_bo_map(struct drm_tegra_bo *bo, void **ptr);
int drm_tegra_bo_unmap(struct drm_tegra_bo *bo);
+struct drm_tegra_channel;
+struct drm_tegra_job;
+
+struct drm_tegra_pushbuf {
+ uint32_t *ptr;
+};
+
+struct drm_tegra_fence;
+
+enum drm_tegra_syncpt_cond {
+ DRM_TEGRA_SYNCPT_COND_IMMEDIATE,
+ DRM_TEGRA_SYNCPT_COND_OP_DONE,
+ DRM_TEGRA_SYNCPT_COND_RD_DONE,
+ DRM_TEGRA_SYNCPT_COND_WR_SAFE,
+ DRM_TEGRA_SYNCPT_COND_MAX,
+};
+
+int drm_tegra_channel_open(struct drm_tegra_channel **channelp,
+ struct drm_tegra *drm,
+ enum drm_tegra_class client);
+int drm_tegra_channel_close(struct drm_tegra_channel *channel);
+
+int drm_tegra_job_new(struct drm_tegra_job **jobp,
+ struct drm_tegra_channel *channel);
+int drm_tegra_job_free(struct drm_tegra_job *job);
+int drm_tegra_job_submit(struct drm_tegra_job *job,
+ struct drm_tegra_fence **fencep);
+
+int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
+ struct drm_tegra_job *job);
+int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf);
+int drm_tegra_pushbuf_prepare(struct drm_tegra_pushbuf *pushbuf,
+ unsigned int words);
+int drm_tegra_pushbuf_relocate(struct drm_tegra_pushbuf *pushbuf,
+ struct drm_tegra_bo *target,
+ unsigned long offset,
+ unsigned long shift);
+int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
+ enum drm_tegra_syncpt_cond cond);
+
+int drm_tegra_fence_wait_timeout(struct drm_tegra_fence *fence,
+ unsigned long timeout);
+int drm_tegra_fence_wait(struct drm_tegra_fence *fence);
+void drm_tegra_fence_free(struct drm_tegra_fence *fence);
+
#endif /* __DRM_TEGRA_H__ */
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-04-09 11:40 ` [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs Thierry Reding
@ 2014-04-10 17:13 ` Erik Faye-Lund
2014-05-02 14:06 ` Thierry Reding
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-10 17:13 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> diff --git a/tegra/fence.c b/tegra/fence.c
> new file mode 100644
> index 000000000000..f58725ca8472
> --- /dev/null
> +++ b/tegra/fence.c
> +drm_public
> +int drm_tegra_fence_wait(struct drm_tegra_fence *fence)
> +{
> + return drm_tegra_fence_wait_timeout(fence, -1);
> +}
Perhaps a convenience-wrapper like this should be inline in the header
to reduce function-call overhead?
> +drm_public
> +int drm_tegra_job_submit(struct drm_tegra_job *job,
> + struct drm_tegra_fence **fencep)
> +{
> + struct drm_tegra *drm = job->channel->drm;
> + struct drm_tegra_pushbuf_private *pushbuf;
> + struct drm_tegra_fence *fence = NULL;
> + struct drm_tegra_cmdbuf *cmdbufs;
> + struct drm_tegra_syncpt *syncpts;
> + struct drm_tegra_submit args;
> + unsigned int i;
> + int err;
> +
> + /*
> + * Make sure the current command stream buffer is queued for
> + * submission.
> + */
> + err = drm_tegra_pushbuf_queue(job->pushbuf);
> + if (err < 0)
> + return err;
> +
> + job->pushbuf = NULL;
> +
> + if (fencep) {
> + fence = calloc(1, sizeof(*fence));
> + if (!fence)
> + return -ENOMEM;
> + }
> +
> + syncpts = calloc(1, sizeof(*syncpts));
> + if (!syncpts) {
> + free(cmdbufs);
cmdbufs never gets assigned to, yet it gets free'd here (and a few
more places further down). I guess this is left-overs from the
previous round that should just die?
But this raises the question, how is job->cmdbufs (and job->relocs)
supposed to get free'd?
I'm a bit curious as to what the expected life-time of these objects
are. Do I need to create a new job-object for each submit, or can I
reuse a job? I think the latter would be preferable... So perhaps we
should have a drm_tegra_job_reset that allows us to throw away the
accumulated cmdbufs and relocs, and start building a new job?
> diff --git a/tegra/private.h b/tegra/private.h
> index 9b6bc9395d23..fc74fb56b58d 100644
> --- a/tegra/private.h
> +++ b/tegra/private.h
> @@ -26,13 +26,31 @@
> #define __DRM_TEGRA_PRIVATE_H__ 1
>
> #include <stdbool.h>
> +#include <stddef.h>
> #include <stdint.h>
>
> #include <libdrm.h>
> +#include <libdrm_lists.h>
> #include <xf86atomic.h>
>
> +#include "tegra_drm.h"
> #include "tegra.h"
>
> +#define container_of(ptr, type, member) ({ \
> + const typeof(((type *)0)->member) *__mptr = (ptr); \
> + (type *)((char *)__mptr - offsetof(type, member)); \
> + })
> +
<snip>
> +
> +struct drm_tegra_pushbuf_private {
> + struct drm_tegra_pushbuf base;
> + struct drm_tegra_job *job;
> + drmMMListHead list;
> + drmMMListHead bos;
> +
> + struct drm_tegra_bo *bo;
> + uint32_t *start;
> + uint32_t *end;
> +};
> +
> +static inline struct drm_tegra_pushbuf_private *
> +pushbuf_priv(struct drm_tegra_pushbuf *pb)
> +{
> + return container_of(pb, struct drm_tegra_pushbuf_private, base);
> +}
> +
This seems to be the only use-case for container_of, and it just
happens to return the exact same pointer as was passed in... so do we
really need that helper?
> diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
> new file mode 100644
> index 000000000000..178d5cd57541
> --- /dev/null
> +++ b/tegra/pushbuf.c
> @@ -0,0 +1,205 @@
<snip>
> +drm_public
> +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> + struct drm_tegra_job *job)
> +{
> + struct drm_tegra_pushbuf_private *pushbuf;
> + void *ptr;
> + int err;
> +
> + pushbuf = calloc(1, sizeof(*pushbuf));
> + if (!pushbuf)
> + return -ENOMEM;
> +
> + DRMINITLISTHEAD(&pushbuf->list);
> + DRMINITLISTHEAD(&pushbuf->bos);
> + pushbuf->job = job;
> +
> + *pushbufp = &pushbuf->base;
> +
> + DRMLISTADD(&pushbuf->list, &job->pushbufs);
Hmm. So the job keeps a list of pushbufs. What purpose does this
serve? Shouldn't the job only need the cmdbuf-list (which it already
has) and the BOs (so they can be dereferenced after being submitted)?
AFAICT, we could make drm_tegra_pushbuf_queue append the BO to a list
in the job instead. That way we don't need to keep all the
pushbuf-objects around, and we might even be able to reuse the same
object rather than keep reallocating them. No?
> +drm_public
> +int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
> + enum drm_tegra_syncpt_cond cond)
> +{
> + struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
> +
> + if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
> + return -EINVAL;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
> + *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;
Perhaps "HOST1X_OPCODE_IMM(0x0, cond << 8 | priv->job->syncpt)", which
saves a word?
Either way, I think this should either call drm_tegra_pushbuf_prepare
to make sure two words are available, or be renamed to reflect that it
causes a push (or two, as in the current form).
> +struct drm_tegra_channel;
> +struct drm_tegra_job;
> +
> +struct drm_tegra_pushbuf {
> + uint32_t *ptr;
> +};
Hmm, single-member structs always makes me cringe a bit. But in this
case it's much better than a double-pointer IMO.
But perhaps some of the members in the private-struct might be useful
out here? For instance, if "uint32_t *end;" was also available, we
could do:
inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
*pushbuf, uint32_t value)
{
assert(pushbuf->ptr < pushbuf->end);
*pushbuf->ptr++ = value;
}
...and easily pick up bugs with too few words? The helper would of
course be in the header-file, so the write gets inlined nicely.
But all in all, a really nice read. Thanks!
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-04-10 17:13 ` Erik Faye-Lund
@ 2014-05-02 14:06 ` Thierry Reding
2014-05-02 14:53 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:06 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 8467 bytes --]
On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > diff --git a/tegra/fence.c b/tegra/fence.c
> > new file mode 100644
> > index 000000000000..f58725ca8472
> > --- /dev/null
> > +++ b/tegra/fence.c
> > +drm_public
> > +int drm_tegra_fence_wait(struct drm_tegra_fence *fence)
> > +{
> > + return drm_tegra_fence_wait_timeout(fence, -1);
> > +}
>
> Perhaps a convenience-wrapper like this should be inline in the header
> to reduce function-call overhead?
Okay, I've moved this into the tegra.h header file.
>
> > +drm_public
> > +int drm_tegra_job_submit(struct drm_tegra_job *job,
> > + struct drm_tegra_fence **fencep)
> > +{
> > + struct drm_tegra *drm = job->channel->drm;
> > + struct drm_tegra_pushbuf_private *pushbuf;
> > + struct drm_tegra_fence *fence = NULL;
> > + struct drm_tegra_cmdbuf *cmdbufs;
> > + struct drm_tegra_syncpt *syncpts;
> > + struct drm_tegra_submit args;
> > + unsigned int i;
> > + int err;
> > +
> > + /*
> > + * Make sure the current command stream buffer is queued for
> > + * submission.
> > + */
> > + err = drm_tegra_pushbuf_queue(job->pushbuf);
> > + if (err < 0)
> > + return err;
> > +
> > + job->pushbuf = NULL;
> > +
> > + if (fencep) {
> > + fence = calloc(1, sizeof(*fence));
> > + if (!fence)
> > + return -ENOMEM;
> > + }
> > +
> > + syncpts = calloc(1, sizeof(*syncpts));
> > + if (!syncpts) {
> > + free(cmdbufs);
>
> cmdbufs never gets assigned to, yet it gets free'd here (and a few
> more places further down). I guess this is left-overs from the
> previous round that should just die?
Indeed. pushbuf was also unused to I removed it as well.
> But this raises the question, how is job->cmdbufs (and job->relocs)
> supposed to get free'd?
>
> I'm a bit curious as to what the expected life-time of these objects
> are. Do I need to create a new job-object for each submit, or can I
> reuse a job? I think the latter would be preferable... So perhaps we
> should have a drm_tegra_job_reset that allows us to throw away the
> accumulated cmdbufs and relocs, and start building a new job?
They are currently freed by drm_tegra_job_free(), so yes, the current
intended usage is to create a new job for each submit. Do you mind if we
keep your proposal for a drm_tegra_job_reset() in mind for a follow-up
patch. It's an optimization that we can easily add later on and I'd like
to avoid too much premature optimization at this point.
> > diff --git a/tegra/private.h b/tegra/private.h
> > index 9b6bc9395d23..fc74fb56b58d 100644
> > --- a/tegra/private.h
> > +++ b/tegra/private.h
> > @@ -26,13 +26,31 @@
> > #define __DRM_TEGRA_PRIVATE_H__ 1
> >
> > #include <stdbool.h>
> > +#include <stddef.h>
> > #include <stdint.h>
> >
> > #include <libdrm.h>
> > +#include <libdrm_lists.h>
> > #include <xf86atomic.h>
> >
> > +#include "tegra_drm.h"
> > #include "tegra.h"
> >
> > +#define container_of(ptr, type, member) ({ \
> > + const typeof(((type *)0)->member) *__mptr = (ptr); \
> > + (type *)((char *)__mptr - offsetof(type, member)); \
> > + })
> > +
> <snip>
> > +
> > +struct drm_tegra_pushbuf_private {
> > + struct drm_tegra_pushbuf base;
> > + struct drm_tegra_job *job;
> > + drmMMListHead list;
> > + drmMMListHead bos;
> > +
> > + struct drm_tegra_bo *bo;
> > + uint32_t *start;
> > + uint32_t *end;
> > +};
> > +
> > +static inline struct drm_tegra_pushbuf_private *
> > +pushbuf_priv(struct drm_tegra_pushbuf *pb)
> > +{
> > + return container_of(pb, struct drm_tegra_pushbuf_private, base);
> > +}
> > +
>
> This seems to be the only use-case for container_of, and it just
> happens to return the exact same pointer as was passed in... so do we
> really need that helper?
It has the benefit of keeping this code working even when somebody
suddenly adds to the beginning of drm_tegra_pushbuf_private. I'd rather
not resolve to force casting just to be on the safe side.
> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
> > new file mode 100644
> > index 000000000000..178d5cd57541
> > --- /dev/null
> > +++ b/tegra/pushbuf.c
> > @@ -0,0 +1,205 @@
> <snip>
> > +drm_public
> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> > + struct drm_tegra_job *job)
> > +{
> > + struct drm_tegra_pushbuf_private *pushbuf;
> > + void *ptr;
> > + int err;
> > +
> > + pushbuf = calloc(1, sizeof(*pushbuf));
> > + if (!pushbuf)
> > + return -ENOMEM;
> > +
> > + DRMINITLISTHEAD(&pushbuf->list);
> > + DRMINITLISTHEAD(&pushbuf->bos);
> > + pushbuf->job = job;
> > +
> > + *pushbufp = &pushbuf->base;
> > +
> > + DRMLISTADD(&pushbuf->list, &job->pushbufs);
>
> Hmm. So the job keeps a list of pushbufs. What purpose does this
> serve? Shouldn't the job only need the cmdbuf-list (which it already
> has) and the BOs (so they can be dereferenced after being submitted)?
This is currently used to keep track of existing pushbuffers and make
sure that they are freed (when the job is freed).
> AFAICT, we could make drm_tegra_pushbuf_queue append the BO to a list
> in the job instead. That way we don't need to keep all the
> pushbuf-objects around, and we might even be able to reuse the same
> object rather than keep reallocating them. No?
I guess we could make drm_tegra_pushbuf_new() reinitialize the current
pushbuf object and always return the same. But perhaps there are
occasions where it's useful to keep a few of them around?
If reusing the pushbuf objects makes sense, then perhaps a different API
would be more useful. Rather than allocate in the context of a job, they
could be allocated in a channel-wide context and added to/associated
with a job only as needed.
> > +drm_public
> > +int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
> > + enum drm_tegra_syncpt_cond cond)
> > +{
> > + struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
> > +
> > + if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
> > + return -EINVAL;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
> > + *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;
>
> Perhaps "HOST1X_OPCODE_IMM(0x0, cond << 8 | priv->job->syncpt)", which
> saves a word?
Interesting. Have you ever tested whether that works properly that way?
I've never seen that in a command stream from the binary driver.
> Either way, I think this should either call drm_tegra_pushbuf_prepare
> to make sure two words are available, or be renamed to reflect that it
> causes a push (or two, as in the current form).
I've added a call to drm_tegra_pushbuf_prepare() here and in
drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
> > +struct drm_tegra_channel;
> > +struct drm_tegra_job;
> > +
> > +struct drm_tegra_pushbuf {
> > + uint32_t *ptr;
> > +};
>
> Hmm, single-member structs always makes me cringe a bit. But in this
> case it's much better than a double-pointer IMO.
>
> But perhaps some of the members in the private-struct might be useful
> out here? For instance, if "uint32_t *end;" was also available, we
> could do:
>
> inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
> *pushbuf, uint32_t value)
> {
> assert(pushbuf->ptr < pushbuf->end);
> *pushbuf->ptr++ = value;
> }
>
> ...and easily pick up bugs with too few words? The helper would of
> course be in the header-file, so the write gets inlined nicely.
Sounds good. If you have no strong objections, I'd like to keep that for
a follow on patch when there's more code that actively uses this API. It
should be fairly safe to make more members public via drm_tegra_pushbuf
rather than the other way around, so I'd like to err on the side of
carefulness for a bit longer.
Thanks for the review, and apologies for being sluggish.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-05-02 14:06 ` Thierry Reding
@ 2014-05-02 14:53 ` Erik Faye-Lund
2014-05-02 15:16 ` Thierry Reding
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 14:53 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Fri, May 2, 2014 at 4:06 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
>>
>> But this raises the question, how is job->cmdbufs (and job->relocs)
>> supposed to get free'd?
>>
>> I'm a bit curious as to what the expected life-time of these objects
>> are. Do I need to create a new job-object for each submit, or can I
>> reuse a job? I think the latter would be preferable... So perhaps we
>> should have a drm_tegra_job_reset that allows us to throw away the
>> accumulated cmdbufs and relocs, and start building a new job?
>
> They are currently freed by drm_tegra_job_free(), so yes, the current
> intended usage is to create a new job for each submit. Do you mind if we
> keep your proposal for a drm_tegra_job_reset() in mind for a follow-up
> patch. It's an optimization that we can easily add later on and I'd like
> to avoid too much premature optimization at this point.
Sure, I don't mind.
>> > diff --git a/tegra/private.h b/tegra/private.h
>> > index 9b6bc9395d23..fc74fb56b58d 100644
>> > --- a/tegra/private.h
>> > +++ b/tegra/private.h
>> > @@ -26,13 +26,31 @@
>> > #define __DRM_TEGRA_PRIVATE_H__ 1
>> >
>> > #include <stdbool.h>
>> > +#include <stddef.h>
>> > #include <stdint.h>
>> >
>> > #include <libdrm.h>
>> > +#include <libdrm_lists.h>
>> > #include <xf86atomic.h>
>> >
>> > +#include "tegra_drm.h"
>> > #include "tegra.h"
>> >
>> > +#define container_of(ptr, type, member) ({ \
>> > + const typeof(((type *)0)->member) *__mptr = (ptr); \
>> > + (type *)((char *)__mptr - offsetof(type, member)); \
>> > + })
>> > +
>> <snip>
>> > +
>> > +struct drm_tegra_pushbuf_private {
>> > + struct drm_tegra_pushbuf base;
>> > + struct drm_tegra_job *job;
>> > + drmMMListHead list;
>> > + drmMMListHead bos;
>> > +
>> > + struct drm_tegra_bo *bo;
>> > + uint32_t *start;
>> > + uint32_t *end;
>> > +};
>> > +
>> > +static inline struct drm_tegra_pushbuf_private *
>> > +pushbuf_priv(struct drm_tegra_pushbuf *pb)
>> > +{
>> > + return container_of(pb, struct drm_tegra_pushbuf_private, base);
>> > +}
>> > +
>>
>> This seems to be the only use-case for container_of, and it just
>> happens to return the exact same pointer as was passed in... so do we
>> really need that helper?
>
> It has the benefit of keeping this code working even when somebody
> suddenly adds to the beginning of drm_tegra_pushbuf_private. I'd rather
> not resolve to force casting just to be on the safe side.
Is that a really a legitimate worry? There's a very clear relationship
between the two structures. Both nouveau and freedreno does the same
thing (for nouveau_bo_priv, nouveau_device_priv, msm_device, msm_pipe
and msm_bo structures), and it seems to work fine for them. Perhaps a
comment along the lines of "/* this needs to be the first member */"
is enough to discourage people from messing with it?
>> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
>> > new file mode 100644
>> > index 000000000000..178d5cd57541
>> > --- /dev/null
>> > +++ b/tegra/pushbuf.c
>> > @@ -0,0 +1,205 @@
>> <snip>
>> > +drm_public
>> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
>> > + struct drm_tegra_job *job)
>> > +{
>> > + struct drm_tegra_pushbuf_private *pushbuf;
>> > + void *ptr;
>> > + int err;
>> > +
>> > + pushbuf = calloc(1, sizeof(*pushbuf));
>> > + if (!pushbuf)
>> > + return -ENOMEM;
>> > +
>> > + DRMINITLISTHEAD(&pushbuf->list);
>> > + DRMINITLISTHEAD(&pushbuf->bos);
>> > + pushbuf->job = job;
>> > +
>> > + *pushbufp = &pushbuf->base;
>> > +
>> > + DRMLISTADD(&pushbuf->list, &job->pushbufs);
>>
>> Hmm. So the job keeps a list of pushbufs. What purpose does this
>> serve? Shouldn't the job only need the cmdbuf-list (which it already
>> has) and the BOs (so they can be dereferenced after being submitted)?
>
> This is currently used to keep track of existing pushbuffers and make
> sure that they are freed (when the job is freed).
>
OK, it seems to me that the need for keeping the pushbuffers around is
simply not there. Only the BOs needs to be kept around, no?
>> AFAICT, we could make drm_tegra_pushbuf_queue append the BO to a list
>> in the job instead. That way we don't need to keep all the
>> pushbuf-objects around, and we might even be able to reuse the same
>> object rather than keep reallocating them. No?
>
> I guess we could make drm_tegra_pushbuf_new() reinitialize the current
> pushbuf object and always return the same. But perhaps there are
> occasions where it's useful to keep a few of them around?
This sounds a bit hacky (and potentially hazardous, depending on the
client). I'd rather have clear semantics, and flexibility for the
clients to do what suits it best, as long as the cost is negligible.
> If reusing the pushbuf objects makes sense, then perhaps a different API
> would be more useful. Rather than allocate in the context of a job, they
> could be allocated in a channel-wide context and added to/associated
> with a job only as needed.
Yes, I think this might make sense. I'll have to ponder a bit more
about this, though.
>> > +drm_public
>> > +int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
>> > + enum drm_tegra_syncpt_cond cond)
>> > +{
>> > + struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
>> > +
>> > + if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
>> > + return -EINVAL;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
>> > + *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;
>>
>> Perhaps "HOST1X_OPCODE_IMM(0x0, cond << 8 | priv->job->syncpt)", which
>> saves a word?
>
> Interesting. Have you ever tested whether that works properly that way?
> I've never seen that in a command stream from the binary driver.
No, I haven't. But from the documentation, there's nothing to suggest
it wouldn't work.
I'll give it a spin later to verify!
>> Either way, I think this should either call drm_tegra_pushbuf_prepare
>> to make sure two words are available, or be renamed to reflect that it
>> causes a push (or two, as in the current form).
>
> I've added a call to drm_tegra_pushbuf_prepare() here and in
> drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
>
Actually, I think drm_tegra_pushbuf_relocate should just be renamed to
drm_tegra_pushbuf_push_reloc(), as that conveys quite clearly that it
is just a push-helper and need to be counted when preparing. That way
it doesn't need to prepare itself.
The reason why I think this is better for that function, is that
relocs will naturally appear in the middle of other pushes, so it
makes sense to perpare them just like the other pushes. I *think*
drm_tegra_pushbuf_sync is only really useful when switching between
GR2D and GR3D, so it's not such a big concern. But I could be wrong,
dunno. Perhaps it's also useful when texturing with a newly rendered
FBO also.
>> > +struct drm_tegra_channel;
>> > +struct drm_tegra_job;
>> > +
>> > +struct drm_tegra_pushbuf {
>> > + uint32_t *ptr;
>> > +};
>>
>> Hmm, single-member structs always makes me cringe a bit. But in this
>> case it's much better than a double-pointer IMO.
>>
>> But perhaps some of the members in the private-struct might be useful
>> out here? For instance, if "uint32_t *end;" was also available, we
>> could do:
>>
>> inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
>> *pushbuf, uint32_t value)
>> {
>> assert(pushbuf->ptr < pushbuf->end);
>> *pushbuf->ptr++ = value;
>> }
>>
>> ...and easily pick up bugs with too few words? The helper would of
>> course be in the header-file, so the write gets inlined nicely.
>
> Sounds good. If you have no strong objections, I'd like to keep that for
> a follow on patch when there's more code that actively uses this API. It
> should be fairly safe to make more members public via drm_tegra_pushbuf
> rather than the other way around, so I'd like to err on the side of
> carefulness for a bit longer.
Hmm, isn't it the *current* code that is "careless" in this case? The
client doesn't have enough context to validate this itself (without
having access to the end-pointer)?
> Thanks for the review, and apologies for being sluggish.
No worries, thanks for the work so far! :)
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-05-02 14:53 ` Erik Faye-Lund
@ 2014-05-02 15:16 ` Thierry Reding
2014-05-02 15:59 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 15:16 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 6851 bytes --]
On Fri, May 02, 2014 at 04:53:55PM +0200, Erik Faye-Lund wrote:
> On Fri, May 2, 2014 at 4:06 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
[...]
> >> > +static inline struct drm_tegra_pushbuf_private *
> >> > +pushbuf_priv(struct drm_tegra_pushbuf *pb)
> >> > +{
> >> > + return container_of(pb, struct drm_tegra_pushbuf_private, base);
> >> > +}
> >> > +
> >>
> >> This seems to be the only use-case for container_of, and it just
> >> happens to return the exact same pointer as was passed in... so do we
> >> really need that helper?
> >
> > It has the benefit of keeping this code working even when somebody
> > suddenly adds to the beginning of drm_tegra_pushbuf_private. I'd rather
> > not resolve to force casting just to be on the safe side.
>
> Is that a really a legitimate worry? There's a very clear relationship
> between the two structures. Both nouveau and freedreno does the same
> thing (for nouveau_bo_priv, nouveau_device_priv, msm_device, msm_pipe
> and msm_bo structures), and it seems to work fine for them. Perhaps a
> comment along the lines of "/* this needs to be the first member */"
> is enough to discourage people from messing with it?
Oh well, if everybody else is doing it and you keep insisting I'll just
drop it. I do like explicit upcasting, but I guess doing it implicitly
will work as well. Worst case this will crash on people if they don't
know what they're doing.
> >> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
> >> > new file mode 100644
> >> > index 000000000000..178d5cd57541
> >> > --- /dev/null
> >> > +++ b/tegra/pushbuf.c
> >> > @@ -0,0 +1,205 @@
> >> <snip>
> >> > +drm_public
> >> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> >> > + struct drm_tegra_job *job)
> >> > +{
> >> > + struct drm_tegra_pushbuf_private *pushbuf;
> >> > + void *ptr;
> >> > + int err;
> >> > +
> >> > + pushbuf = calloc(1, sizeof(*pushbuf));
> >> > + if (!pushbuf)
> >> > + return -ENOMEM;
> >> > +
> >> > + DRMINITLISTHEAD(&pushbuf->list);
> >> > + DRMINITLISTHEAD(&pushbuf->bos);
> >> > + pushbuf->job = job;
> >> > +
> >> > + *pushbufp = &pushbuf->base;
> >> > +
> >> > + DRMLISTADD(&pushbuf->list, &job->pushbufs);
> >>
> >> Hmm. So the job keeps a list of pushbufs. What purpose does this
> >> serve? Shouldn't the job only need the cmdbuf-list (which it already
> >> has) and the BOs (so they can be dereferenced after being submitted)?
> >
> > This is currently used to keep track of existing pushbuffers and make
> > sure that they are freed (when the job is freed).
> >
>
> OK, it seems to me that the need for keeping the pushbuffers around is
> simply not there. Only the BOs needs to be kept around, no?
Right. But unless we come up with an alternative API I can't just drop
this. Otherwise we'll be leaking pushbuffers all over the place if
callers don't clean them up themselves.
> > If reusing the pushbuf objects makes sense, then perhaps a different API
> > would be more useful. Rather than allocate in the context of a job, they
> > could be allocated in a channel-wide context and added to/associated
> > with a job only as needed.
>
> Yes, I think this might make sense. I'll have to ponder a bit more
> about this, though.
What I've been thinking is something rougly like this:
int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbuf, struct
drm_tegra_channel *channel);
int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf);
int drm_tegra_pushbuf_reset(struct drm_tegra_pushbuf *pushbuf);
int drm_tegra_job_queue(struct drm_tegra_job *job,
struct drm_tegra_pushbuf *pushbuf);
Then we could either reset the pushbuf in drm_tegra_job_queue() (in
which case drm_tegra_pushbuf_reset() could be private) or leave it up to
the caller to manually reset when they need to reuse the pushbuffer.
> >> Either way, I think this should either call drm_tegra_pushbuf_prepare
> >> to make sure two words are available, or be renamed to reflect that it
> >> causes a push (or two, as in the current form).
> >
> > I've added a call to drm_tegra_pushbuf_prepare() here and in
> > drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
> >
>
> Actually, I think drm_tegra_pushbuf_relocate should just be renamed to
> drm_tegra_pushbuf_push_reloc(), as that conveys quite clearly that it
> is just a push-helper and need to be counted when preparing. That way
> it doesn't need to prepare itself.
I don't think we necessarily need to rename the function to make it
obvious. This is completely new API anyway, so we can add comments to
explain that drm_tegra_pushbuf_relocate() will push a placeholder word
with a dummy address that will be used to relocate a BO and needs to be
included when preparing a pushbuffer.
> >> > +struct drm_tegra_channel;
> >> > +struct drm_tegra_job;
> >> > +
> >> > +struct drm_tegra_pushbuf {
> >> > + uint32_t *ptr;
> >> > +};
> >>
> >> Hmm, single-member structs always makes me cringe a bit. But in this
> >> case it's much better than a double-pointer IMO.
> >>
> >> But perhaps some of the members in the private-struct might be useful
> >> out here? For instance, if "uint32_t *end;" was also available, we
> >> could do:
> >>
> >> inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
> >> *pushbuf, uint32_t value)
> >> {
> >> assert(pushbuf->ptr < pushbuf->end);
> >> *pushbuf->ptr++ = value;
> >> }
> >>
> >> ...and easily pick up bugs with too few words? The helper would of
> >> course be in the header-file, so the write gets inlined nicely.
> >
> > Sounds good. If you have no strong objections, I'd like to keep that for
> > a follow on patch when there's more code that actively uses this API. It
> > should be fairly safe to make more members public via drm_tegra_pushbuf
> > rather than the other way around, so I'd like to err on the side of
> > carefulness for a bit longer.
>
> Hmm, isn't it the *current* code that is "careless" in this case? The
> client doesn't have enough context to validate this itself (without
> having access to the end-pointer)?
What I meant was being careful about how much we expose. Whatever we
expose now we can potentially never hide again because code may depend
on it being public. I don't think it's particularly careless to require
the caller to prepare a buffer. If they then write past its end, that's
their issue. That's like malloc()ing a buffer and then filling it with
more bytes than you've allocated. That's not the level of safety that
this API needs in my opinion.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs
2014-05-02 15:16 ` Thierry Reding
@ 2014-05-02 15:59 ` Erik Faye-Lund
0 siblings, 0 replies; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 15:59 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Fri, May 2, 2014 at 5:16 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Fri, May 02, 2014 at 04:53:55PM +0200, Erik Faye-Lund wrote:
>> On Fri, May 2, 2014 at 4:06 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
>> >> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
>> >> > new file mode 100644
>> >> > index 000000000000..178d5cd57541
>> >> > --- /dev/null
>> >> > +++ b/tegra/pushbuf.c
>> >> > @@ -0,0 +1,205 @@
>> >> <snip>
>> >> > +drm_public
>> >> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
>> >> > + struct drm_tegra_job *job)
>> >> > +{
>> >> > + struct drm_tegra_pushbuf_private *pushbuf;
>> >> > + void *ptr;
>> >> > + int err;
>> >> > +
>> >> > + pushbuf = calloc(1, sizeof(*pushbuf));
>> >> > + if (!pushbuf)
>> >> > + return -ENOMEM;
>> >> > +
>> >> > + DRMINITLISTHEAD(&pushbuf->list);
>> >> > + DRMINITLISTHEAD(&pushbuf->bos);
>> >> > + pushbuf->job = job;
>> >> > +
>> >> > + *pushbufp = &pushbuf->base;
>> >> > +
>> >> > + DRMLISTADD(&pushbuf->list, &job->pushbufs);
>> >>
>> >> Hmm. So the job keeps a list of pushbufs. What purpose does this
>> >> serve? Shouldn't the job only need the cmdbuf-list (which it already
>> >> has) and the BOs (so they can be dereferenced after being submitted)?
>> >
>> > This is currently used to keep track of existing pushbuffers and make
>> > sure that they are freed (when the job is freed).
>> >
>>
>> OK, it seems to me that the need for keeping the pushbuffers around is
>> simply not there. Only the BOs needs to be kept around, no?
>
> Right. But unless we come up with an alternative API I can't just drop
> this. Otherwise we'll be leaking pushbuffers all over the place if
> callers don't clean them up themselves.
>
OK. But just to get some clarity, if the same level of abstraction
calls drm_tegra_pushbuf_new() and drm_tegra_pushbuf_free() when it's
done, we'll currently get use-after-free and double-frees, right?
As-is, the client would have to call drm_tegra_pushbuf_new(), transfer
the ownership of that object to the job object by calling
drm_tegra_pushbuf_prepare(), and clean it up by calling
drm_tegra_job_submit()? Or am I missing something?
>> > If reusing the pushbuf objects makes sense, then perhaps a different API
>> > would be more useful. Rather than allocate in the context of a job, they
>> > could be allocated in a channel-wide context and added to/associated
>> > with a job only as needed.
>>
>> Yes, I think this might make sense. I'll have to ponder a bit more
>> about this, though.
>
> What I've been thinking is something rougly like this:
>
> int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbuf, struct
> drm_tegra_channel *channel);
> int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf);
> int drm_tegra_pushbuf_reset(struct drm_tegra_pushbuf *pushbuf);
>
> int drm_tegra_job_queue(struct drm_tegra_job *job,
> struct drm_tegra_pushbuf *pushbuf);
>
> Then we could either reset the pushbuf in drm_tegra_job_queue() (in
> which case drm_tegra_pushbuf_reset() could be private) or leave it up to
> the caller to manually reset when they need to reuse the pushbuffer.
>
OK, this looks better to me. I'll still have to wrap my head around
these things a bit better to tell for sure, though ;)
>> >> Either way, I think this should either call drm_tegra_pushbuf_prepare
>> >> to make sure two words are available, or be renamed to reflect that it
>> >> causes a push (or two, as in the current form).
>> >
>> > I've added a call to drm_tegra_pushbuf_prepare() here and in
>> > drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
>> >
>>
>> Actually, I think drm_tegra_pushbuf_relocate should just be renamed to
>> drm_tegra_pushbuf_push_reloc(), as that conveys quite clearly that it
>> is just a push-helper and need to be counted when preparing. That way
>> it doesn't need to prepare itself.
>
> I don't think we necessarily need to rename the function to make it
> obvious. This is completely new API anyway, so we can add comments to
> explain that drm_tegra_pushbuf_relocate() will push a placeholder word
> with a dummy address that will be used to relocate a BO and needs to be
> included when preparing a pushbuffer.
>
True.
I have a "secret plan" here, which I guess I should just be open about:
I wonder if it would make sense in the long run to add multiple
push-variants, to save client code from repeating the same logic,
similar to the drm_tegra_pushbuf_push_word I mentioned above:
inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
*pushbuf, uint32_t value)
{
assert(pushbuf->ptr < pushbuf->end);
*pushbuf->ptr++ = value;
}
inline void drm_tegra_pushbuf_push_float(struct drm_tegra_pushbuf
*pushbuf, float value)
{
union {
float f;
uint32_t i;
} u;
u.f = value;
drm_tegra_pushbuf_push_word(pushbuf, u.i);
}
And probably a similar version for FP20. My thinking was that a reloc
belongs in that same category, and probably should have a similar
interface.
...But thinking about this a bit more, I think libdrm is the wrong
place to add these things. Most registers are packed bits of sorts, so
I guess the client code *needs* to wrap up loads of similar things
anyway. And besides, DDX doesn't care about many of these, so perhaps
Mesa is a better suited place for these "higher-level pushes".
>> >> But perhaps some of the members in the private-struct might be useful
>> >> out here? For instance, if "uint32_t *end;" was also available, we
>> >> could do:
>> >>
>> >> inline void drm_tegra_pushbuf_push_word(struct drm_tegra_pushbuf
>> >> *pushbuf, uint32_t value)
>> >> {
>> >> assert(pushbuf->ptr < pushbuf->end);
>> >> *pushbuf->ptr++ = value;
>> >> }
>> >>
>> >> ...and easily pick up bugs with too few words? The helper would of
>> >> course be in the header-file, so the write gets inlined nicely.
>> >
>> > Sounds good. If you have no strong objections, I'd like to keep that for
>> > a follow on patch when there's more code that actively uses this API. It
>> > should be fairly safe to make more members public via drm_tegra_pushbuf
>> > rather than the other way around, so I'd like to err on the side of
>> > carefulness for a bit longer.
>>
>> Hmm, isn't it the *current* code that is "careless" in this case? The
>> client doesn't have enough context to validate this itself (without
>> having access to the end-pointer)?
>
> What I meant was being careful about how much we expose. Whatever we
> expose now we can potentially never hide again because code may depend
> on it being public.
Fair enough. We're still hidden behind an experimental api-switch ATM.
But yeah, we should be careful about what we expose.
> I don't think it's particularly careless to require
> the caller to prepare a buffer. If they then write past its end, that's
> their issue. That's like malloc()ing a buffer and then filling it with
> more bytes than you've allocated. That's not the level of safety that
> this API needs in my opinion.
True, but we should also try to be helpful in finding out what went
wrong. And to follow up on your malloc-example, most sane CRTs have
some heap-corruption detection mechanisms to aid debugging. I don't
think this suggestion in particular is bending over backwards, though.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
` (3 preceding siblings ...)
2014-04-09 11:40 ` [PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:33 ` Erik Faye-Lund
` (2 more replies)
2014-04-09 11:40 ` [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test Thierry Reding
2014-04-09 11:40 ` [PATCH v2 libdrm 7/7] libdrm: valgrind-clear a few more IOCTL arguments Thierry Reding
6 siblings, 3 replies; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
This library provides helpers for common functionality needed by test
programs.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- fix a couple of memory leaks and get rid of some unneeded code
tests/tegra/Makefile.am | 10 +-
tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
tests/tegra/drm-test-tegra.h | 55 ++++++++++
tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
tests/tegra/drm-test.h | 72 +++++++++++++
5 files changed, 521 insertions(+), 1 deletion(-)
create mode 100644 tests/tegra/drm-test-tegra.c
create mode 100644 tests/tegra/drm-test-tegra.h
create mode 100644 tests/tegra/drm-test.c
create mode 100644 tests/tegra/drm-test.h
diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
index 8b481bde4f11..e468029d152e 100644
--- a/tests/tegra/Makefile.am
+++ b/tests/tegra/Makefile.am
@@ -5,9 +5,17 @@ AM_CPPFLAGS = \
AM_CFLAGS = -Wall -Werror
+noinst_LTLIBRARIES = libdrm-test.la
+libdrm_test_la_SOURCES = \
+ drm-test.c \
+ drm-test.h \
+ drm-test-tegra.c \
+ drm-test-tegra.h
+
LDADD = \
../../tegra/libdrm_tegra.la \
- ../../libdrm.la
+ ../../libdrm.la \
+ libdrm-test.la
TESTS = \
openclose \
diff --git a/tests/tegra/drm-test-tegra.c b/tests/tegra/drm-test-tegra.c
new file mode 100644
index 000000000000..aed353af739b
--- /dev/null
+++ b/tests/tegra/drm-test-tegra.c
@@ -0,0 +1,137 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <stdio.h>
+
+#include "drm-test-tegra.h"
+#include "tegra.h"
+
+int drm_tegra_gr2d_open(struct drm_tegra_gr2d **gr2dp, struct drm_tegra *drm)
+{
+ struct drm_tegra_gr2d *gr2d;
+ int err;
+
+ gr2d = calloc(1, sizeof(*gr2d));
+ if (!gr2d)
+ return -ENOMEM;
+
+ gr2d->drm = drm;
+
+ err = drm_tegra_channel_open(&gr2d->channel, drm, DRM_TEGRA_GR2D);
+ if (err < 0) {
+ free(gr2d);
+ return err;
+ }
+
+ *gr2dp = gr2d;
+
+ return 0;
+}
+
+int drm_tegra_gr2d_close(struct drm_tegra_gr2d *gr2d)
+{
+ if (!gr2d)
+ return -EINVAL;
+
+ drm_tegra_channel_close(gr2d->channel);
+ free(gr2d);
+
+ return 0;
+}
+
+int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
+ unsigned int x, unsigned int y, unsigned int width,
+ unsigned int height, uint32_t color)
+{
+ struct drm_tegra_bo *fbo = fb->data;
+ struct drm_tegra_pushbuf *pushbuf;
+ struct drm_tegra_fence *fence;
+ struct drm_tegra_job *job;
+ int err;
+
+ err = drm_tegra_job_new(&job, gr2d->channel);
+ if (err < 0)
+ return err;
+
+ err = drm_tegra_pushbuf_new(&pushbuf, job);
+ if (err < 0)
+ return err;
+
+ err = drm_tegra_pushbuf_prepare(pushbuf, 32);
+ if (err < 0)
+ return err;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
+ *pushbuf->ptr++ = 0x0000003a;
+ *pushbuf->ptr++ = 0x00000000;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
+ *pushbuf->ptr++ = 0x00000000;
+ *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
+ *pushbuf->ptr++ = 0x000000cc;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
+
+ /* relocate destination buffer */
+ err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
+ if (err < 0) {
+ fprintf(stderr, "failed to relocate buffer object: %d\n", err);
+ return err;
+ }
+
+ *pushbuf->ptr++ = fb->pitch;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
+ *pushbuf->ptr++ = color;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
+ *pushbuf->ptr++ = 0x00000000;
+
+ *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
+ *pushbuf->ptr++ = height << 16 | width;
+ *pushbuf->ptr++ = y << 16 | x;
+
+ err = drm_tegra_job_submit(job, &fence);
+ if (err < 0) {
+ fprintf(stderr, "failed to submit job: %d\n", err);
+ return err;
+ }
+
+ err = drm_tegra_fence_wait(fence);
+ if (err < 0) {
+ fprintf(stderr, "failed to wait for fence: %d\n", err);
+ return err;
+ }
+
+ drm_tegra_fence_free(fence);
+ drm_tegra_pushbuf_free(pushbuf);
+ drm_tegra_job_free(job);
+
+ return 0;
+}
diff --git a/tests/tegra/drm-test-tegra.h b/tests/tegra/drm-test-tegra.h
new file mode 100644
index 000000000000..d1cb6b1ee440
--- /dev/null
+++ b/tests/tegra/drm-test-tegra.h
@@ -0,0 +1,55 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef TEGRA_DRM_TEST_TEGRA_H
+#define TEGRA_DRM_TEST_TEGRA_H
+
+#include "drm-test.h"
+#include "tegra.h"
+
+#define HOST1X_OPCODE_SETCL(offset, classid, mask) \
+ ((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) << 6) | ((mask) & 0x3f))
+#define HOST1X_OPCODE_INCR(offset, count) \
+ ((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
+#define HOST1X_OPCODE_NONINCR(offset, count) \
+ ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
+#define HOST1X_OPCODE_MASK(offset, mask) \
+ ((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0xffff))
+#define HOST1X_OPCODE_IMM(offset, data) \
+ ((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0xffff))
+#define HOST1X_OPCODE_EXTEND(subop, value) \
+ ((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xffffff))
+
+#define HOST1X_CLASS_GR2D 0x51
+
+struct drm_tegra_gr2d {
+ struct drm_tegra *drm;
+ struct drm_tegra_channel *channel;
+};
+
+int drm_tegra_gr2d_open(struct drm_tegra_gr2d **gr2dp, struct drm_tegra *drm);
+int drm_tegra_gr2d_close(struct drm_tegra_gr2d *gr2d);
+int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
+ unsigned int x, unsigned int y, unsigned int width,
+ unsigned int height, uint32_t color);
+
+#endif
diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c
new file mode 100644
index 000000000000..1f91d17f61bb
--- /dev/null
+++ b/tests/tegra/drm-test.c
@@ -0,0 +1,248 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/ioctl.h>
+
+#include "xf86drm.h"
+#include "xf86drmMode.h"
+#include "drm_fourcc.h"
+
+#include "drm-test.h"
+
+static int drm_screen_probe_connector(struct drm_screen *screen,
+ drmModeConnectorPtr connector)
+{
+ drmModeEncoderPtr encoder;
+ drmModeCrtcPtr crtc;
+ drmModeFBPtr fb;
+
+ encoder = drmModeGetEncoder(screen->fd, connector->encoder_id);
+ if (!encoder)
+ return -ENODEV;
+
+ crtc = drmModeGetCrtc(screen->fd, encoder->crtc_id);
+ if (!crtc) {
+ drmModeFreeEncoder(encoder);
+ return -ENODEV;
+ }
+
+ screen->old_fb = crtc->buffer_id;
+
+ fb = drmModeGetFB(screen->fd, crtc->buffer_id);
+ if (!fb) {
+ /* TODO: create new framebuffer */
+ drmModeFreeEncoder(encoder);
+ drmModeFreeCrtc(crtc);
+ return -ENOSYS;
+ }
+
+ screen->connector = connector->connector_id;
+ screen->old_fb = crtc->buffer_id;
+ screen->crtc = encoder->crtc_id;
+ /* TODO: check crtc->mode_valid */
+ screen->mode = crtc->mode;
+
+ screen->width = fb->width;
+ screen->height = fb->height;
+ screen->pitch = fb->pitch;
+ screen->depth = fb->depth;
+ screen->bpp = fb->bpp;
+
+ drmModeFreeEncoder(encoder);
+ drmModeFreeCrtc(crtc);
+ drmModeFreeFB(fb);
+
+ return 0;
+}
+
+int drm_screen_open(struct drm_screen **screenp, int fd)
+{
+ drmModeConnectorPtr connector;
+ struct drm_screen *screen;
+ bool found = false;
+ drmModeResPtr res;
+ unsigned int i;
+ int err;
+
+ if (!screenp || fd < 0)
+ return -EINVAL;
+
+ screen = calloc(1, sizeof(*screen));
+ if (!screen)
+ return -ENOMEM;
+
+ screen->format = DRM_FORMAT_XRGB8888;
+ screen->fd = fd;
+
+ res = drmModeGetResources(fd);
+ if (!res) {
+ free(screen);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < res->count_connectors; i++) {
+ connector = drmModeGetConnector(fd, res->connectors[i]);
+ if (!connector)
+ continue;
+
+ if (connector->connection != DRM_MODE_CONNECTED) {
+ drmModeFreeConnector(connector);
+ continue;
+ }
+
+ err = drm_screen_probe_connector(screen, connector);
+ if (err < 0) {
+ drmModeFreeConnector(connector);
+ continue;
+ }
+
+ drmModeFreeConnector(connector);
+ found = true;
+ break;
+ }
+
+ drmModeFreeResources(res);
+
+ if (!found) {
+ free(screen);
+ return -ENODEV;
+ }
+
+ *screenp = screen;
+
+ return 0;
+}
+
+int drm_screen_close(struct drm_screen *screen)
+{
+ int err;
+
+ err = drmModeSetCrtc(screen->fd, screen->crtc, screen->old_fb, 0, 0,
+ &screen->connector, 1, &screen->mode);
+ if (err < 0) {
+ fprintf(stderr, "drmModeSetCrtc() failed: %m\n");
+ return -errno;
+ }
+
+ free(screen);
+
+ return 0;
+}
+
+int drm_framebuffer_new(struct drm_framebuffer **fbp,
+ struct drm_screen *screen, uint32_t handle,
+ unsigned int width, unsigned int height,
+ unsigned int pitch, uint32_t format,
+ void *data)
+{
+ struct drm_framebuffer *fb;
+ uint32_t handles[4];
+ uint32_t pitches[4];
+ uint32_t offsets[4];
+ int err;
+
+ fb = calloc(1, sizeof(*fb));
+ if (!fb)
+ return -ENOMEM;
+
+ fb->fd = screen->fd;
+ fb->width = width;
+ fb->height = height;
+ fb->pitch = pitch;
+ fb->format = format;
+ fb->data = data;
+
+ handles[0] = handle;
+ pitches[0] = pitch;
+ offsets[0] = 0;
+
+ err = drmModeAddFB2(screen->fd, width, height, format, handles,
+ pitches, offsets, &fb->handle, 0);
+ if (err < 0)
+ return -errno;
+
+ *fbp = fb;
+
+ return 0;
+}
+
+int drm_framebuffer_free(struct drm_framebuffer *fb)
+{
+ int err;
+
+ err = drmModeRmFB(fb->fd, fb->handle);
+ if (err < 0)
+ return -errno;
+
+ free(fb);
+
+ return 0;
+}
+
+int drm_screen_set_framebuffer(struct drm_screen *screen,
+ struct drm_framebuffer *fb)
+{
+ int err;
+
+ err = drmModeSetCrtc(screen->fd, screen->crtc, fb->handle, 0, 0,
+ &screen->connector, 1, &screen->mode);
+ if (err < 0)
+ return -errno;
+
+ return 0;
+}
+
+int drm_open(const char *path)
+{
+ int fd, err;
+
+ fd = open(path, O_RDWR);
+ if (fd < 0)
+ return -errno;
+
+ err = drmSetMaster(fd);
+ if (err < 0) {
+ close(fd);
+ return -errno;
+ }
+
+ return fd;
+}
+
+void drm_close(int fd)
+{
+ drmDropMaster(fd);
+ close(fd);
+}
diff --git a/tests/tegra/drm-test.h b/tests/tegra/drm-test.h
new file mode 100644
index 000000000000..0f3d1ec26e2b
--- /dev/null
+++ b/tests/tegra/drm-test.h
@@ -0,0 +1,72 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifndef TEGRA_DRM_TEST_H
+#define TEGRA_DRM_TEST_H
+
+#include <stdint.h>
+#include <stdlib.h>
+
+#include "xf86drmMode.h"
+
+struct drm_screen {
+ int fd;
+
+ unsigned int width;
+ unsigned int height;
+ unsigned int pitch;
+ unsigned int depth;
+ unsigned int bpp;
+
+ drmModeModeInfo mode;
+ uint32_t connector;
+ uint32_t old_fb;
+ uint32_t format;
+ uint32_t crtc;
+};
+
+struct drm_framebuffer {
+ unsigned int width;
+ unsigned int height;
+ unsigned int pitch;
+ uint32_t format;
+ uint32_t handle;
+ void *data;
+ int fd;
+};
+
+int drm_screen_open(struct drm_screen **screenp, int fd);
+int drm_screen_close(struct drm_screen *screen);
+int drm_screen_set_framebuffer(struct drm_screen *screen,
+ struct drm_framebuffer *fb);
+
+int drm_framebuffer_new(struct drm_framebuffer **fbp,
+ struct drm_screen *screen, uint32_t handle,
+ unsigned int width, unsigned int height,
+ unsigned int pitch, uint32_t format,
+ void *data);
+int drm_framebuffer_free(struct drm_framebuffer *fb);
+
+int drm_open(const char *path);
+void drm_close(int fd);
+
+#endif
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-09 11:40 ` [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Thierry Reding
@ 2014-04-10 17:33 ` Erik Faye-Lund
2014-05-02 14:42 ` Thierry Reding
2014-04-28 19:44 ` Erik Faye-Lund
2014-05-02 15:18 ` Erik Faye-Lund
2 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-10 17:33 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This library provides helpers for common functionality needed by test
> programs.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - fix a couple of memory leaks and get rid of some unneeded code
>
> tests/tegra/Makefile.am | 10 +-
> tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
> tests/tegra/drm-test-tegra.h | 55 ++++++++++
> tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
> tests/tegra/drm-test.h | 72 +++++++++++++
> 5 files changed, 521 insertions(+), 1 deletion(-)
> create mode 100644 tests/tegra/drm-test-tegra.c
> create mode 100644 tests/tegra/drm-test-tegra.h
> create mode 100644 tests/tegra/drm-test.c
> create mode 100644 tests/tegra/drm-test.h
This isn't really important at this point, but it looks to me like
tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps
some other tests can benefit from it? Doing so is of course something
whoever writes those tests should do, though. Leaving them in the
tegra-subdir is probably fine.
> +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
> + unsigned int x, unsigned int y, unsigned int width,
> + unsigned int height, uint32_t color)
> +{
> + struct drm_tegra_bo *fbo = fb->data;
> + struct drm_tegra_pushbuf *pushbuf;
> + struct drm_tegra_fence *fence;
> + struct drm_tegra_job *job;
> + int err;
> +
> + err = drm_tegra_job_new(&job, gr2d->channel);
> + if (err < 0)
> + return err;
> +
> + err = drm_tegra_pushbuf_new(&pushbuf, job);
> + if (err < 0)
> + return err;
> +
I think this helper would be generally more useful if it skipped the
above two, and required the call-sites to do them instead.
> + err = drm_tegra_pushbuf_prepare(pushbuf, 32);
> + if (err < 0)
> + return err;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
> + *pushbuf->ptr++ = 0x0000003a;
> + *pushbuf->ptr++ = 0x00000000;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
> + *pushbuf->ptr++ = 0x00000000;
> + *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
> + *pushbuf->ptr++ = 0x000000cc;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
> +
> + /* relocate destination buffer */
> + err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
> + if (err < 0) {
> + fprintf(stderr, "failed to relocate buffer object: %d\n", err);
> + return err;
> + }
> +
> + *pushbuf->ptr++ = fb->pitch;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
> + *pushbuf->ptr++ = color;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
> + *pushbuf->ptr++ = 0x00000000;
> +
> + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
> + *pushbuf->ptr++ = height << 16 | width;
> + *pushbuf->ptr++ = y << 16 | x;
> +
...and stop here.
That way we can use it for tests that perform multiple fills in one submit etc.
> +#define HOST1X_OPCODE_SETCL(offset, classid, mask) \
> + ((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) << 6) | ((mask) & 0x3f))
> +#define HOST1X_OPCODE_INCR(offset, count) \
> + ((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
> +#define HOST1X_OPCODE_NONINCR(offset, count) \
> + ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
> +#define HOST1X_OPCODE_MASK(offset, mask) \
> + ((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0xffff))
> +#define HOST1X_OPCODE_IMM(offset, data) \
> + ((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0xffff))
> +#define HOST1X_OPCODE_EXTEND(subop, value) \
> + ((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xffffff))
> +
> +#define HOST1X_CLASS_GR2D 0x51
Hmm, shouldn't these be available from somewhere else already? No
point in repeating the same macros over and over again, no?
> diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c
> new file mode 100644
> index 000000000000..1f91d17f61bb
> --- /dev/null
> +++ b/tests/tegra/drm-test.c
> @@ -0,0 +1,248 @@
> +/*
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <sys/ioctl.h>
> +
> +#include "xf86drm.h"
> +#include "xf86drmMode.h"
> +#include "drm_fourcc.h"
> +
> +#include "drm-test.h"
> +
> +static int drm_screen_probe_connector(struct drm_screen *screen,
> + drmModeConnectorPtr connector)
> +{
> + drmModeEncoderPtr encoder;
> + drmModeCrtcPtr crtc;
> + drmModeFBPtr fb;
> +
> + encoder = drmModeGetEncoder(screen->fd, connector->encoder_id);
> + if (!encoder)
> + return -ENODEV;
> +
> + crtc = drmModeGetCrtc(screen->fd, encoder->crtc_id);
> + if (!crtc) {
> + drmModeFreeEncoder(encoder);
> + return -ENODEV;
> + }
> +
> + screen->old_fb = crtc->buffer_id;
> +
> + fb = drmModeGetFB(screen->fd, crtc->buffer_id);
> + if (!fb) {
> + /* TODO: create new framebuffer */
What's the implications of not doing what this TODO says?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-10 17:33 ` Erik Faye-Lund
@ 2014-05-02 14:42 ` Thierry Reding
2014-05-02 15:13 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:42 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 6879 bytes --]
On Thu, Apr 10, 2014 at 07:33:33PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This library provides helpers for common functionality needed by test
> > programs.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - fix a couple of memory leaks and get rid of some unneeded code
> >
> > tests/tegra/Makefile.am | 10 +-
> > tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
> > tests/tegra/drm-test-tegra.h | 55 ++++++++++
> > tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
> > tests/tegra/drm-test.h | 72 +++++++++++++
> > 5 files changed, 521 insertions(+), 1 deletion(-)
> > create mode 100644 tests/tegra/drm-test-tegra.c
> > create mode 100644 tests/tegra/drm-test-tegra.h
> > create mode 100644 tests/tegra/drm-test.c
> > create mode 100644 tests/tegra/drm-test.h
>
> This isn't really important at this point, but it looks to me like
> tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps
> some other tests can benefit from it? Doing so is of course something
> whoever writes those tests should do, though. Leaving them in the
> tegra-subdir is probably fine.
Daniel Vetter and I have been "discussing" this for a while now. There
are a bunch of tests in the intel-gpu-tools repository that aren't Intel
specific either and the idea is to eventually collect all those test
cases in a common location so that they can be reused by other drivers
too, but so far nobody's had time to do that yet.
> > +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
> > + unsigned int x, unsigned int y, unsigned int width,
> > + unsigned int height, uint32_t color)
> > +{
> > + struct drm_tegra_bo *fbo = fb->data;
> > + struct drm_tegra_pushbuf *pushbuf;
> > + struct drm_tegra_fence *fence;
> > + struct drm_tegra_job *job;
> > + int err;
> > +
> > + err = drm_tegra_job_new(&job, gr2d->channel);
> > + if (err < 0)
> > + return err;
> > +
> > + err = drm_tegra_pushbuf_new(&pushbuf, job);
> > + if (err < 0)
> > + return err;
> > +
>
> I think this helper would be generally more useful if it skipped the
> above two, and required the call-sites to do them instead.
>
> > + err = drm_tegra_pushbuf_prepare(pushbuf, 32);
> > + if (err < 0)
> > + return err;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
> > + *pushbuf->ptr++ = 0x0000003a;
> > + *pushbuf->ptr++ = 0x00000000;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
> > + *pushbuf->ptr++ = 0x00000000;
> > + *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
> > + *pushbuf->ptr++ = 0x000000cc;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
> > +
> > + /* relocate destination buffer */
> > + err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
> > + if (err < 0) {
> > + fprintf(stderr, "failed to relocate buffer object: %d\n", err);
> > + return err;
> > + }
> > +
> > + *pushbuf->ptr++ = fb->pitch;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
> > + *pushbuf->ptr++ = color;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
> > + *pushbuf->ptr++ = 0x00000000;
> > +
> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
> > + *pushbuf->ptr++ = height << 16 | width;
> > + *pushbuf->ptr++ = y << 16 | x;
> > +
>
> ...and stop here.
>
> That way we can use it for tests that perform multiple fills in one submit etc.
How about we make drm_tegra_gr2d_fill() take a drm_tegra_pushbuf object
and push the commands as you suggest and then add a wrapper, say
drm_tegra_gr2d_fill_simple() for convenience when a single fill per
submit is good enough?
> > +#define HOST1X_OPCODE_SETCL(offset, classid, mask) \
> > + ((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) << 6) | ((mask) & 0x3f))
> > +#define HOST1X_OPCODE_INCR(offset, count) \
> > + ((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
> > +#define HOST1X_OPCODE_NONINCR(offset, count) \
> > + ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
> > +#define HOST1X_OPCODE_MASK(offset, mask) \
> > + ((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0xffff))
> > +#define HOST1X_OPCODE_IMM(offset, data) \
> > + ((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0xffff))
> > +#define HOST1X_OPCODE_EXTEND(subop, value) \
> > + ((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xffffff))
> > +
> > +#define HOST1X_CLASS_GR2D 0x51
>
> Hmm, shouldn't these be available from somewhere else already? No
> point in repeating the same macros over and over again, no?
I don't think we have these anywhere else. It seems to be custom to have
numerous redefinitions of this kind of macro throughout various drivers.
I suspect that the reason is that these can vary depending on chipset
revision and keeping a global list would require a revision prefix to be
prepended to all of them.
Even if we want them somewhere else, I wouldn't know where they'd be
best kept to be honest.
> > diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c
[...]
> > +static int drm_screen_probe_connector(struct drm_screen *screen,
> > + drmModeConnectorPtr connector)
> > +{
> > + drmModeEncoderPtr encoder;
> > + drmModeCrtcPtr crtc;
> > + drmModeFBPtr fb;
> > +
> > + encoder = drmModeGetEncoder(screen->fd, connector->encoder_id);
> > + if (!encoder)
> > + return -ENODEV;
> > +
> > + crtc = drmModeGetCrtc(screen->fd, encoder->crtc_id);
> > + if (!crtc) {
> > + drmModeFreeEncoder(encoder);
> > + return -ENODEV;
> > + }
> > +
> > + screen->old_fb = crtc->buffer_id;
> > +
> > + fb = drmModeGetFB(screen->fd, crtc->buffer_id);
> > + if (!fb) {
> > + /* TODO: create new framebuffer */
>
> What's the implications of not doing what this TODO says?
It currently just means that we don't want to deal with this situation,
which I think shouldn't be happening in the first place anyway. So the
TODO is there mostly as a reminder that this could happen and that there
*might* be something more useful than returning an error that could be
done.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-05-02 14:42 ` Thierry Reding
@ 2014-05-02 15:13 ` Erik Faye-Lund
0 siblings, 0 replies; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 15:13 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Fri, May 2, 2014 at 4:42 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Apr 10, 2014 at 07:33:33PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > From: Thierry Reding <treding@nvidia.com>
>> >
>> > This library provides helpers for common functionality needed by test
>> > programs.
>> >
>> > Signed-off-by: Thierry Reding <treding@nvidia.com>
>> > ---
>> > Changes in v2:
>> > - fix a couple of memory leaks and get rid of some unneeded code
>> >
>> > tests/tegra/Makefile.am | 10 +-
>> > tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
>> > tests/tegra/drm-test-tegra.h | 55 ++++++++++
>> > tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
>> > tests/tegra/drm-test.h | 72 +++++++++++++
>> > 5 files changed, 521 insertions(+), 1 deletion(-)
>> > create mode 100644 tests/tegra/drm-test-tegra.c
>> > create mode 100644 tests/tegra/drm-test-tegra.h
>> > create mode 100644 tests/tegra/drm-test.c
>> > create mode 100644 tests/tegra/drm-test.h
>>
>> This isn't really important at this point, but it looks to me like
>> tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps
>> some other tests can benefit from it? Doing so is of course something
>> whoever writes those tests should do, though. Leaving them in the
>> tegra-subdir is probably fine.
>
> Daniel Vetter and I have been "discussing" this for a while now. There
> are a bunch of tests in the intel-gpu-tools repository that aren't Intel
> specific either and the idea is to eventually collect all those test
> cases in a common location so that they can be reused by other drivers
> too, but so far nobody's had time to do that yet.
Right. Let's just leave it for later, then.
>> > +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer *fb,
>> > + unsigned int x, unsigned int y, unsigned int width,
>> > + unsigned int height, uint32_t color)
>> > +{
>> > + struct drm_tegra_bo *fbo = fb->data;
>> > + struct drm_tegra_pushbuf *pushbuf;
>> > + struct drm_tegra_fence *fence;
>> > + struct drm_tegra_job *job;
>> > + int err;
>> > +
>> > + err = drm_tegra_job_new(&job, gr2d->channel);
>> > + if (err < 0)
>> > + return err;
>> > +
>> > + err = drm_tegra_pushbuf_new(&pushbuf, job);
>> > + if (err < 0)
>> > + return err;
>> > +
>>
>> I think this helper would be generally more useful if it skipped the
>> above two, and required the call-sites to do them instead.
>>
>> > + err = drm_tegra_pushbuf_prepare(pushbuf, 32);
>> > + if (err < 0)
>> > + return err;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
>> > + *pushbuf->ptr++ = 0x0000003a;
>> > + *pushbuf->ptr++ = 0x00000000;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
>> > + *pushbuf->ptr++ = 0x00000000;
>> > + *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
>> > + *pushbuf->ptr++ = 0x000000cc;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
>> > +
>> > + /* relocate destination buffer */
>> > + err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
>> > + if (err < 0) {
>> > + fprintf(stderr, "failed to relocate buffer object: %d\n", err);
>> > + return err;
>> > + }
>> > +
>> > + *pushbuf->ptr++ = fb->pitch;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
>> > + *pushbuf->ptr++ = color;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
>> > + *pushbuf->ptr++ = 0x00000000;
>> > +
>> > + *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
>> > + *pushbuf->ptr++ = height << 16 | width;
>> > + *pushbuf->ptr++ = y << 16 | x;
>> > +
>>
>> ...and stop here.
>>
>> That way we can use it for tests that perform multiple fills in one submit etc.
>
> How about we make drm_tegra_gr2d_fill() take a drm_tegra_pushbuf object
> and push the commands as you suggest and then add a wrapper, say
> drm_tegra_gr2d_fill_simple() for convenience when a single fill per
> submit is good enough?
I wouldn't be against such a convenience-wrapper, no. I would
personally leave that for the person who added more such tests,
though. I guess that could be said about my suggestion as well ;-)
>> > +#define HOST1X_OPCODE_SETCL(offset, classid, mask) \
>> > + ((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) << 6) | ((mask) & 0x3f))
>> > +#define HOST1X_OPCODE_INCR(offset, count) \
>> > + ((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
>> > +#define HOST1X_OPCODE_NONINCR(offset, count) \
>> > + ((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0xffff))
>> > +#define HOST1X_OPCODE_MASK(offset, mask) \
>> > + ((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0xffff))
>> > +#define HOST1X_OPCODE_IMM(offset, data) \
>> > + ((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0xffff))
>> > +#define HOST1X_OPCODE_EXTEND(subop, value) \
>> > + ((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xffffff))
>> > +
>> > +#define HOST1X_CLASS_GR2D 0x51
>>
>> Hmm, shouldn't these be available from somewhere else already? No
>> point in repeating the same macros over and over again, no?
>
> I don't think we have these anywhere else. It seems to be custom to have
> numerous redefinitions of this kind of macro throughout various drivers.
> I suspect that the reason is that these can vary depending on chipset
> revision and keeping a global list would require a revision prefix to be
> prepended to all of them.
>
> Even if we want them somewhere else, I wouldn't know where they'd be
> best kept to be honest.
OK. That's a bit of a shame though, as it means "everyone"
(xf86-video-opentegra and mesa + grate tests) must carry these
definitions. But it's not important, and can be fixed later if it
turns out to be a nuisance.
>> > diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c
> [...]
>> > +static int drm_screen_probe_connector(struct drm_screen *screen,
>> > + drmModeConnectorPtr connector)
>> > +{
>> > + drmModeEncoderPtr encoder;
>> > + drmModeCrtcPtr crtc;
>> > + drmModeFBPtr fb;
>> > +
>> > + encoder = drmModeGetEncoder(screen->fd, connector->encoder_id);
>> > + if (!encoder)
>> > + return -ENODEV;
>> > +
>> > + crtc = drmModeGetCrtc(screen->fd, encoder->crtc_id);
>> > + if (!crtc) {
>> > + drmModeFreeEncoder(encoder);
>> > + return -ENODEV;
>> > + }
>> > +
>> > + screen->old_fb = crtc->buffer_id;
>> > +
>> > + fb = drmModeGetFB(screen->fd, crtc->buffer_id);
>> > + if (!fb) {
>> > + /* TODO: create new framebuffer */
>>
>> What's the implications of not doing what this TODO says?
>
> It currently just means that we don't want to deal with this situation,
> which I think shouldn't be happening in the first place anyway. So the
> TODO is there mostly as a reminder that this could happen and that there
> *might* be something more useful than returning an error that could be
> done.
Right. Perhaps it could happen if no monitor is connected? That's
probably not important, though.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-09 11:40 ` [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Thierry Reding
2014-04-10 17:33 ` Erik Faye-Lund
@ 2014-04-28 19:44 ` Erik Faye-Lund
2014-05-02 14:47 ` Thierry Reding
2014-05-02 15:18 ` Erik Faye-Lund
2 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-28 19:44 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> This library provides helpers for common functionality needed by test
> programs.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - fix a couple of memory leaks and get rid of some unneeded code
>
> tests/tegra/Makefile.am | 10 +-
> tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
> tests/tegra/drm-test-tegra.h | 55 ++++++++++
> tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
> tests/tegra/drm-test.h | 72 +++++++++++++
> 5 files changed, 521 insertions(+), 1 deletion(-)
> create mode 100644 tests/tegra/drm-test-tegra.c
> create mode 100644 tests/tegra/drm-test-tegra.h
> create mode 100644 tests/tegra/drm-test.c
> create mode 100644 tests/tegra/drm-test.h
>
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> index 8b481bde4f11..e468029d152e 100644
> --- a/tests/tegra/Makefile.am
> +++ b/tests/tegra/Makefile.am
> @@ -5,9 +5,17 @@ AM_CPPFLAGS = \
>
> AM_CFLAGS = -Wall -Werror
>
> +noinst_LTLIBRARIES = libdrm-test.la
> +libdrm_test_la_SOURCES = \
> + drm-test.c \
> + drm-test.h \
> + drm-test-tegra.c \
> + drm-test-tegra.h
> +
> LDADD = \
> ../../tegra/libdrm_tegra.la \
> - ../../libdrm.la
> + ../../libdrm.la \
> + libdrm-test.la
>
Hmm, I need the following on top to please the linker:
diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
index 286af4b..88230c0 100644
--- a/tests/tegra/Makefile.am
+++ b/tests/tegra/Makefile.am
@@ -14,8 +14,8 @@ libdrm_test_la_SOURCES = \
LDADD = \
../../tegra/libdrm_tegra.la \
- ../../libdrm.la \
- libdrm-test.la
+ libdrm-test.la \
+ ../../libdrm.la
TESTS = \
openclose \
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-28 19:44 ` Erik Faye-Lund
@ 2014-05-02 14:47 ` Thierry Reding
0 siblings, 0 replies; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:47 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 2518 bytes --]
On Mon, Apr 28, 2014 at 09:44:35PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This library provides helpers for common functionality needed by test
> > programs.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - fix a couple of memory leaks and get rid of some unneeded code
> >
> > tests/tegra/Makefile.am | 10 +-
> > tests/tegra/drm-test-tegra.c | 137 ++++++++++++++++++++++++
> > tests/tegra/drm-test-tegra.h | 55 ++++++++++
> > tests/tegra/drm-test.c | 248 +++++++++++++++++++++++++++++++++++++++++++
> > tests/tegra/drm-test.h | 72 +++++++++++++
> > 5 files changed, 521 insertions(+), 1 deletion(-)
> > create mode 100644 tests/tegra/drm-test-tegra.c
> > create mode 100644 tests/tegra/drm-test-tegra.h
> > create mode 100644 tests/tegra/drm-test.c
> > create mode 100644 tests/tegra/drm-test.h
> >
> > diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> > index 8b481bde4f11..e468029d152e 100644
> > --- a/tests/tegra/Makefile.am
> > +++ b/tests/tegra/Makefile.am
> > @@ -5,9 +5,17 @@ AM_CPPFLAGS = \
> >
> > AM_CFLAGS = -Wall -Werror
> >
> > +noinst_LTLIBRARIES = libdrm-test.la
> > +libdrm_test_la_SOURCES = \
> > + drm-test.c \
> > + drm-test.h \
> > + drm-test-tegra.c \
> > + drm-test-tegra.h
> > +
> > LDADD = \
> > ../../tegra/libdrm_tegra.la \
> > - ../../libdrm.la
> > + ../../libdrm.la \
> > + libdrm-test.la
> >
>
> Hmm, I need the following on top to please the linker:
>
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> index 286af4b..88230c0 100644
> --- a/tests/tegra/Makefile.am
> +++ b/tests/tegra/Makefile.am
> @@ -14,8 +14,8 @@ libdrm_test_la_SOURCES = \
>
> LDADD = \
> ../../tegra/libdrm_tegra.la \
> - ../../libdrm.la \
> - libdrm-test.la
> + libdrm-test.la \
> + ../../libdrm.la
>
> TESTS = \
> openclose \
Yeah, this was reported by pretty much everybody who tested this. And I
have no setup where I could reproduce it. I eventually managed to
trigger a similar error by explicitly passing both -Wl,--as-needed and
-Wl,--no-copy-dt-needed-entries in LDFLAGS at configure time. I now have
a version that builds with or without these flags, so I hope that
everyone will now be happy.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 libdrm 5/7] tegra: Add helper library for tests
2014-04-09 11:40 ` [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Thierry Reding
2014-04-10 17:33 ` Erik Faye-Lund
2014-04-28 19:44 ` Erik Faye-Lund
@ 2014-05-02 15:18 ` Erik Faye-Lund
2 siblings, 0 replies; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 15:18 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> +int drm_open(const char *path)
> +{
> + int fd, err;
> +
> + fd = open(path, O_RDWR);
> + if (fd < 0)
> + return -errno;
> +
> + err = drmSetMaster(fd);
> + if (err < 0) {
> + close(fd);
> + return -errno;
> + }
Could we make this opt-in? Otherwise "make check" needs to be run as
root to pass, which is a bit iffy IMO as it also invokes the compiler
and all tools as root. I'd prefer not having to give my dev-tools (and
potentially buggy in-tree scripts) that much privilege...
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
` (4 preceding siblings ...)
2014-04-09 11:40 ` [PATCH v2 libdrm 5/7] tegra: Add helper library for tests Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
2014-04-10 17:28 ` Erik Faye-Lund
2014-04-09 11:40 ` [PATCH v2 libdrm 7/7] libdrm: valgrind-clear a few more IOCTL arguments Thierry Reding
6 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
This test uses the IOCTLs for job submission and fences to fill a sub-
region of the screen to a specific color using gr2d.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- free framebuffer when done
tests/tegra/.gitignore | 1 +
tests/tegra/Makefile.am | 1 +
tests/tegra/gr2d-fill.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 148 insertions(+)
create mode 100644 tests/tegra/gr2d-fill.c
diff --git a/tests/tegra/.gitignore b/tests/tegra/.gitignore
index 5c5216c5c5e6..5e0b5c601dd1 100644
--- a/tests/tegra/.gitignore
+++ b/tests/tegra/.gitignore
@@ -1 +1,2 @@
+gr2d-fill
openclose
diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
index e468029d152e..286af4b4d706 100644
--- a/tests/tegra/Makefile.am
+++ b/tests/tegra/Makefile.am
@@ -19,6 +19,7 @@ LDADD = \
TESTS = \
openclose \
+ gr2d-fill
if HAVE_INSTALL_TESTS
testdir = $(libexecdir)/libdrm/tests/tegra
diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
new file mode 100644
index 000000000000..b6ba35a9d668
--- /dev/null
+++ b/tests/tegra/gr2d-fill.c
@@ -0,0 +1,146 @@
+/*
+ * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
+ */
+
+#ifdef HAVE_CONFIG_H
+# include "config.h"
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <sys/ioctl.h>
+
+#include "xf86drm.h"
+#include "xf86drmMode.h"
+#include "drm_fourcc.h"
+
+#include "drm-test-tegra.h"
+#include "tegra.h"
+
+int main(int argc, char *argv[])
+{
+ uint32_t format = DRM_FORMAT_XRGB8888;
+ struct drm_tegra_gr2d *gr2d;
+ struct drm_framebuffer *fb;
+ struct drm_screen *screen;
+ unsigned int pitch, size;
+ struct drm_tegra_bo *bo;
+ struct drm_tegra *drm;
+ uint32_t handle;
+ int fd, err;
+ void *ptr;
+
+ fd = drm_open(argv[1]);
+ if (fd < 0) {
+ fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
+ strerror(errno));
+ return 1;
+ }
+
+ err = drm_screen_open(&screen, fd);
+ if (err < 0) {
+ fprintf(stderr, "failed to open screen: %s\n", strerror(-err));
+ return 1;
+ }
+
+ err = drm_tegra_new(&drm, fd);
+ if (err < 0) {
+ fprintf(stderr, "failed to create Tegra DRM context: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ err = drm_tegra_gr2d_open(&gr2d, drm);
+ if (err < 0) {
+ fprintf(stderr, "failed to open gr2d channel: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ pitch = screen->width * screen->bpp / 8;
+ size = pitch * screen->height;
+
+ err = drm_tegra_bo_new(&bo, drm, 0, size);
+ if (err < 0) {
+ fprintf(stderr, "failed to create buffer object: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ err = drm_tegra_bo_get_handle(bo, &handle);
+ if (err < 0) {
+ fprintf(stderr, "failed to get handle to buffer object: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ err = drm_tegra_bo_map(bo, &ptr);
+ if (err < 0) {
+ fprintf(stderr, "failed to map buffer object: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ memset(ptr, 0xff, size);
+
+ err = drm_framebuffer_new(&fb, screen, handle, screen->width,
+ screen->height, pitch, format, bo);
+ if (err < 0) {
+ fprintf(stderr, "failed to create framebuffer: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ err = drm_screen_set_framebuffer(screen, fb);
+ if (err < 0) {
+ fprintf(stderr, "failed to display framebuffer: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ sleep(1);
+
+ err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
+ fb->width / 2, fb->height / 2, 0x00000000);
+ if (err < 0) {
+ fprintf(stderr, "failed to fill rectangle: %s\n",
+ strerror(-err));
+ return 1;
+ }
+
+ sleep(1);
+
+ drm_framebuffer_free(fb);
+ drm_tegra_bo_put(bo);
+ drm_tegra_gr2d_close(gr2d);
+ drm_tegra_close(drm);
+ drm_screen_close(screen);
+ drm_close(fd);
+
+ return 0;
+}
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test
2014-04-09 11:40 ` [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test Thierry Reding
@ 2014-04-10 17:28 ` Erik Faye-Lund
2014-05-02 14:25 ` Thierry Reding
0 siblings, 1 reply; 29+ messages in thread
From: Erik Faye-Lund @ 2014-04-10 17:28 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
> new file mode 100644
> index 000000000000..b6ba35a9d668
> --- /dev/null
> +++ b/tests/tegra/gr2d-fill.c
> @@ -0,0 +1,146 @@
> +/*
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <sys/ioctl.h>
> +
> +#include "xf86drm.h"
> +#include "xf86drmMode.h"
> +#include "drm_fourcc.h"
> +
> +#include "drm-test-tegra.h"
> +#include "tegra.h"
> +
> +int main(int argc, char *argv[])
> +{
> + uint32_t format = DRM_FORMAT_XRGB8888;
> + struct drm_tegra_gr2d *gr2d;
> + struct drm_framebuffer *fb;
> + struct drm_screen *screen;
> + unsigned int pitch, size;
> + struct drm_tegra_bo *bo;
> + struct drm_tegra *drm;
> + uint32_t handle;
> + int fd, err;
> + void *ptr;
> +
> + fd = drm_open(argv[1]);
> + if (fd < 0) {
> + fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
> + strerror(errno));
> + return 1;
> + }
I'm not quite sure I understand this part. Why would argv[1] be
anything else than NULL? Is this useful for manual debugging, perhaps?
> + err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
> + fb->width / 2, fb->height / 2, 0x00000000);
> + if (err < 0) {
> + fprintf(stderr, "failed to fill rectangle: %s\n",
> + strerror(-err));
> + return 1;
> + }
> +
> + sleep(1);
> +
Why do we need to sleep here?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test
2014-04-10 17:28 ` Erik Faye-Lund
@ 2014-05-02 14:25 ` Thierry Reding
2014-05-02 15:02 ` Erik Faye-Lund
0 siblings, 1 reply; 29+ messages in thread
From: Thierry Reding @ 2014-05-02 14:25 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 4447 bytes --]
On Thu, Apr 10, 2014 at 07:28:16PM +0200, Erik Faye-Lund wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
> > new file mode 100644
> > index 000000000000..b6ba35a9d668
> > --- /dev/null
> > +++ b/tests/tegra/gr2d-fill.c
> > @@ -0,0 +1,146 @@
> > +/*
> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +# include "config.h"
> > +#endif
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <sys/ioctl.h>
> > +
> > +#include "xf86drm.h"
> > +#include "xf86drmMode.h"
> > +#include "drm_fourcc.h"
> > +
> > +#include "drm-test-tegra.h"
> > +#include "tegra.h"
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + uint32_t format = DRM_FORMAT_XRGB8888;
> > + struct drm_tegra_gr2d *gr2d;
> > + struct drm_framebuffer *fb;
> > + struct drm_screen *screen;
> > + unsigned int pitch, size;
> > + struct drm_tegra_bo *bo;
> > + struct drm_tegra *drm;
> > + uint32_t handle;
> > + int fd, err;
> > + void *ptr;
> > +
> > + fd = drm_open(argv[1]);
> > + if (fd < 0) {
> > + fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
> > + strerror(errno));
> > + return 1;
> > + }
>
> I'm not quite sure I understand this part. Why would argv[1] be
> anything else than NULL? Is this useful for manual debugging, perhaps?
Yes. On newer Tegra generations the nouveau driver can create its own
device files in /dev/dri and depending on device probe order, the card0
device can be the wrong one. For such cases these test programs can be
passed the path to the correct device via the command-line.
Probably a better approach would be to search for a compatible device
via udev matching or iterating over /dev/dri/card* and querying the
driver name (which is what drm_tegra_new() already does). But that seems
overkill currently. Perhaps when this turns into a more fully-fledged
test suite that could be implemented more cleverly.
For now I've addressed this by making the tests fallback to a default
device (/dev/dri/card0) if no argument has been specified.
> > + err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
> > + fb->width / 2, fb->height / 2, 0x00000000);
> > + if (err < 0) {
> > + fprintf(stderr, "failed to fill rectangle: %s\n",
> > + strerror(-err));
> > + return 1;
> > + }
> > +
> > + sleep(1);
> > +
>
> Why do we need to sleep here?
This is a visual test, so that one second gives me some time to see if
the result looks as expected.
I have tentative plans to implement a set of more automated tests, where
an image is rendered and the checksum computed over the content can be
compared with a known good reference checksum, but for now this is
better than nothing.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test
2014-05-02 14:25 ` Thierry Reding
@ 2014-05-02 15:02 ` Erik Faye-Lund
0 siblings, 0 replies; 29+ messages in thread
From: Erik Faye-Lund @ 2014-05-02 15:02 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel@lists.freedesktop.org
On Fri, May 2, 2014 at 4:25 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Apr 10, 2014 at 07:28:16PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
>> > new file mode 100644
>> > index 000000000000..b6ba35a9d668
>> > --- /dev/null
>> > +++ b/tests/tegra/gr2d-fill.c
>> > @@ -0,0 +1,146 @@
>> > +/*
>> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
>> > + */
>> > +
>> > +#ifdef HAVE_CONFIG_H
>> > +# include "config.h"
>> > +#endif
>> > +
>> > +#include <errno.h>
>> > +#include <fcntl.h>
>> > +#include <stdbool.h>
>> > +#include <stdint.h>
>> > +#include <stdio.h>
>> > +#include <stdlib.h>
>> > +#include <string.h>
>> > +#include <unistd.h>
>> > +
>> > +#include <sys/ioctl.h>
>> > +
>> > +#include "xf86drm.h"
>> > +#include "xf86drmMode.h"
>> > +#include "drm_fourcc.h"
>> > +
>> > +#include "drm-test-tegra.h"
>> > +#include "tegra.h"
>> > +
>> > +int main(int argc, char *argv[])
>> > +{
>> > + uint32_t format = DRM_FORMAT_XRGB8888;
>> > + struct drm_tegra_gr2d *gr2d;
>> > + struct drm_framebuffer *fb;
>> > + struct drm_screen *screen;
>> > + unsigned int pitch, size;
>> > + struct drm_tegra_bo *bo;
>> > + struct drm_tegra *drm;
>> > + uint32_t handle;
>> > + int fd, err;
>> > + void *ptr;
>> > +
>> > + fd = drm_open(argv[1]);
>> > + if (fd < 0) {
>> > + fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
>> > + strerror(errno));
>> > + return 1;
>> > + }
>>
>> I'm not quite sure I understand this part. Why would argv[1] be
>> anything else than NULL? Is this useful for manual debugging, perhaps?
>
> Yes. On newer Tegra generations the nouveau driver can create its own
> device files in /dev/dri and depending on device probe order, the card0
> device can be the wrong one. For such cases these test programs can be
> passed the path to the correct device via the command-line.
>
> Probably a better approach would be to search for a compatible device
> via udev matching or iterating over /dev/dri/card* and querying the
> driver name (which is what drm_tegra_new() already does). But that seems
> overkill currently. Perhaps when this turns into a more fully-fledged
> test suite that could be implemented more cleverly.
>
> For now I've addressed this by making the tests fallback to a default
> device (/dev/dri/card0) if no argument has been specified.
>
Thanks for the explanation, makes sense!
>> > + err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
>> > + fb->width / 2, fb->height / 2, 0x00000000);
>> > + if (err < 0) {
>> > + fprintf(stderr, "failed to fill rectangle: %s\n",
>> > + strerror(-err));
>> > + return 1;
>> > + }
>> > +
>> > + sleep(1);
>> > +
>>
>> Why do we need to sleep here?
>
> This is a visual test, so that one second gives me some time to see if
> the result looks as expected.
>
> I have tentative plans to implement a set of more automated tests, where
> an image is rendered and the checksum computed over the content can be
> compared with a known good reference checksum, but for now this is
> better than nothing.
Aha, makes sense. Thanks, looks good!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 libdrm 7/7] libdrm: valgrind-clear a few more IOCTL arguments
2014-04-09 11:40 [PATCH v2 libdrm 0/7] Add NVIDIA Tegra support Thierry Reding
` (5 preceding siblings ...)
2014-04-09 11:40 ` [PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test Thierry Reding
@ 2014-04-09 11:40 ` Thierry Reding
6 siblings, 0 replies; 29+ messages in thread
From: Thierry Reding @ 2014-04-09 11:40 UTC (permalink / raw)
To: dri-devel
From: Thierry Reding <treding@nvidia.com>
Fixes a few complaints raised by valgrind when running the Tegra tests.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
xf86drmMode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xf86drmMode.c b/xf86drmMode.c
index a6bb2ee44576..861248b7c2fc 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -282,6 +282,7 @@ int drmModeAddFB2(int fd, uint32_t width, uint32_t height,
struct drm_mode_fb_cmd2 f;
int ret;
+ VG_CLEAR(f);
f.width = width;
f.height = height;
f.pixel_format = pixel_format;
@@ -309,6 +310,7 @@ drmModeFBPtr drmModeGetFB(int fd, uint32_t buf)
struct drm_mode_fb_cmd info;
drmModeFBPtr r;
+ VG_CLEAR(info);
info.fb_id = buf;
if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, &info))
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread