After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 693826 - dmabuf-based GstMemory and GstAllocator incl. v4l2src support
dmabuf-based GstMemory and GstAllocator incl. v4l2src support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-02-14 16:59 UTC by Benjamin Gaignard
Modified: 2014-04-29 04:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add a new GstMemory/GstAllocator based on dmabuf (8.56 KB, patch)
2013-02-14 16:59 UTC, Benjamin Gaignard
needs-work Details | Review
additional path for v4l2src (9.29 KB, patch)
2013-02-14 17:00 UTC, Benjamin Gaignard
none Details | Review
add a new GstMemory/GstAllocator based on dmabuf (9.07 KB, patch)
2013-02-15 09:55 UTC, Benjamin Gaignard
needs-work Details | Review
additional path for v4l2src (9.63 KB, patch)
2013-02-15 09:56 UTC, Benjamin Gaignard
rejected Details | Review
add a new GstMemory/GstAllocator based on dmabuf (11.13 KB, patch)
2013-02-15 15:21 UTC, Benjamin Gaignard
needs-work Details | Review
additional path for v4l2src (9.32 KB, patch)
2013-02-15 15:22 UTC, Benjamin Gaignard
none Details | Review
add a new GstMemory/GstAllocator based on dmabuf (17.55 KB, patch)
2013-02-18 11:22 UTC, Benjamin Gaignard
needs-work Details | Review
additional path for v4l2src (9.06 KB, patch)
2013-02-18 11:23 UTC, Benjamin Gaignard
needs-work Details | Review
add a new GstMemory/GstAllocator based on dmabuf (18.52 KB, patch)
2013-02-18 13:48 UTC, Benjamin Gaignard
needs-work Details | Review
add a new GstMemory/GstAllocator based on dmabuf (18.56 KB, patch)
2013-02-18 14:19 UTC, Benjamin Gaignard
committed Details | Review
additional path for v4l2src (9.37 KB, patch)
2013-02-19 10:51 UTC, Benjamin Gaignard
committed Details | Review
add missing config.h file on top of gstdmabuf.c (739 bytes, patch)
2013-02-19 11:08 UTC, Benjamin Gaignard
committed Details | Review

Description Benjamin Gaignard 2013-02-14 16:59:02 UTC
Created attachment 236104 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

Hello,

This patch add support of dmabuf (http://lwn.net/Articles/489703/) for gstreamer.

I have create a new GstMemory/GstAllocator base on dmabuf which only mmap the memory when requested.

To test it I have also modify v4l2src to make it export buffer file descriptor instead of the classic mmapped buffer.

This command line make v4l export dmabuf fd buffers and the memory isn't mmap in user space.
gst-launch-1.0 v4l2src io-mode=4 ! fakesink --gst-debug=dmabuf:5

while this force the memory to be mmap
gst-launch-1.0 v4l2src io-mode=4 ! fakesink dump=true --gst-debug=dmabuf:5
Comment 1 Benjamin Gaignard 2013-02-14 17:00:44 UTC
Created attachment 236105 [details] [review]
additional path for v4l2src

this patch show how v4l2src can use gst-dmabuf memory to handle buffer exported with VIDIOC_EXPBUF ioctl
Comment 2 Sebastian Dröge (slomo) 2013-02-14 17:12:34 UTC
Review of attachment 236104 [details] [review]:

Overall, this should be in gstreamer core or gst-plugins-base in one of the libraries

::: gst/dmabuf/gstdmabuf.c
@@ +39,3 @@
+  gpointer data;
+  gsize size;
+} dmabuf_mem;

Part of this struct should probably be public to allow code to get the fd for the dmabuf instead of requiring mapping. E.g. could be done with a GstBufferMeta for dmabuf, or also by having a GstDmaBufMemory public struct (and internally using one that has more fields, see GstEvent and GstEventImpl for example)

@@ +70,3 @@
+  /* do not mmap twice the buffer */
+  if (res)
+    return res;

Depending on the flags it should do something... e.g. if mapped read-only before and write is requested it should fail

@@ +166,3 @@
+
+GstMemory *
+dmabuf_mem_alloc (gint fd, gsize size)

Maybe the allocator should be per fd: gst_dmabuf_allocator_new(int fd);

@@ +192,3 @@
+{
+  return mem->allocator == _dmabuf_allocator;
+}

