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 768794 - ion: DMA Buf allocator based on ion
ion: DMA Buf allocator based on ion
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 770585
 
 
Reported: 2016-07-14 06:18 UTC by kevin
Modified: 2018-11-03 13:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
DMA Buf allocator based on ion. (13.08 KB, patch)
2016-07-14 06:22 UTC, kevin
none Details | Review
Refine the patch. (12.02 KB, patch)
2016-08-15 03:12 UTC, kevin
none Details | Review
dmabuf: Make the allocator sub-classable (3.97 KB, patch)
2016-10-24 15:13 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
Add free function. (2.94 KB, patch)
2016-11-21 06:16 UTC, kevin
none Details | Review
DMA Buf allocator based on ion. (10.15 KB, patch)
2016-11-21 06:19 UTC, kevin
none Details | Review
DMA Buf allocator based on ion. (10.16 KB, patch)
2016-11-21 09:05 UTC, kevin
none Details | Review
DMA Buf allocator based on ion. (10.27 KB, patch)
2016-11-21 09:08 UTC, kevin
none Details | Review
DMA Buf allocator based on ion. (12.03 KB, patch)
2016-11-25 07:03 UTC, kevin
none Details | Review
DMA Buf allocator based on ion. (12.15 KB, patch)
2016-11-25 07:50 UTC, kevin
none Details | Review
dmabuf memory allocator based on ion driver. (11.16 KB, patch)
2017-09-13 06:22 UTC, kevin
needs-work Details | Review
support get phys memory. (2.36 KB, patch)
2017-09-13 06:23 UTC, kevin
needs-work Details | Review
dmabuf: set fd memory to keep mapped. (961 bytes, patch)
2017-09-13 06:24 UTC, kevin
needs-work Details | Review

Description kevin 2016-07-14 06:18:01 UTC
DMA Buf allocator based on ion
Comment 1 kevin 2016-07-14 06:22:30 UTC
Created attachment 331464 [details] [review]
DMA Buf allocator based on ion.
Comment 2 Nicolas Dufresne (ndufresne) 2016-07-14 10:55:54 UTC
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 ?
Comment 3 kevin 2016-07-15 03:40:19 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2016-07-15 16:21:34 UTC
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.
Comment 5 kevin 2016-07-16 13:52:53 UTC
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.
Comment 6 kevin 2016-08-15 03:12:19 UTC
Created attachment 333309 [details] [review]
Refine the patch.
Comment 7 kevin 2016-08-15 07:39:54 UTC
Will submit glupload/gldownload based on ion allocator later in other thread.
Comment 8 Matthew Waters (ystreet00) 2016-08-27 04:48:41 UTC
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.
Comment 9 Benjamin Gaignard 2016-10-23 12:25:37 UTC
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
Comment 10 Nicolas Dufresne (ndufresne) 2016-10-24 14:27:49 UTC
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.
Comment 11 Benjamin Gaignard 2016-10-24 14:47:49 UTC
(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.
Comment 12 Nicolas Dufresne (ndufresne) 2016-10-24 15:13:34 UTC
Created attachment 338358 [details] [review]
dmabuf: Make the allocator sub-classable

This should allos for cleaner code when implement such allocator.
Comment 13 Benjamin Gaignard 2016-10-24 15:20:37 UTC
Review of attachment 338358 [details] [review]:

I was writing exactly the same thing on my side.
Looks good for me.
Comment 14 Nicolas Dufresne (ndufresne) 2016-10-24 15:38:12 UTC
(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.
Comment 15 Julien Isorce 2016-11-03 07:47:15 UTC
Hi, please push DMABuf patch. Thx
Comment 16 Nicolas Dufresne (ndufresne) 2016-11-03 19:31:11 UTC
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
Comment 17 Tim-Philipp Müller 2016-11-03 22:01:51 UTC
> 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)
Comment 18 Nicolas Dufresne (ndufresne) 2016-11-04 00:41:33 UTC
Good point, I will add standard padding, thanks.
Comment 19 kevin 2016-11-18 10:06:47 UTC
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);
}
Comment 20 Nicolas Dufresne (ndufresne) 2016-11-18 18:55:11 UTC
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.
Comment 21 Nicolas Dufresne (ndufresne) 2016-11-18 19:58:36 UTC
(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.
Comment 22 kevin 2016-11-19 00:16:02 UTC
Do need to change GstIonMemory inherit from GstFdMemory or GstDmabufMemory? If no, GstIonMemory still need put into GstFdmemory as one quark data.
Comment 23 Nicolas Dufresne (ndufresne) 2016-11-19 00:58:45 UTC
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.
Comment 24 Nicolas Dufresne (ndufresne) 2016-11-19 15:15:52 UTC
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.
Comment 25 kevin 2016-11-21 06:16:02 UTC
Created attachment 340392 [details] [review]
Add free function.
Comment 26 kevin 2016-11-21 06:19:16 UTC
Created attachment 340393 [details] [review]
DMA Buf allocator based on ion.
Comment 27 kevin 2016-11-21 06:22:32 UTC
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.
Comment 28 kevin 2016-11-21 09:05:39 UTC
Created attachment 340400 [details] [review]
DMA Buf allocator based on ion.
Comment 29 kevin 2016-11-21 09:08:11 UTC
Created attachment 340401 [details] [review]
DMA Buf allocator based on ion.
Comment 30 kevin 2016-11-24 03:33:25 UTC
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."
Comment 31 kevin 2016-11-24 07:23:17 UTC
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
Comment 32 Nicolas Dufresne (ndufresne) 2016-11-24 14:41:30 UTC
Make sense, is Qualcomm the only one hacking ION this way ?
Comment 33 kevin 2016-11-25 07:02:49 UTC
Yes, I only found Qualcomm's ION header file.
Comment 34 kevin 2016-11-25 07:03:48 UTC
Created attachment 340728 [details] [review]
DMA Buf allocator based on ion.
Comment 35 kevin 2016-11-25 07:50:49 UTC
Created attachment 340730 [details] [review]
DMA Buf allocator based on ion.
Comment 36 Benjamin Gaignard 2016-11-25 13:02:18 UTC
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
Comment 37 kevin 2017-09-13 06:22:38 UTC
Created attachment 359688 [details] [review]
dmabuf memory allocator based on ion driver.
Comment 38 kevin 2017-09-13 06:23:25 UTC
Created attachment 359689 [details] [review]
support get phys memory.
Comment 39 kevin 2017-09-13 06:24:10 UTC
Created attachment 359690 [details] [review]
dmabuf: set fd memory to keep mapped.
Comment 40 Benjamin Gaignard 2017-09-13 07:41:19 UTC
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.
Comment 41 Benjamin Gaignard 2017-09-13 07:41:25 UTC
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.
Comment 42 Benjamin Gaignard 2017-09-13 07:43:50 UTC
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.
Comment 43 Benjamin Gaignard 2017-09-13 07:45:28 UTC
Review of attachment 359690 [details] [review]:

Looks good for me
Comment 44 Nicolas Dufresne (ndufresne) 2017-09-13 13:52:26 UTC
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.
Comment 45 Nicolas Dufresne (ndufresne) 2017-09-13 13:58:49 UTC
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)
Comment 46 Nicolas Dufresne (ndufresne) 2017-09-13 14:10:25 UTC
(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.
Comment 47 GStreamer system administrator 2018-11-03 13:53:37 UTC
-- 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.