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 794216 - allocators: Add DMABuf synchronization
allocators: Add DMABuf synchronization
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 787594 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2018-03-10 02:29 UTC by Nicolas Dufresne (ndufresne)
Modified: 2018-03-21 22:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
allocators: Add DMABuf synchronization (4.97 KB, patch)
2018-03-10 02:29 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
allocators: Add DMABuf synchronization (3.83 KB, patch)
2018-03-10 04:42 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2018-03-10 02:29:49 UTC
See commit message, this hint the drivers whenever CPU access is done.
This fixes notably visible artifact seen with the following pipeline
using an UVC camera:

  v4l2src ! videoconvert ! video/x-raw,format=RGBA ! kmssink

Though it does not fix the trully zero-copy path with UVC because of a driver bug.
Comment 1 Nicolas Dufresne (ndufresne) 2018-03-10 02:29:53 UTC
Created attachment 369530 [details] [review]
allocators: Add DMABuf synchronization

When doing CPU Access, some architecture may require caches to be
synchronize before use. Otherwise, some visual artifact may be
visible, as the CPU modification may still resides in cache.
Comment 2 Nicolas Dufresne (ndufresne) 2018-03-10 04:29:38 UTC
Review of attachment 369530 [details] [review]:

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +84,3 @@
+  GstDmaBufAllocator *allocator = (GstDmaBufAllocator *) gmem->allocator;
+#ifdef HAVE_LINUX_DMA_BUF_H
+  struct dma_buf_sync sync = { DMA_BUF_SYNC_END };

There is bug here, the kernel requires to specify if this is read or write, which is stored in the GstMapInfo, but it's not exposed here.
Comment 3 Nicolas Dufresne (ndufresne) 2018-03-10 04:34:15 UTC
Ah, I need to port to mem_(un)map_full, so I'm not the first one to have had this issue.
Comment 4 Nicolas Dufresne (ndufresne) 2018-03-10 04:42:51 UTC
Created attachment 369532 [details] [review]
allocators: Add DMABuf synchronization

When doing CPU Access, some architecture may require caches to be
synchronize before use. Otherwise, some visual artifact may be
visible, as the CPU modification may still resides in cache.
Comment 5 Nicolas Dufresne (ndufresne) 2018-03-13 17:43:15 UTC
*** Bug 787594 has been marked as a duplicate of this bug. ***
Comment 6 Guillaume Desmottes 2018-03-14 09:19:39 UTC
Review of attachment 369532 [details] [review]:

::: gst-libs/gst/allocators/gstdmabuf.c
@@ +35,3 @@
  */
 
+#ifdef HAVE_LINUX_DMA_BUF_H

Assuming you tested using meson, did you check that's the exact name defined by AC_CHECK_HEADERS()?

@@ +87,3 @@
+
+  if (ioctl (gst_fd_memory_get_fd (gmem), DMA_BUF_IOCTL_SYNC, &sync) < 0)
+      GST_WARNING_OBJECT (allocator, "Failed to synchronize DMABuf: %s (%i)",

Is it mandatory for drivers to implement this ioctl? Just to be sure we are not going to introduce loads of WARNING to existing working code.
Comment 7 Nicolas Dufresne (ndufresne) 2018-03-14 12:56:36 UTC
(In reply to Guillaume Desmottes from comment #6)
> Review of attachment 369532 [details] [review] [review]:
> 
> ::: gst-libs/gst/allocators/gstdmabuf.c
> @@ +35,3 @@
>   */
>  
> +#ifdef HAVE_LINUX_DMA_BUF_H
> 
> Assuming you tested using meson, did you check that's the exact name defined
> by AC_CHECK_HEADERS()?

Yes, AC will capitalize and replace non letters to _, I've mimic this in Meson to make it match.

> 
> @@ +87,3 @@
> +
> +  if (ioctl (gst_fd_memory_get_fd (gmem), DMA_BUF_IOCTL_SYNC, &sync) < 0)
> +      GST_WARNING_OBJECT (allocator, "Failed to synchronize DMABuf: %s
> (%i)",
> 
> Is it mandatory for drivers to implement this ioctl? Just to be sure we are
> not going to introduce loads of WARNING to existing working code.

The ioctl is implemented in the code. It is a no-op that succeed. Its not clear why it would fail, but Tim was wondering if we could make the warning trigger only once. The memory will be valid if it fails, but it's coherency will be undefined for dma transfer.
Comment 8 Nicolas Dufresne (ndufresne) 2018-03-21 21:29:01 UTC
Attachment 369532 [details] pushed as 8ee306e - allocators: Add DMABuf synchronization
Comment 9 Nicolas Dufresne (ndufresne) 2018-03-21 21:32:48 UTC
This is a candidate for 1.14.
Comment 10 Nicolas Dufresne (ndufresne) 2018-03-21 22:16:41 UTC
Oops,

Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Mar 21 18:15:49 2018 -0400

    dmabufallocator: Fix build if LINUX_DMA_BUF_H is missing