You can just check the mem_type of the allocator, no need for a new function
Comment 3 Sebastian Dröge (slomo) 2013-02-14 17:15:19 UTC
Also, is the userspace mmap() part of dmabuf in the kernel now or still under discussion?
Comment 4 Benjamin Gaignard 2013-02-15 09:55:27 UTC
Created attachment 236225 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

fix comments done by Sebastian
Comment 5 Benjamin Gaignard 2013-02-15 09:56:27 UTC
Created attachment 236226 [details] [review]
additional path for v4l2src

change gst dmabuf alloc function name
Comment 6 Benjamin Gaignard 2013-02-15 10:00:13 UTC
userspace mmap is now fully supported by dmabuf in the kernel, there is no more discussion about that now.

Since I have only use gstdmabuf for v4l2 (and maybe later Xsink), I have put dmabuf lib in -good. If you have a better directory in mind, please advice me and I will provide a new patch.
Comment 7 Sebastian Dröge (slomo) 2013-02-15 10:02:48 UTC
Review of attachment 236225 [details] [review]:

Should still be in a core or base library

::: gst/dmabuf/gstdmabuf.c
@@ +74,3 @@
+  if (res) {
+    /* only return address if mapping flags are equivalent */
+    if (mem->mmapping_flags == prot)

Don't really need to be equivalent, right? But the new flags should be a subset of the old ones, and if it was write-mapped it shouldn't be able to map again
This needs some locking btw

::: gst/dmabuf/gstdmabuf.h
@@ +34,3 @@
+} GstDmaBufMemory;
+
+GstMemory * gst_dmabuf_allocator_new (gint fd , gsize size);

I talked with Wim about this and that was the conclusion:
- Have an opaque GstDmaBufMemory type and gint gst_dmabuf_memory_get_fd(mem)
- Have a constructor for a generic dmabuf allocator (GstAllocator * gst_dmabuf_allocator_new(void)) and GstMemory * gst_dmabuf_allocator_alloc(allocator, fd, size, params, ...)
- No GstMeta or anything
Comment 8 Sebastian Dröge (slomo) 2013-02-15 10:09:16 UTC
Review of attachment 236225 [details] [review]:

Actually the allocator should be a singleton, so you should have a GstAllocator *gst_dmabuf_allocator_obtain()

::: gst/dmabuf/gstdmabuf.c
@@ +142,3 @@
+} dmabuf_mem_AllocatorClass;
+
+GType dmabuf_mem_allocator_get_type (void);

The get_type() function should probably be in the header too, and a opaque definition of GstDmaBufAllocator

@@ +183,3 @@
+
+  if (!_dmabuf_mem_type_initialized)
+    dmabuf_mem_init ();

Also this is not threadsafe
Comment 9 Benjamin Gaignard 2013-02-15 15:21:47 UTC
Created attachment 236253 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf
Comment 10 Benjamin Gaignard 2013-02-15 15:22:13 UTC
Created attachment 236254 [details] [review]
additional path for v4l2src
Comment 11 Benjamin Gaignard 2013-02-15 15:23:36 UTC
Review of attachment 236226 [details] [review]:

obsolete
Comment 12 Sebastian Dröge (slomo) 2013-02-16 11:14:54 UTC
Review of attachment 236253 [details] [review]:

Please create the next patch against gst-plugins-base, adding a new library called libgstallocators :)

::: configure.ac
@@ +1066,3 @@
 gst/deinterlace/Makefile
 gst/debugutils/Makefile
+gst/dmabuf/Makefile

Please check somehow if dmabuf is supported on this system before building that part

::: gst/dmabuf/gstdmabuf.c
@@ +66,3 @@
+    g_warning ("Freeing memory still mapped");
+
+  close (dbmem->fd);

The documentation of gst_dmabuf_allocator_alloc() should mention that the GstMemory takes ownership of the fd and closes it after usage

@@ +156,3 @@
+  GST_DEBUG ("%p: copy %" G_GSSIZE_FORMAT " %" G_GSIZE_FORMAT, mem, offset,
+      size);
+  return gst_dmabuf_allocator_alloc (mem->mem.allocator, dup (mem->fd), size);

I guess dup() can fail. You should catch that here and return NULL then

