Created attachment 363033 [details] [review]
fbmemory: need unmap if mapping flags are not subset of previous

Review of attachment 363033 [details] [review]:

What's the exact sequence of events you're attempting to allow?

e.g.
gst_memory_map(READ)
gst_memory_map(WRITE)
gst_memory_unmap(READ)
gst_memory_unmap(WRITE)

or,

gst_memory_map(READ)
gst_memory_unmap(READ)
gst_memory_map(WRITE)
gst_memory_unmap(WRITE)

The first is unsupported and should return NULL.  The second is valid usage though.

::: gst-libs/gst/allocators/gstfdmemory.c
@@ +103,3 @@
+      munmap ((void *) mem->data, gmem->maxsize);
+      mem->data = NULL;
+      mem->mmap_count = 0;;

And how do we know that this is safe?  i.e. another user does not have another mapping?

(In reply to Matthew Waters (ystreet00) from comment #2)
> Review of attachment 363033 [details] [review] [review]:
> 
> What's the exact sequence of events you're attempting to allow?
> 
> e.g.
> gst_memory_map(READ)
> gst_memory_map(WRITE)
> gst_memory_unmap(READ)
> gst_memory_unmap(WRITE)
> 
> or,
> 
> gst_memory_map(READ)
> gst_memory_unmap(READ)
> gst_memory_map(WRITE)
> gst_memory_unmap(WRITE)
> 
> The first is unsupported and should return NULL.  The second is valid usage
> though.

the second one actually. But dmabuf allocator set the keep mapped flag, so when do gst_memory_unmap(READ) it just return and do nothing. Which will cause the write map fail

> 
> ::: gst-libs/gst/allocators/gstfdmemory.c
> @@ +103,3 @@
> +      munmap ((void *) mem->data, gmem->maxsize);
> +      mem->data = NULL;
> +      mem->mmap_count = 0;;
> 
> And how do we know that this is safe?  i.e. another user does not have
> another mapping?

The keep_map flag should be smarter, otherwise it's just a terribly friend. I didn't know this flag was enabled for dmabuf, it's a bit unfortunate, since it a total breakage of DMABuf usage. You must unmap if you don't implement DMA_SYNC. I'm find that patch and revert.

I'm sorry, there is nothing to revert upstream, this flag is not set on the DMABuf allocator. Not sure why it is for you.

(In reply to Nicolas Dufresne (stormer) from comment #5)
> I'm sorry, there is nothing to revert upstream, this flag is not set on the
> DMABuf allocator. Not sure why it is for you.

Sorry for that, there is an internal patch on my side which change dmabuf to keep mapped. But this is really a problem for a subclass of fbmemory which set this flags. The flag is not so smarter as we think

Agreed, I thinks this bug report is completely valid. Maybe we could implement some "lazy" remapping when there is a transition from "keep mapped" read to "keep map" write ? We would then upgrade the map from read to readwrite ? Would that make sense ?

For anything DMABuf though, enabling this flag requires to use DMA_BUF_IOCTL_SYNC() READ/WRITE/RW accordingly around the read/write maps. Otherwise, there is a risk of loosing data that might still be on the CPU cache. The API is quite straight forward though, it's basically a 1 to 1 to map operations.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/396.