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 700411 - dmabuf: Make sure that memory is unmapped before releasing it
dmabuf: Make sure that memory is unmapped before releasing it
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.x
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-15 20:39 UTC by Benjamin Gaignard
Modified: 2013-05-17 09:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
optimize dmabuf map/unmap (2.63 KB, patch)
2013-05-15 20:39 UTC, Benjamin Gaignard
none Details | Review
make sure that memory is unmapped (1.87 KB, patch)
2013-05-16 10:05 UTC, Benjamin Gaignard
needs-work Details | Review
make sure that memory is unmapped (1.87 KB, patch)
2013-05-16 11:46 UTC, Benjamin Gaignard
needs-work Details | Review
make sure that memory is unmapped (1.56 KB, patch)
2013-05-17 07:17 UTC, Benjamin Gaignard
committed Details | Review

Description Benjamin Gaignard 2013-05-15 20:39:22 UTC
Created attachment 244354 [details] [review]
optimize dmabuf map/unmap

Simplify dmabuf allocator to avoid call mmap and munmap after each gst_buffer_{map/unmap} request.

Memory is mmap at the first call of gst_buffer_map and is unmap when the buffer released.
Comment 1 Sebastian Dröge (slomo) 2013-05-16 07:27:30 UTC
I don't think this is correct. After unmapping the hardware/driver has the possibility to update its internal state with any changed memory, and especially some API could use the dmabuf directly instead of the mmap'd memory after unmapping.
Comment 2 Benjamin Gaignard 2013-05-16 09:54:16 UTC
OK, I catch your point.

If the memory is still mapped when we want to release it, I think we should call unmap anyway.
Reworked path do this and some clean up in variables names.
Comment 3 Benjamin Gaignard 2013-05-16 10:05:10 UTC
Created attachment 244384 [details] [review]
make sure that memory is unmapped
Comment 4 Sebastian Dröge (slomo) 2013-05-16 10:28:16 UTC
Review of attachment 244384 [details] [review]:

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +69,3 @@
+  if (mem->data) {
+    GST_INFO("Freeing memory still mapped");
+    munmap ((void *) mem->data, mem->mmap_size);

This should be a g_warning(), it's a programming error to free memory that is still mapped.

@@ +115,3 @@
     mem->mmap_size = maxsize;
     mem->mmap_count++;
+    gmem->size = maxsize;

Why?
Comment 5 Benjamin Gaignard 2013-05-16 11:44:39 UTC
I will change GST_INFO to g_warning.

gmem->size is needed to avoid this error message:
gst-plugins-base/gst-libs/gst/video/video-frame.c:147:gst_video_frame_map_id: invalid buffer size 0 < 1639680

frame->map[0].size value is set from GstMemory size field in gst_memory_map function.
Comment 6 Benjamin Gaignard 2013-05-16 11:46:08 UTC
Created attachment 244397 [details] [review]
make sure that memory is unmapped
Comment 7 Sebastian Dröge (slomo) 2013-05-17 06:38:22 UTC
Review of attachment 244397 [details] [review]:

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +69,2 @@
+  if (mem->data) {
+    g_warning("Freeing memory still mapped");

Best would be to add some information here about here this happens, e.g.
g_warning (G_STRLOC ":%s: Freeing memory %p still mapped", G_STRFUNC, mem)

@@ +115,3 @@
     mem->mmap_size = maxsize;
     mem->mmap_count++;
+    gmem->size = maxsize;

This should already be fixed by

commit ced858fa657abe7422e0d5da3a7247570fef7605
Author: Michael Olbrich <m.olbrich@pengutronix.de>
Date:   Thu May 16 09:07:46 2013 +0200

    dmabuf: set the initial memory size to the full size
    
    https://bugzilla.gnome.org/show_bug.cgi?id=700427
Comment 8 Benjamin Gaignard 2013-05-17 07:17:57 UTC
Created attachment 244485 [details] [review]
make sure that memory is unmapped

rmove gmem->size assignment the previous fix that problem
Comment 9 Sebastian Dröge (slomo) 2013-05-17 07:50:47 UTC
commit e90e2bb8221b2671ecd2e19f48201b5871f3c6a5
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Fri May 17 09:16:08 2013 +0200

    dmabuf: Make sure that memory is unmapped before releasing it
    
    Be sure that memory is unmapped before releasing it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=700411