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

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

Also, is the userspace mmap() part of dmabuf in the kernel now or still under discussion?

Created attachment 236225 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

fix comments done by Sebastian

Created attachment 236226 [details] [review]
additional path for v4l2src

change gst dmabuf alloc function name

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.

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

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

Created attachment 236253 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

Created attachment 236254 [details] [review]
additional path for v4l2src

Review of attachment 236226 [details] [review]:

obsolete

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).

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 :)

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.

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

Created attachment 236588 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

this patch check if mmap is available or not.

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

Created attachment 236590 [details] [review]
add a new GstMemory/GstAllocator based on dmabuf

Review of attachment 236590 [details] [review]:

Looks good to me now :)

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

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

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

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

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