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 787593 - kmssink: Export DMABuf
kmssink: Export DMABuf
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.13.x
Other Linux
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-09-12 19:32 UTC by Nicolas Dufresne (ndufresne)
Modified: 2017-10-06 19:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
kmssink: Check if we can prime export (1.84 KB, patch)
2017-09-12 19:33 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kmsbufferpool: Removed unused member fd (919 bytes, patch)
2017-09-12 19:33 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kmsbufferpool: Don't check allocator pointer twice (952 bytes, patch)
2017-09-12 19:33 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kmssink: Move the mem cache into the allocator (7.40 KB, patch)
2017-09-12 19:33 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
kms: Export DMABuf from Dumb buffer when possible (6.94 KB, patch)
2017-09-12 19:33 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
kmssink: Move the mem cache into the allocator (7.49 KB, patch)
2017-10-05 20:17 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kms: Export DMABuf from Dumb buffer when possible (6.80 KB, patch)
2017-10-05 20:17 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kmsallocator: Update GstVideoInfo.size when extrapolating (1.10 KB, patch)
2017-10-05 20:18 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
fix compilation (1.25 KB, patch)
2017-10-06 00:38 UTC, U. Artie Eoff
committed Details | Review
kmssink: Fix crash on NULL dmabuf allocator pointer (1.02 KB, patch)
2017-10-06 19:10 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2017-09-12 19:32:07 UTC
The following patchset makes kmssink export DMABuf from the Dumb buffers. This is not the ideal way for zero-copy, but it allow to get downstream DMABuf allocation working in certain cases. Tested with v4l2src, uvc driver, packed YUY2 (to avoid stride issues).

  v4l2src io-mode=dmabuf-import ! kmssink

There is cache coherency issues that causes visual artefact, but that's also visible the other way around (io-mode=dmabuf).
Comment 1 Nicolas Dufresne (ndufresne) 2017-09-12 19:33:10 UTC
Created attachment 359657 [details] [review]
kmssink: Check if we can prime export

This will be used later to decide if a DMABuf allocator should be
exposed.
Comment 2 Nicolas Dufresne (ndufresne) 2017-09-12 19:33:14 UTC
Created attachment 359658 [details] [review]
kmsbufferpool: Removed unused member fd
Comment 3 Nicolas Dufresne (ndufresne) 2017-09-12 19:33:18 UTC
Created attachment 359659 [details] [review]
kmsbufferpool: Don't check allocator pointer twice
Comment 4 Nicolas Dufresne (ndufresne) 2017-09-12 19:33:22 UTC
Created attachment 359660 [details] [review]
kmssink: Move the mem cache into the allocator

No functional change, the cache will be later reused by the buffer
pool to keep track of the kmssink memory when exporting dmabuf.
Comment 5 Nicolas Dufresne (ndufresne) 2017-09-12 19:33:27 UTC
Created attachment 359661 [details] [review]
kms: Export DMABuf from Dumb buffer when possible
Comment 6 Nicolas Dufresne (ndufresne) 2017-09-21 20:37:51 UTC
I've detect a regression with these patches. The following pipeline crash on intel drm, but didn't before:

  gst-launch-1.0 videotestsrc ! video/x-raw,width=100,height=100 ! kmssink

We need to figure this out before this can be merged.

==15699== Thread 2 videotestsrc0:sr:
==15699== Invalid write of size 8
==15699==    at 0x4063440: ??? (in /run/user/1000/orcexec.dLFbE2 (deleted))
==15699==    by 0xF5068DA: video_orc_pack_YUY2 (video-orc.c:1822)
==15699==    by 0xF544812: pack_YUY2 (video-format.c:209)
==15699==    by 0xF2E2A9D: convert_hline_generic (videotestsrc.c:1289)
==15699==    by 0xF2E276D: videotestsrc_convert_tmpline (videotestsrc.c:273)
==15699==    by 0xF2E3338: gst_video_test_src_smpte (videotestsrc.c:422)
==15699==    by 0xF2E0453: gst_video_test_src_fill (gstvideotestsrc.c:1152)
==15699==    by 0xF7AA7AC: gst_base_src_default_create (gstbasesrc.c:1515)
==15699==    by 0xF7ACBBF: gst_base_src_get_range (gstbasesrc.c:2504)
==15699==    by 0xF7AEC82: gst_base_src_loop (gstbasesrc.c:2804)
==15699==    by 0x4EEA8A8: gst_task_func (gsttask.c:332)
==15699==    by 0x51E2EFF: g_thread_pool_thread_proxy (gthreadpool.c:307)
==15699==  Address 0x4054000 is not stack'd, malloc'd or (recently) free'd
Comment 7 Guillaume Desmottes 2017-09-27 14:10:38 UTC
Review of attachment 359660 [details] [review]:

There is actually a small functional change. The cache is now protected by the allocator's lock while it was by the sink's lock before.

