GNOME Bugzilla – Bug 768794
ion: DMA Buf allocator based on ion
Last modified: 2018-11-03 13:53:37 UTC
DMA Buf allocator based on ion
Created attachment 331464 [details] [review] DMA Buf allocator based on ion.
Review of attachment 331464 [details] [review]: Please run gst-indent on those, the coding style if largely wrong. Looks good in general, but I have few question: 1) How come the heap_mask is hardocded ? What odes 8 means ? 2) Wouldn't it be better to put this allocation into gstreamer-allocator library, and add support for that allocation in few elements ? 3) Is is possible to explain what one need to enable in a Linux Kernel to use this allocator ? 4) Having ion.h is probably insufficient to make sure we have ion in our kernel, how do we figure-out when this allocator should be used ?
Will fix coding style issue. GLdownloadelement can propose one GL buffer pool to upstream. The GL buffer pool allocate DMA FD memory from ION allocator. and than create one EGL image from DMA FD. So upstream can use GL buffer and downstream can use fd memory. Suppose GLupload also can propose GL buffer pool and dma-buf memory to avoid video copy. Is it reasonable? ion.h is sufficient for build. ion device open will fail when use the ion allocator if platform haven't ion driver.
Please click "reply" to reply to questions, so we can understand you. 1) isn't replied 2) isn't replied 3) isn't replied (In reply to kevin from comment #3) > Will fix coding style issue. > > GLdownloadelement can propose one GL buffer pool to upstream. The GL buffer > pool allocate DMA FD memory from ION allocator. and than create one EGL > image from DMA FD. So upstream can use GL buffer and downstream can use fd > memory. Ok, and are you going to implement that ? > > Suppose GLupload also can propose GL buffer pool and dma-buf memory to avoid > video copy. Is it reasonable? It's reasonable. In better words, you suggest using this allocator (when available) in DMABuf importer element like glupload, kmssink, v4l2 m2m, v4l2sink etc. > > ion.h is sufficient for build. ion device open will fail when use the ion > allocator if platform haven't ion driver. So that's a reply for 4), I guess importer will try to open the device to decided to use or not this allocator. Please reply to the other questions.
I will fix issues you mentioned in 1) and 2). DMABuf allocator based on ION can be used in below two case. gst-launch-1.0 videotestsrc ! glimagesink gst-launch-1.0 ... ! gldownloadelement ! filesink As EGL on Linux can create EGL image from DMA FD. This means GPU can import DMABuf, but can't export DMABuf. So need ION driver allocate DMABuf. On one hand, GPU can import DMABuf with EGL image. on there hand, CPU can access DMABuf after mmap DMABuf. So we can avoid video memory copy. We already implemented gldownloadelement based on ION allocator. Will implement glupload based on ION allocator if our idea is reasonable. Our implement include gldownloadelement propose one GL buffer pool based on ION allocator to upstream element and send one dmabuf buffer to downstream element. gluploadelement will propose GL buffer pool and dmabuf pool to upstream element. Upstream element can select dmabuf pool and send dmabuf buffer to gluploadelement even if the upstream element can't export DMABuf.
Created attachment 333309 [details] [review] Refine the patch.
Will submit glupload/gldownload based on ion allocator later in other thread.
Review of attachment 333309 [details] [review]: I think this should be inheriting from GstFDMemory instead of allocating GstDmabufMemory. You can then create a separate allocator that subclasses the Dmabuf allocator for the Dmabuf compatibility in gstreamer (although this code doesn't provide that so that may not be necessary). There's also no pkg-config file for this. ::: gst-libs/gst/ion/Makefile.am @@ +1,1 @@ +lib_LTLIBRARIES = libgstbadion-@GST_API_VERSION@.la Why is bad in the name? It's not conflicting with any other library. @@ +5,3 @@ + +libgstbadion_@GST_API_VERSION@_la_CFLAGS = $(GST_CFLAGS) \ + -DGST_USE_UNSTABLE_API What unstable API ar you using that requires this flag? ::: gst-libs/gst/ion/gstionmemory.c @@ +36,3 @@ +#define gst_ion_allocator_parent_class parent_class + +#define PAGE_ALIGN(x) (((x) + 4095) & ~4095) This should probably use getpagesize () or similar http://man7.org/linux/man-pages/man2/getpagesize.2.html @@ +120,3 @@ + allocation_data.len = ion_size; + allocation_data.align = params->align; + allocation_data.heap_id_mask = ION_HEAP_TYPE_DMA_MASK; This should be overridable for other heaps. @@ +121,3 @@ + allocation_data.align = params->align; + allocation_data.heap_id_mask = ION_HEAP_TYPE_DMA_MASK; + allocation_data.flags = 0; flags possibly also needs to be overridable @@ +137,3 @@ + + ion_mem = g_slice_new0 (GstIONMemory); + gst_memory_init (GST_MEMORY_CAST (ion_mem), GST_MEMORY_FLAG_NO_SHARE, Why do you have NO_SHARE here? any specific reason? @@ +147,3 @@ + + gst_mini_object_set_qdata (GST_MINI_OBJECT (mem), GST_ION_MEMORY_QUARK, + ion_mem, (GDestroyNotify) gst_memory_unref); This won't propagate the ion memory across copies/shares.
Review of attachment 333309 [details] [review]: I really would like to this in gst-plugins-base allocator director. Anyway in -bad that good enough for me now. Get that will be really helpful since it is a good way to allocate contiguous memory. ::: gst-libs/gst/ion/gstionmemory.c @@ +38,3 @@ +#define PAGE_ALIGN(x) (((x) + 4095) & ~4095) + +G_DEFINE_TYPE (GstIONAllocator, gst_ion_allocator, GST_TYPE_ALLOCATOR) Since ION driver use dmabuf I think it should inherit of GST_TYPE_DMABUF_ALLOCATOR so a function like gst_is_dmabuf_memory() will return true. @@ +95,3 @@ + + return quark; +} why do you need quark ? @@ +120,3 @@ + allocation_data.len = ion_size; + allocation_data.align = params->align; + allocation_data.heap_id_mask = ION_HEAP_TYPE_DMA_MASK; ION_HEAP_TYPE_DMA_MASK is fine for me because I want to allocate (by default) contiguous memory. Maybe we should have an allocator for each heap type ? named ionsystem, ionsystem_contig, ioncarveout, iondma. You could extend gst_ion_alloc_alloc with one parameter for the type and only create alloc function for each allocator. @@ +121,3 @@ + allocation_data.align = params->align; + allocation_data.heap_id_mask = ION_HEAP_TYPE_DMA_MASK; + allocation_data.flags = 0; you can pass params->flags here @@ +226,3 @@ + allocator->mem_type = GST_ALLOCATOR_ION; + allocator->mem_map = NULL; + allocator->mem_unmap = NULL; why overwrite mem_map and mem_unmap ? gst_fd_mem_map and gst_fd_mem_unmap default functions are fine here
Benjamin, this implementation is not a FDAllocator subclass, in fact, it does not implement it's own GstMemory (it call another allocator, dmabuf internally). I don't like this, because it's confusing. Please, let's make DMABufAllocator sub-classable first, and then base this new allocator on it.
(In reply to Nicolas Dufresne (stormer) from comment #10) > Benjamin, this implementation is not a FDAllocator subclass, in fact, it > does not implement it's own GstMemory (it call another allocator, dmabuf > internally). > > I don't like this, because it's confusing. Please, let's make > DMABufAllocator sub-classable first, and then base this new allocator on it. Nicolas, you are right but I just realize that DMABufAllocator isn't sub-classable when testing this patch. I'm working to change that and I will open a new bug for it asap.
Created attachment 338358 [details] [review] dmabuf: Make the allocator sub-classable This should allos for cleaner code when implement such allocator.
Review of attachment 338358 [details] [review]: I was writing exactly the same thing on my side. Looks good for me.
(In reply to Benjamin Gaignard from comment #9) > Review of attachment 333309 [details] [review] [review]: > > I really would like to this in gst-plugins-base allocator director. > Anyway in -bad that good enough for me now. > Get that will be really helpful since it is a good way to allocate > contiguous memory. I agree. > > ::: gst-libs/gst/ion/gstionmemory.c > @@ +38,3 @@ > +#define PAGE_ALIGN(x) (((x) + 4095) & ~4095) > + > +G_DEFINE_TYPE (GstIONAllocator, gst_ion_allocator, GST_TYPE_ALLOCATOR) > > Since ION driver use dmabuf I think it should inherit of > GST_TYPE_DMABUF_ALLOCATOR > so a function like gst_is_dmabuf_memory() will return true. That was non obvious, I have attached a patch that make it sub-classable. I wanted to do this in 1.11 anyway. > > @@ +120,3 @@ > + allocation_data.len = ion_size; > + allocation_data.align = params->align; > + allocation_data.heap_id_mask = ION_HEAP_TYPE_DMA_MASK; > > ION_HEAP_TYPE_DMA_MASK is fine for me because I want to allocate (by > default) contiguous memory. > Maybe we should have an allocator for each heap type ? Seems like a big amount of work for just a flag. We could register multiple allocator instance for each type (without creating more GType). For that, we just need that to be configurable (like a construct parameter or property). > named ionsystem, ionsystem_contig, ioncarveout, iondma. > You could extend gst_ion_alloc_alloc with one parameter for the type and > only create alloc function for each allocator. No need to introduce a custom allocation. You can (and should) extend GstMemoryFlags. IMPORTANT note, if you subclass, don't forget to clear GST_ALLOCATOR_FLAG_CUSTOM_ALLOC in your _init() function.
Hi, please push DMABuf patch. Thx
Comment on attachment 338358 [details] [review] dmabuf: Make the allocator sub-classable 1.11 is open, I'm merging this now as it also useful for upcoming VAAPI work. Attachment 338358 [details] pushed as c37b1e8 - dmabuf: Make the allocator sub-classable
> Attachment 338358 [details] pushed as c37b1e8 - dmabuf: Make the allocator > sub-classable Just to avoid future regrets: we don't foresee the need for padding here, do we? (e.g. for vfuncs)
Good point, I will add standard padding, thanks.
After changed dmabuf allocator to sub-classable. Below function need changed too. Below function needn't allocate GstFdMemory, it should use received GstMemory from input parameter. And the user of the function need modify. such as v4l2 element, kmssink. GstMemory * gst_fd_allocator_alloc (GstAllocator * allocator, gint fd, gsize size, GstFdMemoryFlags flags) { #ifdef HAVE_MMAP GstFdMemory *mem; g_return_val_if_fail (GST_IS_FD_ALLOCATOR (allocator), NULL); mem = g_slice_new0 (GstFdMemory); gst_memory_init (GST_MEMORY_CAST (mem), 0, GST_ALLOCATOR_CAST (allocator), NULL, size, 0, 0, size); }
I'm sorry, I don't understand what you would like to change here. If it's about the function signature, it's a no, this is a stable API.
(In reply to Tim-Philipp Müller from comment #17) > Just to avoid future regrets: we don't foresee the need for padding here, do > we? (e.g. for vfuncs) Fixed, by commit a1a2a3331516301132d9094279118dcc441fcd70.
Do need to change GstIonMemory inherit from GstFdMemory or GstDmabufMemory? If no, GstIonMemory still need put into GstFdmemory as one quark data.
I think we need to ask ourself what we want to do. Turning an ION allocation into a DMABuf FD is cheap and make the exchange very generic. So for that case, it should sublclass DMABuf allocator. Note, I didn't add any vfunc, if needed we can fix that. In general, I don't think it's needed. Exposing the ION handles would probably be different, I'm not sure how those handles works really. Also, I don't have any plan yet how to handle mutable memory. So far they are all mutable to DMABuf so it make sense to just expose the dmabuf.
Looking Closer at ion, it seems we can discard the ion handle as soon as we have the dmabuf, and there is a way to get back this handle anyway. So the quark is not strictly required. The quark is not a very elegant solution. If we want to keep the ion information, I think we need to turn GstFDMemory structure into something public, so we can extend it.
Created attachment 340392 [details] [review] Add free function.
Created attachment 340393 [details] [review] DMA Buf allocator based on ion.
Moved the ion allocator to gst-plugin-base. Refined the patch based on review comments. Don't changed ion heap mask and ion flag, as most of use case of ion is for CMA memory allocate.
Created attachment 340400 [details] [review] DMA Buf allocator based on ion.
Created attachment 340401 [details] [review] DMA Buf allocator based on ion.
I will update the patch based on below method: "Seems like a big amount of work for just a flag. We could register multiple allocator instance for each type (without creating more GType). For that, we just need that to be configurable (like a construct parameter or property). No need to introduce a custom allocation. You can (and should) extend GstMemoryFlags."
As the heap id and flag is platform dependent, we should add to properties for those. Below is the link for qcom ion head file. https://android.googlesource.com/platform/hardware/qcom/msm8x26/+/refs/heads/master-soong/original-kernel-headers/linux/msm_ion.h
Make sense, is Qualcomm the only one hacking ION this way ?
Yes, I only found Qualcomm's ION header file.
Created attachment 340728 [details] [review] DMA Buf allocator based on ion.
Created attachment 340730 [details] [review] DMA Buf allocator based on ion.
Review of attachment 340730 [details] [review]: ::: gst-libs/gst/allocators/gstionmemory.c @@ +126,3 @@ + + fd_data.handle = ion_handle; + ret = gst_ion_ioctl (self->fd, ION_IOC_MAP, &fd_data); maybe you can use ION_IOC_SHARE instead of ION_IOC_MAP, it does the same but the name is less confusing @@ +188,3 @@ + /* ION_IOC_IMPORT will add one ref for ion handle. */ + gst_ion_ioctl (self->fd, ION_IOC_FREE, &handle_data); + gst_ion_ioctl (self->fd, ION_IOC_FREE, &handle_data); Why are you doing ION_IOC_IMPORT and 2 ION_IOC_FREE ? I believe that just close the fd is enough
Created attachment 359688 [details] [review] dmabuf memory allocator based on ion driver.
Created attachment 359689 [details] [review] support get phys memory.
Created attachment 359690 [details] [review] dmabuf: set fd memory to keep mapped.
Review of attachment 359688 [details] [review]: This is based on the previous ION ABI. Laura Abbott have done a large clean up and simplify the interface. You need to rework your patch to follow this new ION interface.
Review of attachment 359689 [details] [review]: I think that getting the physical address is a security issue and not welcome for me. Further more it is based on custom (proprietary ?) ioctl.
Review of attachment 359690 [details] [review]: Looks good for me
Review of attachment 359689 [details] [review]: ::: gst-libs/gst/allocators/gstionmemory.c @@ +84,3 @@ + }; + + ret = ioctl(self->fd, ION_IOC_CUSTOM, &custom); CUSTOM like in only specific drivers implements this ? If so, shall be ifdef this with some platform check ? Physical address in userland is a pretty bad idea in general (with all the security related issues that comes with it). For this reason, I don't particularly like the idea of sponsoring this idea in GStreamer final APIs. I think your patch need a long explanation in the description here at least before it could be considered.
Review of attachment 359690 [details] [review]: This patch lacks a proper description. Performance whise it looks like a good idea, but we don't implement DMA_BUF_IOCTL_SYNC, so I suspect we'll endup with coherency issues. I already see bunch of cases where coherency is broken, it's not clear to me who's doing it wrong, but I'd like this to be sorted out before we take the risk of making it worst. (of course, if you use CMA you may not see any issues)
(In reply to Benjamin Gaignard from comment #42) > Review of attachment 359689 [details] [review] [review]: > > I think that getting the physical address is a security issue and not > welcome for me. > Further more it is based on custom (proprietary ?) ioctl. Oops, I'm sorry, I repeated just that.
-- 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-bad/issues/410.