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 730441 - dmabuf: shared the mapping with shared copies of the memory
dmabuf: shared the mapping with shared copies of the memory
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal blocker
: 1.3.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-20 12:46 UTC by Michael Olbrich
Modified: 2014-07-11 12:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (3.52 KB, patch)
2014-05-20 12:46 UTC, Michael Olbrich
committed Details | Review
[PATCH] v4l2allocator: Use qdata instead of parenting to DmabufMemory (3.50 KB, patch)
2014-07-10 22:12 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] dmabuf: Ensure _get_fd() works even for shared memory (1.52 KB, patch)
2014-07-10 22:18 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Michael Olbrich 2014-05-20 12:46:06 UTC
Created attachment 276856 [details] [review]
patch

With lots of shared memory instances (e.g. created by a RTP payloader) the
overhead of duplicating the file descriptor and crating extra mappings is
significant.
To avoid this, the parent memory maps the whole region and the shared
copies just reuse the same mapping.
Comment 1 Nicolas Dufresne (ndufresne) 2014-05-20 13:40:07 UTC
Review of attachment 276856 [details] [review]:

Looks good to me. Duplication is only needed for sharing that FD with another process in fact. Also, one less dup() which can leak if during fork() (see dup3).
Comment 2 Nicolas Dufresne (ndufresne) 2014-05-20 13:45:48 UTC
Comment on attachment 276856 [details] [review]
patch

commit b60888fd4bcacd42bb4e27fa938272d6e72c5c32
Author: Michael Olbrich <m.olbrich@pengutronix.de>
Date:   Tue May 20 11:18:56 2014 +0200

    dmabuf: share the mapping with shared copies of the memory
    
    With lots of shared memory instances (e.g. created by a RTP payloader) the
    overhead of duplicating the file descriptor and creating extra mappings is
    significant. To avoid this, the parent memory maps the whole region and the
    shared copies just reuse the same mapping.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730441
Comment 3 Nicolas Dufresne (ndufresne) 2014-05-20 13:46:31 UTC
Thanks for your patch.
Comment 4 Nicolas Dufresne (ndufresne) 2014-07-10 21:02:17 UTC
This patch is actually causing regressions. On a shared dmabuf memory, calling gst_dmabuf_memory_get_fd() will return -1.

Other problems, the parent memory is no longer always a dmabuf memory, hence calling gst_dmabuf_map (parent, ...) will lead to crash.

A solution would be:

- Implement dmabuf mapping in V4l2Memory
- Change call to gst_dmabuf_map() into gst_memory_map()
- Set the FD correctly on _share, close only if no parent

This way we would not have to dup the FD in v4l2, though small some code duplication will exist between the allocator and v4l2.

This is a lot so close to a release, reverting would also fix the regression, with the added cost of doing a lot of dup().
Comment 5 Nicolas Dufresne (ndufresne) 2014-07-10 22:11:06 UTC
Actually, if I port v4l2 to use a qdata and it's destroy notify, all I have to do is keep the FD around, and check if parent == NULL before closing the dmabuf fd.
Comment 6 Nicolas Dufresne (ndufresne) 2014-07-10 22:12:19 UTC
Created attachment 280454 [details] [review]
[PATCH] v4l2allocator: Use qdata instead of parenting to DmabufMemory


Parenting V4l2Memory to DmabufMemory was in conflict with recent
optimization in DmabufMemory to avoid dup(), and didn't work with
memory sharing. Instead, use a qdata and it's destroy notify.

https://bugzilla.gnome.org/show_bug.cgi?id=73044
---
 sys/v4l2/gstv4l2allocator.c  | 22 ++++++++++++++--------
 sys/v4l2/gstv4l2allocator.h  |  4 ++++
 sys/v4l2/gstv4l2bufferpool.c |  5 +++--
 3 files changed, 21 insertions(+), 10 deletions(-)
Comment 7 Nicolas Dufresne (ndufresne) 2014-07-10 22:18:27 UTC
Created attachment 280455 [details] [review]
[PATCH] dmabuf: Ensure _get_fd() works even for shared memory


Fixes regression introduced by:

commit b60888fd4bcacd42bb4e27fa938272d6e72c5c32
Author: Michael Olbrich <m.olbrich@pengutronix.de>
Date:   Tue May 20 11:18:56 2014 +0200

    dmabuf: share the mapping with shared copies of the memory

https://bugzilla.gnome.org/show_bug.cgi?id=73044
---
 gst-libs/gst/allocators/gstdmabuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 8 Nicolas Dufresne (ndufresne) 2014-07-10 22:19:43 UTC
With these two, we can avoid reverting the optimization, careful review would be appreciated.
Comment 9 Sebastian Dröge (slomo) 2014-07-11 07:51:52 UTC
Thanks Nicolas! The qdata stuff looks very suspicious and should probably be revisited in 1.6 but it should work that way... just doesn't look very clean :)

commit 0ac0cbcc0eb3eb7ae5f07bc6c0d5ef6469d483ba
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Jul 10 18:11:20 2014 -0400

    v4l2allocator: Use qdata instead of parenting to DmabufMemory
    
    Parenting V4l2Memory to DmabufMemory was in conflict with recent
    optimization in DmabufMemory to avoid dup(), and didn't work with
    memory sharing. Instead, use a qdata and it's destroy notify.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730441
Comment 10 Michael Olbrich 2014-07-11 07:59:05 UTC
The patch for gstdmabuf.c looks good and I didn't see any regessions with it.
But I wonder if this could be solved differently:

delegate _get_fd() to the parent if fd == -1 and delegate _map()/_unmap() only to the parent if fd == -1

That should fix _get_fd() and make parenting V4l2Memory to DmabufMemory possible.
Comment 11 Sebastian Dröge (slomo) 2014-07-11 08:22:20 UTC
We should check if there are no assumptions elsewhere that the parent of a memory is assumed to be of the same type. But something like this is exactly what should be investigated for after 1.4 :)
Comment 12 Nicolas Dufresne (ndufresne) 2014-07-11 12:27:46 UTC
(In reply to comment #10)
> The patch for gstdmabuf.c looks good and I didn't see any regessions with it.
> But I wonder if this could be solved differently:
> 
> delegate _get_fd() to the parent if fd == -1 and delegate _map()/_unmap() only
> to the parent if fd == -1
> 
> That should fix _get_fd() and make parenting V4l2Memory to DmabufMemory
> possible.

That does not work if you share the memory. You will end up calling gst_dmabuf_get_fd () on a GstV4l2Memory. Also _map()/_unmap() is not implement for GstV4l2Memory (with DMABUF), so cannot be delagated.
Comment 13 Nicolas Dufresne (ndufresne) 2014-07-11 12:30:54 UTC
For your interest, I'm not a big fan for parenting the way I was doing. QData isn't too bad, but requires a big lock. In the long run, I though of generalizing allocator that hold FD in it. Would be an interface, from there, we could have a cleaner way to track FD based memory lifetime. In this case, the V4L2Memory is completely internal, it's to get the same code path for tracking memory being given back to the allocator.