::: sys/kms/gstkmsallocator.c
@@ +57,3 @@
 {
   int fd;
+  GList *mem_cache;

Add a comment saying it's protected by the object lock?
Comment 8 Víctor Manuel Jáquez Leal 2017-09-27 14:18:02 UTC
Review of attachment 359661 [details] [review]:

::: sys/kms/gstkmsallocator.c
@@ +403,3 @@
   allocator->priv = gst_kms_allocator_get_instance_private (allocator);
   allocator->priv->fd = -1;
+  allocator->priv->dmabuf_alloc = gst_dmabuf_allocator_new ();

I would instantiate this internal allocator lazily, when gst_kms_allocator_dmabuf_export() is called by first time, since there's the only function where is used.
Comment 9 Guillaume Desmottes 2017-09-27 14:29:30 UTC
Review of attachment 359660 [details] [review]:

::: sys/kms/gstkmsallocator.c
@@ -553,0 +556,42 @@
+
+/* FIXME, using gdata for caching on upstream memory is not tee safe */
+GstMemory *
... 39 more ...

I'd add a comment saying that @kmsmem is (transfer full) as it's not that easy to guess from a quick read of the code.
Comment 10 Guillaume Desmottes 2017-09-27 14:29:40 UTC
Review of attachment 359661 [details] [review]:

::: sys/kms/gstkmsallocator.h
@@ +87,3 @@
 					       GstVideoInfo *vinfo);
 
+GstMemory*    gst_kms_allocator_dmabuf_export (GstAllocator *allocator,

"GstMemory *"

::: sys/kms/gstkmsbufferpool.h
@@ +46,3 @@
+ *
+ * An option that can be activated on buffer pool to request DMABuf
+ * buffers. Callers us responsible to check if this is supported. Dumb buffers

"Callers ars"
Comment 11 Nicolas Dufresne (ndufresne) 2017-10-05 20:17:14 UTC
Created attachment 360991 [details] [review]
kmssink: Move the mem cache into the allocator

Updated according to review.
Comment 12 Nicolas Dufresne (ndufresne) 2017-10-05 20:17:56 UTC
Created attachment 360992 [details] [review]
kms: Export DMABuf from Dumb buffer when possible

updated according to review.
Comment 13 Nicolas Dufresne (ndufresne) 2017-10-05 20:18:34 UTC
Created attachment 360993 [details] [review]
kmsallocator: Update GstVideoInfo.size when extrapolating

This one fixes the regression, in fact it was just revealing a bug we already had, oops.
Comment 14 Nicolas Dufresne (ndufresne) 2017-10-05 20:20:48 UTC
Attachment 359657 [details] pushed as 045a919 - kmssink: Check if we can prime export
Attachment 359658 [details] pushed as 0a25ca8 - kmsbufferpool: Removed unused member fd
Attachment 359659 [details] pushed as 119294f - kmsbufferpool: Don't check allocator pointer twice
Attachment 360991 [details] pushed as 9d5a524 - kmssink: Move the mem cache into the allocator
Attachment 360992 [details] pushed as 922031b - kms: Export DMABuf from Dumb buffer when possible
Attachment 360993 [details] pushed as 2057a36 - kmsallocator: Update GstVideoInfo.size when extrapolating
Comment 15 U. Artie Eoff 2017-10-05 22:02:20 UTC
FYI, 922031b breaks compilation on Ubuntu Trusty...

gstkmsallocator.c: In function 'gst_kms_allocator_dmabuf_export':
gstkmsallocator.c:582:21: error: 'DRM_RDWR' undeclared (first use in this function)
DRM_CLOEXEC | DRM_RDWR, &prime_fd);
^
gstkmsallocator.c:582:21: note: each undeclared identifier is reported only once for each function it appears in
Comment 16 Nicolas Dufresne (ndufresne) 2017-10-06 00:03:48 UTC
Trusty is 14.04 ? I'll track down de required libdrm version and update. I don't think targeting such an old distro make any sense.
Comment 17 U. Artie Eoff 2017-10-06 00:09:08 UTC
(In reply to Nicolas Dufresne (stormer) from comment #16)
> Trusty is 14.04 ? I'll track down de required libdrm version and update. I
> don't think targeting such an old distro make any sense.

Yes, it's 14.04.x and is LTS until April 2019... so seems it would make sense to target.  If not, then minimum libdrm version needs to be up'd in configure checks.

Seems it would be easier to add:

#ifndef DRM_RDWR 
#define DRM_RDWR O_RDWR 
#endif
Comment 18 Nicolas Dufresne (ndufresne) 2017-10-06 00:11:23 UTC
I'm not interested myself in supporting this, but if you propose a patch, I will merge it.
Comment 19 U. Artie Eoff 2017-10-06 00:33:40 UTC
It's more about supporting minimum required libdrm than trusty... DRM_RDWR was not defined until 2.4.68, yet configure.ac only requires at least 2.4.55.

I'll attach a patch since it's easy enough.
Comment 20 U. Artie Eoff 2017-10-06 00:38:10 UTC
Created attachment 361008 [details] [review]
fix compilation
Comment 21 Nicolas Dufresne (ndufresne) 2017-10-06 00:52:01 UTC
Comment on attachment 361008 [details] [review]
fix compilation

Thanks
Comment 22 Nicolas Dufresne (ndufresne) 2017-10-06 00:52:47 UTC
Commited as ddba9e40f24548d8ec0e98f9684e1a1227ac5ee8
Comment 23 Nicolas Dufresne (ndufresne) 2017-10-06 19:10:12 UTC
Created attachment 361061 [details] [review]
kmssink: Fix crash on NULL dmabuf allocator pointer

Now that we are doing lazy allocation, we may endup calling _stop()
before the allocator was created. As a side effect, we need to nul-check
the pointer before calling it's method (_clear_cache()).
Comment 24 Nicolas Dufresne (ndufresne) 2017-10-06 19:10:41 UTC
Comment on attachment 361061 [details] [review]
kmssink: Fix crash on NULL dmabuf allocator pointer

Attachment 361061 [details] pushed as f272ddf - kmssink: Fix crash on NULL dmabuf allocator pointer