GNOME Bugzilla – Bug 730441
dmabuf: shared the mapping with shared copies of the memory
Last modified: 2014-07-11 12:30:54 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.
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 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
Thanks for your patch.
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().
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.
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(-)
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(-)
With these two, we can avoid reverting the optimization, careful review would be appreciated.
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
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.
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 :)
(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.
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.