@@ +203,3 @@
+  static GstAllocator *dmabuf_allocator;
+
+  if (g_once_init_enter (&dmabuf_allocator)) {

This is not how g_once_init_{enter,leave}() should be used. It's for initializing a gsize-sized variable, not a pointer (which can be of different sizes). Better use GOnce here

@@ +214,3 @@
+
+GstAllocator *
+gst_dmabuf_allocator_new (void)

Should be called gst_dmabuf_allocator_obtain(). It's a singleton, no new instance is returned here
Also add documentation

@@ +226,3 @@
+
+GstMemory *
+gst_dmabuf_allocator_alloc (GstAllocator * allocator, gint fd, gsize size)

Documentation.
Is a dmabuf always completely specified with the fd and size? No flags or anything else, like for specifying a read-only dmabuf?

@@ +263,3 @@
+ */
+gint
+gst_dmabuf_memory_get_fd (GstMemory * mem)

Do a g_return_val_if_fail(gst_is_dmabuf_memory(mem), -1) here. It should IMHO be the developers task to make sure it actually is dmabuf memory

@@ +282,3 @@
+{
+  gboolean ret = GST_IS_DMABUF_ALLOCATOR (mem->allocator);
+  ret &= g_strcmp0 (mem->allocator->mem_type, ALLOCATOR_NAME) == 0;

Only check the mem_type here, no the allocator. There could in theory be other allocators in the future that return dmabuf memory (and for them we would need to make part of the internal structs here public).
Comment 13 Benjamin Gaignard 2013-02-18 11:22:09 UTC
Created attachment 236577 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

This patch create a new libgstallocators lib in -base/gst-libs.
dmabuf allocator is included in this lib.

In userspace dmabuf is only using file descriptors so there is no system dependence to be check, dma-buf.h file is for kernel usage only and isn't exported in /usr/include directory.

I hope that this patch fix most of the comments done before :)
Comment 14 Benjamin Gaignard 2013-02-18 11:23:44 UTC
Created attachment 236578 [details] [review]
additional path for v4l2src

This patch add dmabuf support to v4l2 buffer pool.

This now use lib gstallocators from -base and gstdmabuf.h exported by -base too.
Comment 15 Sebastian Dröge (slomo) 2013-02-18 11:35:34 UTC
Review of attachment 236577 [details] [review]:

Just mostly nitpicking now, looks good :)

::: configure.ac
@@ +845,3 @@
 gst-libs/Makefile
 gst-libs/gst/Makefile
+gst-libs/gst/allocators/Makefile

There should be a configure check for checking if dmabuf is supported or not... which then enables compilation of the dmabuf code or not. The library should be built all the time though, and there should be a gstallocatorsconfig.h or something like that that specifies which allocators are available.

::: gst-libs/gst/allocators/Makefile.am
@@ +16,3 @@
+
+if HAVE_INTROSPECTION
+BUILT_GIRSOURCES = Gstallocators-@GST_API_VERSION@.gir

Uppercase 'a', GstAllocators-  (also everywhere else of course)

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +154,3 @@
+{
+  gint newfd = dup (mem->fd);
+  g_return_val_if_fail (newfd != -1, NULL);

I don't think this should be an assertion... or is it guaranteed that all dmabufs are always dup'able?

@@ +206,3 @@
+  GstAllocator *allocator =
+      g_object_new (dmabuf_mem_allocator_get_type (), NULL);
+  gst_allocator_register (ALLOCATOR_NAME, gst_object_ref (allocator));

Why the additional ref here?

@@ +245,3 @@
+  if (!allocator) {
+    GST_WARNING ("missing allocator");
+    return NULL;

You could let it use gst_dmabuf_allocator_obtain() if the allocator is NULL

::: pkgconfig/gstreamer-allocators-uninstalled.pc.in
@@ +12,3 @@
+Version: @VERSION@
+Requires: gstreamer-@GST_API_VERSION@
+Libs: @abs_top_builddir@/gst-libs/gst/allocators/libgstallocators-@GST_API_VERSION@.la @LIBM@

You don't need libm
Comment 16 Benjamin Gaignard 2013-02-18 13:48:31 UTC
Created attachment 236588 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

this patch check if mmap is available or not.
Comment 17 Sebastian Dröge (slomo) 2013-02-18 14:04:58 UTC
Review of attachment 236588 [details] [review]:

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +23,3 @@
+#include "gstdmabuf.h"
+#include <sys/mman.h>
+#include <unistd.h>

These two includes should be in the #ifdef HAVE_MMAP block too

@@ +42,3 @@
+  gint mmap_count;
+  GMutex lock;
+} GstDmaBufMemory;

same for this

@@ +220,3 @@
+ * gst_dmabuf_allocator_obtain
+ * return a dmabuf allocator
+ * Use gst_object_unref() to release the allocator after usage.

Document that this can return NULL and when
Comment 18 Benjamin Gaignard 2013-02-18 14:19:34 UTC
Created attachment 236590 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf
Comment 19 Sebastian Dröge (slomo) 2013-02-18 14:37:49 UTC
Review of attachment 236590 [details] [review]:

Looks good to me now :)
Comment 20 Sebastian Dröge (slomo) 2013-02-19 09:06:47 UTC
Comment on attachment 236590 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

