GNOME Bugzilla – Bug 770585
glupload: propose dma buffer pool upstream based on ion allocator
Last modified: 2018-11-03 11:49:09 UTC
1. Propose ion dma-fd buffer pool to upstream 2. If upstream don't choose the proposed buffer pool, then create our own and do buffer copy to avoid memory copy from CPU to GPU side 3. Add buffer alignment
Created attachment 334412 [details] [review] propose dma buffer pool upstream based on ion allocator
ion allocator is in upstream progress in another thread: https://bugzilla.gnome.org/show_bug.cgi?id=769962
Review of attachment 334412 [details] [review]: Why is all this needed? What's wrong with upstream setting all this information? What kind of elements are upstream? ::: gst-libs/gst/gl/gstglupload.c @@ +849,2 @@ /* This will eliminate most non-dmabuf out there */ + if (!gst_is_dmabuf_memory (gst_buffer_peek_memory (buffer, 0))) { What's the difference between this failure case and the raw data uploader? @@ +980,3 @@ +#endif + if (!allocator) { + GST_WARNING ("New ion allocator failed."); This warning will occur on all non-ion platforms. @@ +990,3 @@ + goto setup_failed; + + gst_query_set_nth_allocation_pool (query, 0, pool, info.size, 1, 30); Why were these numbers (1, 30) chosen?
(In reply to Matthew Waters (ystreet00) from comment #3) > Review of attachment 334412 [details] [review] [review]: > > Why is all this needed? What's wrong with upstream setting all this > information? What kind of elements are upstream? Hi Matthew, I'am so sorry that I have paste the wrong link to ion allocator thread which is should be： https://bugzilla.gnome.org/show_bug.cgi?id=768794 > > ::: gst-libs/gst/gl/gstglupload.c > @@ +849,2 @@ > /* This will eliminate most non-dmabuf out there */ > + if (!gst_is_dmabuf_memory (gst_buffer_peek_memory (buffer, 0))) { > > What's the difference between this failure case and the raw data uploader? In the failure case, glupload will create a local dmabuf pool and copy data to dmabuf so that eglimage can import this dmabuf. While raw data will copy date to PBO and use PBO upload texture. This two case is the same in most cases but except some embedded system. For these platforms, PBO has some hardware limitation such as only specical format(RGBA) can use DMA transform texture from PBO to GPU and other format will fallback to CPU copy. This will impact performance. > > @@ +980,3 @@ > +#endif > + if (!allocator) { > + GST_WARNING ("New ion allocator failed."); > > This warning will occur on all non-ion platforms. Will change to use DEBUG > > @@ +990,3 @@ > + goto setup_failed; > + > + gst_query_set_nth_allocation_pool (query, 0, pool, info.size, 1, 30); > > Why were these numbers (1, 30) chosen? We have a use case in which playbin will load libav decoder for video decoder. libav decoder will not respect the downstream video alignment and use his own. This will cause eglCreateImage fail because hardware memory alignment limitation. But libav decoder will reject this buffer pool when max buffer nums less than 32, so we set it to 30 and let libav decoder reject our buffer pool and we do buffer copy in glupload.
(In reply to Haihua Hu from comment #4) > (In reply to Matthew Waters (ystreet00) from comment #3) > > Review of attachment 334412 [details] [review] [review] [review]: > > > > Why is all this needed? What's wrong with upstream setting all this > > information? What kind of elements are upstream? > > Hi Matthew, I'am so sorry that I have paste the wrong link to ion allocator > thread which is should be： > https://bugzilla.gnome.org/show_bug.cgi?id=768794 That does not answer the questions. > > > > ::: gst-libs/gst/gl/gstglupload.c > > @@ +849,2 @@ > > /* This will eliminate most non-dmabuf out there */ > > + if (!gst_is_dmabuf_memory (gst_buffer_peek_memory (buffer, 0))) { > > > > What's the difference between this failure case and the raw data uploader? > > In the failure case, glupload will create a local dmabuf pool and copy data > to dmabuf so that eglimage can import this dmabuf. While raw data will copy > date to PBO and use PBO upload texture. This two case is the same in most > cases but except some embedded system. For these platforms, PBO has some > hardware limitation such as only specical format(RGBA) can use DMA transform > texture from PBO to GPU and other format will fallback to CPU copy. This > will impact performance. dmabuf doesn't create the memory though, ion does. While they are somewhat similar in purpose, they do slightly different things. > > > > @@ +990,3 @@ > > + goto setup_failed; > > + > > + gst_query_set_nth_allocation_pool (query, 0, pool, info.size, 1, 30); > > > > Why were these numbers (1, 30) chosen? > > We have a use case in which playbin will load libav decoder for video > decoder. libav decoder will not respect the downstream video alignment and > use his own. This will cause eglCreateImage fail because hardware memory > alignment limitation. But libav decoder will reject this buffer pool when > max buffer nums less than 32, so we set it to 30 and let libav decoder > reject our buffer pool and we do buffer copy in glupload. Erm, that's not going to fly. What should happen is that the bufferpool should fail setting the config with the unaligned size of which the easiest thing is to propose a separate pool with the ion allocator. At which point it makes more sense to write a new upload method for ion than shoehorn this into the dmabuf allocator. Another question, what hardware/software are you testing on? Will this work on any hardware that supports ion?
(In reply to Matthew Waters (ystreet00) from comment #5) > > Erm, that's not going to fly. What should happen is that the bufferpool > should fail setting the config with the unaligned size of which the easiest > thing is to propose a separate pool with the ion allocator. At which point > it makes more sense to write a new upload method for ion than shoehorn this > into the dmabuf allocator. > > Another question, what hardware/software are you testing on? Will this work > on any hardware that supports ion? You mean we need to implement a new buffer pool which is specific for ion buffer and override _set_config() vmethod to make it fail to set the unaligned size? As a result, we need to write a new uploader for ion dma-buf. We are testing this plugin on imx soc for linux, this patch will be useful on the platform which has ion support.
(In reply to Haihua Hu from comment #6) > (In reply to Matthew Waters (ystreet00) from comment #5) > > > > Erm, that's not going to fly. What should happen is that the bufferpool > > should fail setting the config with the unaligned size of which the easiest > > thing is to propose a separate pool with the ion allocator. At which point > > it makes more sense to write a new upload method for ion than shoehorn this > > into the dmabuf allocator. > > You mean we need to implement a new buffer pool which is specific for ion > buffer and override _set_config() vmethod to make it fail to set the > unaligned size? As a result, we need to write a new uploader for ion dma-buf. Yes to all that. > > Another question, what hardware/software are you testing on? Will this work > > on any hardware that supports ion? > > We are testing this plugin on imx soc for linux, this patch will be useful > on the platform which has ion support. Ok.
Is this still being worked on?
-- 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/287.