I've been thinking about this. It shouldn't be too difficult to use EXPBUF and the dmabuf allocator in the MMAP use-case, and fall back to the current mmap if EXPBUF fails. I'd think this can be done unconditionally, or are there reasons to make this optional?

V4L2_MEMORY_DMABUF is something else. GstV4l2BufferPool currently mixes the gstreamer buffer pool stuff with v4l2 buffer handling. So working with buffers from another pool is not easy to implement.

This bug has been introduce by my patch because I have done an error while back porting v4l2 dmabuf from kernel 3.9 to 3.4.... sorry for that.

My goal was (is) to make X sink and v4l2src share dmabuf buffers. When X sink will be able to export a pool of dmabuf buffers, v4l2src will only have to queue/dequeue V4L2_MEMORY_DMABUF buffers, no needs of V4L2_MEMORY_MMAP + VIDIOC_EXPBUF is this case.

Any news on this?

To be sure to correctly implemented/test it this time, I would like to have a downstream element able to provide buffers from a dmabuf allocator. 

A first step was to have a way to test allocator type: https://bugzilla.gnome.org/show_bug.cgi?id=703659

I'm working on waylandsink to add support of wayland DRM protocol and make it aware of dmabuf allocator. I hope it will be enough to test pool negotiation with v4l2src.

Are the dmabuf buffers coming from the waylandsink linked in their lifetime to the duration of the connection to the wayland compositor? In v4l2src, as long as a single dmabuf allocated buffer from the v4l2 device is alive, one can't change the REQBUFS.

Any progress here, what can we do about this?

Adding some thought, note that even if you negotiated everything correctly, the dmabuf you get from upstream might still not be usable for your device driver. V4L2 driver may use different memory model, e.g. UVC could (if it was implement, I think Hans has a branch), import any dmabuf with supported alignment and stride, because it's using the vmalloc allocator. Though, most driver use the contiguous allocator, for which a vmalloc dmabuf (e.g. exported by UVC) would not work. So, just like we are doing in the USERPTR experimental branch, we need all appropriate fallback.