commit 26ff0ced15cbfd9b2c892b5f8b5a7792a2e9fb18
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Feb 19 10:05:17 2013 +0100

    allocators: Integrate into the documentation

commit d405dddd507f0503e5b10344703406749b1d0762
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Feb 19 09:40:42 2013 +0100

    allocators: Add .def file with all exports

commit 37f0067946af9de9404093a90a3896d3e5461092
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Feb 19 09:39:24 2013 +0100

    allocators: Add single-include header

commit 76400ef22684b72a1155b82b15a0fea797eb70c0
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Feb 19 09:35:51 2013 +0100

    dmabuf: Improve documentation and annotations a bit

commit ceecdb8e1ddabf96ced519c40fb80104b380befb
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Mon Feb 18 15:18:38 2013 +0100

    allocators: Add dmabuf-based GstMemory and GstAllocator
    
    Create new GstMemory and GstAllocator base on dmabuf.
    Memory is not allocated/freed by userland but mapped/unmmaped
    from a dmabuf file descriptor when requested.
    
    This allocator is included in a new lib called libgstallocators
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693826
Comment 21 Sebastian Dröge (slomo) 2013-02-19 09:12:58 UTC
Review of attachment 236578 [details] [review]:

::: configure.ac
@@ +582,3 @@
+# Check if v4l2 support dmabuf
+if test x$HAVE_GST_V4L2 = xyes; then
+	AC_CHECK_DECLS(V4L2_MEMORY_DMABUF,,,[

Can't you just check #ifdef V4L2_MEMORY_DMABUF inside the code?

::: sys/v4l2/gstv4l2bufferpool.c
@@ +206,3 @@
+
+        meta->vbuffer.memory = V4L2_MEMORY_DMABUF;
+        pool->allocator = gst_dmabuf_allocator_obtain ();

For every buffer you set a new allocator in the pool, increasing the refcount once per buffer. The allocator should probably also be set in the buffer pool config somehow
Comment 22 Benjamin Gaignard 2013-02-19 10:51:20 UTC
Created attachment 236740 [details] [review]
additional path for v4l2src

the test of VIDIOC_EXPBUF could not be done with a #ifdef (like V4L2_CAP_VIDEO_OUTPUT_OVERLAY below in the code) because it is a value in an enum and not a #define in videodev2.h
Comment 23 Benjamin Gaignard 2013-02-19 11:08:26 UTC
Created attachment 236741 [details] [review]
add missing config.h file on top of gstdmabuf.c

add missing config.h file on top of gstdmabuf.c
Comment 24 Sebastian Dröge (slomo) 2013-02-19 11:46:56 UTC
Comment on attachment 236741 [details] [review]
add missing config.h file on top of gstdmabuf.c

commit 9a69f66ed111883e67e85c956bbd009f2d3cdb7c
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Tue Feb 19 11:52:22 2013 +0100

    dmabuf: Include config.h
Comment 25 Sebastian Dröge (slomo) 2013-02-19 11:57:49 UTC
commit e29ab42922706e0c4a1bb3fc8041c9a9dac6d4f1
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Tue Feb 19 11:47:20 2013 +0100

    v4l2: Add support of dmabuf
    
    v4l has add a new IOCTL to export a buffer by using dmabuf.
    This patch allow to use this new IOTCL if it has been defined in videodev2.h
    I introduce a new IO mode (GST_V4L2_IO_DMABUF) to enable this way of working.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693826