GNOME Bugzilla – Bug 787593
kmssink: Export DMABuf
Last modified: 2017-10-06 19:10:41 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).
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.
Created attachment 359658 [details] [review] kmsbufferpool: Removed unused member fd
Created attachment 359659 [details] [review] kmsbufferpool: Don't check allocator pointer twice
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.
Created attachment 359661 [details] [review] kms: Export DMABuf from Dumb buffer when possible
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
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?
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.
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.
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"
Created attachment 360991 [details] [review] kmssink: Move the mem cache into the allocator Updated according to review.
Created attachment 360992 [details] [review] kms: Export DMABuf from Dumb buffer when possible updated according to review.
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.
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
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
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.
(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
I'm not interested myself in supporting this, but if you propose a patch, I will merge it.
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.
Created attachment 361008 [details] [review] fix compilation
Comment on attachment 361008 [details] [review] fix compilation Thanks
Commited as ddba9e40f24548d8ec0e98f9684e1a1227ac5ee8
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 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