Final note, v4l2 really need to track it's memory, since it can't detach from it. I'm working on an allocator for that, but I have the impression that the dmabuf allocator will be unusable in that context since we still have to track our buffers, something the dmabuf allocator don't do (it also don't allocate anything, as allocating dmabuf is a per-driver action). Anything I'm missing ? Was this considered in this attempt to provide an allocator ?

Linaro is working on a "central dma-buf allocator".

One of the goal of this allocator is to solve the problem of memory you describe by implementing "delayed allocation" feature. The devices have to attach themselves and their memory constraints to the buffer with a call to dma_buf_attach. The buffer allocation will be done only at the first call of dma_buf_map_attachement, at this time all the devices memory constraints should have been collected and the "central allocator" could select an allocator (vmalloc, dma-coherent ...) that match with those constraints.

The big question is when, and if Linaro will port all the existing drivers (something that never happened in the v4l2 space). It's good to know, but nothing we can use at the moment, and the current dmabuf allocator is still useless because it prevents the elements from tracking memory by the driver (hence prevent verifying they are reclaimed before changing format, or shutting down).

"delayed allocation" does not really help with gstreamer and v4l2. By the time the sink can call dma_buf_attach the buffer was already written if the source provides the buffer. To handle this, it must also be possible to migrate the buffer when attaching with incompatible memory constraints.

I think we're much further now. Though still need a mechanic or options to correctly advertise dmabuf and automatically use it when possible. For v4l2 we could always export, I didn't notice any performance difference here.

As Benjamin mention on IRC, we can play with the presence of the dmabuf allocator in the query. To decide to export, for upstream pool (decide_allocation), we'd look if the allocator is proposed in the list of allocators, if so export (using that allocator). For downstream pool, we'd offer our own allocator and the dmabuf allocator, and decide to export base on what has been configured on the pool.

For the import case there is two scenario. When importing from downstream pool (decide allocator), we would try and find a dmabuf allocator, if we find it, we'd set that in the config and then import. When buffers comes from upstream, we can't do that in advance, so we'll have to make the decision on first buffer. Though, on first frame it's fine to try and fallback, later it will probably be harder to switch.

Note that code like v4l2 that ignore downstream pool in certain cases will need to be reviewed, as normally the proposed allocator might be specialised, and only usable with the associated pool. The allocator with custom allocation method (unless known) will need to be skipped.

Created attachment 291100 [details] [review]
allow use of dmabuf

With this patch I get my video decoder exporting dmabuf file descriptors but it doesn't set in the query that the pool have a dmabuf allocator.

I need your opinion to be sure I going in the good direction.

Review of attachment 291100 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +2290,3 @@
+    if (v4l2object->req_mode == GST_V4L2_IO_AUTO) {
+      GstV4l2BufferPool *vpool = GST_V4L2_BUFFER_POOL_CAST(v4l2object->pool);
+      if (GST_OBJECT_FLAG_IS_SET(vpool->vallocator, GST_V4L2_ALLOCATOR_FLAG_DMABUF_REQBUFS))

I would prefer if you use the marco:
GST_V4L2_ALLOCATOR_CAN_REQUEST (vpool->allocator, DMABUF)

@@ -2296,6 +2307,0 @@
-  /* Map the buffers */
-  GST_LOG_OBJECT (v4l2object->element, "initiating buffer pool");
-
... 3 more ...

I'm not sure I understand this part, why would setup_pool() not setup the pool anymore ?

The only I have found to know if the driver support DMABUF is to test the allocator flags so I have to create the pool before select the mode.

If we don't want the sequence a solution could be to call gst_v4l2_allocator_new() to get a temporary allocator, test the flags and release the allocator.

We can probably move the probing outside of the allocator.

I have split the patch in two parts: one to move vb queue probing in to v4l2object instead of v4l2allocator. The flags propagate from v4l2object to v4l2bufferpool and then v4l2allocator.

The second patch allow to select dmabuf (export) if the vb queue support it.

Created attachment 291164 [details] [review]
change probing

Created attachment 291165 [details] [review]
allow use of dmabuf

Review of attachment 291164 [details] [review]:

That actually seems like a good idea. One small detail, after what I'm all happy with it.

::: sys/v4l2/gstv4l2object.c
@@ +2268,3 @@
+
+static guint32
+gst_v4l2_object_probe (GstV4l2Object * v4l2object, guint32 memory,

Small fix required here, probe is too generic name now, we probe a lot more stuff in v4l2 object.

Review of attachment 291165 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +3378,3 @@
+    if (allocator)
+      gst_object_unref (allocator);
+    allocator = gst_object_ref (GST_V4L2_BUFFER_POOL (obj->pool)->allocator);

This part should be removed. You can't really do that, since in this particular case, the buffer pool ignores the config and use a DMA buf allocator, and as set_config() has not been called, it not yet the allocator that will be used.

I suppose you would like to see the DMABUF allocator in the pool config for when you receive the buffer downstream (I think you should peek the memories, but both way works). If that is the case, you should simply add code in set_config() of the pool to also update the allocator. I just didn't thought about it, so there is no code yet.

Created attachment 291186 [details] [review]
change probing

Created attachment 291187 [details] [review]
allow use of dmabuf

Review of attachment 291186 [details] [review]:

All good.

Review of attachment 291187 [details] [review]:

Good.

These patch don't apply against git master, would it be possible to rebase/resubmit ?

Ok, I understood these where written for 1.4 and manage to handle it. Please, provide patch on top of git master next time. Also, we don't use the Signed-off notation in this project, but we do cross reference of bugs. So instead of your signed off next time add a line with the complete bug URI

objectname: Short title

Long description.

https://bugzilla.gnome.org/show_bug.cgi?id=699382

Comment on attachment 291186 [details] [review]
change probing

commit ec6b8b84af719d828ddd91c724e715c0b4a556bc
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Fri Nov 21 16:13:05 2014 +0100

    v4l2: move vb_queue probing from allocator to v4l2object
    
    The goal is to make those information available in v4l2_object
    to be able later to select the best allocation method for the pool
    
    https://bugzilla.gnome.org/show_bug.cgi?id=699382

Comment on attachment 291187 [details] [review]
allow use of dmabuf

commit e6c2ad5571e5dedb212287efe238eb450032cd4f
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Fri Nov 21 16:36:15 2014 +0100

    v4l2object: allow to automatic selection of dmabuf
    
    If the v4l2 queue support dmabuf select this buffer pool mode
    and update the query with allocator.
    This patch only concern exporting dmabuf and not importing dmabuf
    fd from downstream element.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=699382

And this one, to fix the build.

commit ad4480d53408a4d97ab531174ef37f258f3253c0
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Fri Nov 21 11:44:24 2014 -0500

    v4l2allocator: Remove unused variable
    
    this was introduced by commit ec6b8b
    
    https://bugzilla.gnome.org/show_bug.cgi?id=6993

Next step would be the importation part, I guess I will create new bugs for that.

- Import from downstream pool (v4l2src, m2m capture)
- Import from upstream (v4l2sink, m2m output)

(In reply to comment #28)
> (From update of attachment 291186 [details] [review])
> commit ec6b8b84af719d828ddd91c724e715c0b4a556bc
> Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Date:   Fri Nov 21 16:13:05 2014 +0100
> 
>     v4l2: move vb_queue probing from allocator to v4l2object

This one breaks "v4l2src ! xvimagesink" or "v4l2src ! fakesink"

0:00:00.067722042 26721      0x2147680 ERROR          v4l2allocator gstv4l2allocator.c:679:gst_v4l2_allocator_start:<v4l2src0:pool:src:allocator> error requesting 3 buffers: Invalid argument
0:00:00.067745145 26721      0x2147680 ERROR                   v4l2 gstv4l2bufferpool.c:774:gst_v4l2_buffer_pool_start:<v4l2src0:pool:src> we received 0 buffer from device '/dev/video0', we want at least 2
0:00:00.067753620 26721      0x2147680 ERROR             bufferpool gstbufferpool.c:533:gboolean gst_buffer_pool_set_active(GstBufferPool *, gboolean):<v4l2src0:pool:src> start failed

This is with a Logitech c920 and Linux 3.16 btw

Was taking too much time to figure-out so:

commit ea4d9745e473ea63ded7fe20df1c9024ee9d705c
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Mon Nov 24 10:36:54 2014 -0500

    Revert "v4l2allocator: Remove unused variable"
    
    This reverts commit ad4480d53408a4d97ab531174ef37f258f3253c0.

commit 43d5a523f11127e1a21388b6a22ca3b9f37f333d
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Mon Nov 24 10:36:30 2014 -0500

    Revert "v4l2: move vb_queue probing from allocator to v4l2object"
    
    This reverts commit ec6b8b84af719d828ddd91c724e715c0b4a556bc.

commit 3591a91067232a8daeae6c27f000d9579ca4b0c7
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Mon Nov 24 10:33:29 2014 -0500

    Revert "v4l2object: allow to automatic selection of dmabuf"
    
    This reverts commit e6c2ad5571e5dedb212287efe238eb450032cd4f.

This need further debugging. While doing short investigation, I at least spotted 1 thing wrong, we check if we can import DMABUF in order to default to exporting DMABUF, that is wrong of course. The only way to check if we can export DMABUF is to actually export DMABUF, limitation of V4L2 API.

With current kernels we could probably call EXPBUF before REQBUFS. We should get ENOTTY if it's not supported and EINVAL otherwise. I'm not sure how older kernels would handle this though.

But I think it would be better if we talk to the kernel guys. Maybe we can get a flag for this.

Few patches have just been pushed in v4l2 dev tree to support exportbufs for more memory type:
http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=e078b79d8aa70a48fb3fa684e6a6548c5127646b

http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=041c7b6ac74ee7a4375faf80e2864fc2bce78edc

http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=4650635069dfc9f7d59a55e048560bf05e3ca442

http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=952768c41a993dc1d813baa29d87ecb3da9b266e

http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=f5294f455afd30bdc90f31d6d0101bb773e9ddba

(In reply to comment #35)
> With current kernels we could probably call EXPBUF before REQBUFS. We should
> get ENOTTY if it's not supported and EINVAL otherwise. I'm not sure how older
> kernels would handle this though.
> 
> But I think it would be better if we talk to the kernel guys. Maybe we can get
> a flag for this.

Seems like a fair suggestion. I'll have a chat with them at least, to know if this has any change to work long enough (and work on older drivers).

Ok, had to scratch my head a little on why the "moving probe" patch was wrong. Turns out the reason is that GstV4l2Object isn't not a GstObject, neither is it is a GObject. Using GST_OBJECT_FLAG_SET() won't do here.

The remaining is being taken care in bug 779466 now. Let's get rid of this one, and file spcefic bugs for specific issues.