GNOME Bugzilla – Bug 700411
dmabuf: Make sure that memory is unmapped before releasing it
Last modified: 2013-05-17 09:59:48 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.
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.
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.
Created attachment 244384 [details] [review] make sure that memory is unmapped
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?
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.
Created attachment 244397 [details] [review] make sure that memory is unmapped
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
Created attachment 244485 [details] [review] make sure that memory is unmapped rmove gmem->size assignment the previous fix that problem
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