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 699382 - v4l2: dmabuf handling is not complete
v4l2: dmabuf handling is not complete
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 699337 707534
Blocks:
 
 
Reported: 2013-05-01 13:43 UTC by Sebastian Dröge (slomo)
Modified: 2017-03-02 15:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
allow use of dmabuf (2.27 KB, patch)
2014-11-20 13:46 UTC, Benjamin Gaignard
reviewed Details | Review
change probing (11.67 KB, patch)
2014-11-21 13:53 UTC, Benjamin Gaignard
needs-work Details | Review
allow use of dmabuf (1.77 KB, patch)
2014-11-21 13:53 UTC, Benjamin Gaignard
needs-work Details | Review
change probing (11.76 KB, patch)
2014-11-21 15:38 UTC, Benjamin Gaignard
committed Details | Review
allow use of dmabuf (1.89 KB, patch)
2014-11-21 15:38 UTC, Benjamin Gaignard
committed Details | Review

Description Sebastian Dröge (slomo) 2013-05-01 13:43:12 UTC
+++ This bug was initially created as a clone of Bug #699337 +++

The whole DMABUF code is currently broken.
V4L2_MEMORY_DMABUF is an alternative to V4L2_MEMORY_USERPTR that can be used to queue other DMABUF file descriptors. To create new DMABUF file descriptors V4L2_MEMORY_MMAP + VIDIOC_EXPBUF must be used. The two modes cannot be mixed.
Right now QBUF fails because the device is configured for MMAP and a DMABUF buffer is queued.
Comment 1 Michael Olbrich 2013-05-02 09:45:38 UTC
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.
Comment 2 Benjamin Gaignard 2013-05-02 11:55:23 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2013-07-17 13:17:59 UTC
Any news on this?
Comment 4 Benjamin Gaignard 2013-07-17 13:28:36 UTC
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.
Comment 5 Olivier Crête 2013-09-05 03:25:46 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2013-12-17 15:17:22 UTC
Any progress here, what can we do about this?
Comment 7 Nicolas Dufresne (ndufresne) 2014-03-29 21:02:29 UTC
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 ?
Comment 8 Benjamin Gaignard 2014-03-31 09:27:38 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2014-03-31 13:10:45 UTC
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).
Comment 10 Michael Olbrich 2014-03-31 13:51:57 UTC
"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.
Comment 11 Nicolas Dufresne (ndufresne) 2014-05-08 20:03:29 UTC
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.
Comment 12 Nicolas Dufresne (ndufresne) 2014-05-17 02:16:45 UTC
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.
Comment 13 Benjamin Gaignard 2014-11-20 13:46:04 UTC
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.
Comment 14 Nicolas Dufresne (ndufresne) 2014-11-20 15:04:14 UTC
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 ?
Comment 15 Benjamin Gaignard 2014-11-20 15:53:23 UTC
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.
Comment 16 Nicolas Dufresne (ndufresne) 2014-11-20 16:01:18 UTC
We can probably move the probing outside of the allocator.
Comment 17 Benjamin Gaignard 2014-11-21 13:52:47 UTC
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.
Comment 18 Benjamin Gaignard 2014-11-21 13:53:25 UTC
Created attachment 291164 [details] [review]
change probing
Comment 19 Benjamin Gaignard 2014-11-21 13:53:50 UTC
Created attachment 291165 [details] [review]
allow use of dmabuf
Comment 20 Nicolas Dufresne (ndufresne) 2014-11-21 14:19:57 UTC
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.
Comment 21 Nicolas Dufresne (ndufresne) 2014-11-21 14:42:05 UTC
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.
Comment 22 Benjamin Gaignard 2014-11-21 15:38:01 UTC
Created attachment 291186 [details] [review]
change probing
Comment 23 Benjamin Gaignard 2014-11-21 15:38:26 UTC
Created attachment 291187 [details] [review]
allow use of dmabuf
Comment 24 Nicolas Dufresne (ndufresne) 2014-11-21 16:20:43 UTC
Review of attachment 291186 [details] [review]:

All good.
Comment 25 Nicolas Dufresne (ndufresne) 2014-11-21 16:21:50 UTC
Review of attachment 291187 [details] [review]:

Good.
Comment 26 Nicolas Dufresne (ndufresne) 2014-11-21 16:25:08 UTC
These patch don't apply against git master, would it be possible to rebase/resubmit ?
Comment 27 Nicolas Dufresne (ndufresne) 2014-11-21 16:32:55 UTC
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 28 Nicolas Dufresne (ndufresne) 2014-11-21 16:44:38 UTC
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 29 Nicolas Dufresne (ndufresne) 2014-11-21 16:45:49 UTC
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
Comment 30 Nicolas Dufresne (ndufresne) 2014-11-21 16:47:36 UTC
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)
Comment 31 Sebastian Dröge (slomo) 2014-11-24 14:05:56 UTC
(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"
Comment 32 Sebastian Dröge (slomo) 2014-11-24 14:06:37 UTC
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
Comment 33 Sebastian Dröge (slomo) 2014-11-24 14:09:10 UTC
This is with a Logitech c920 and Linux 3.16 btw
Comment 34 Nicolas Dufresne (ndufresne) 2014-11-24 15:44:08 UTC
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.
Comment 35 Michael Olbrich 2014-11-25 11:34:48 UTC
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.
Comment 37 Nicolas Dufresne (ndufresne) 2014-11-25 14:15:34 UTC
(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).
Comment 38 Nicolas Dufresne (ndufresne) 2014-12-09 00:01:47 UTC
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.
Comment 39 Nicolas Dufresne (ndufresne) 2017-03-02 15:32:52 UTC
The remaining is being taken care in bug 779466 now. Let's get rid of this one, and file spcefic bugs for specific issues.