GNOME Bugzilla – Bug 794216
allocators: Add DMABuf synchronization
Last modified: 2018-03-21 22:16:41 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.
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.
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.
Ah, I need to port to mem_(un)map_full, so I'm not the first one to have had this issue.
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.
*** Bug 787594 has been marked as a duplicate of this bug. ***
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.
(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.
Attachment 369532 [details] pushed as 8ee306e - allocators: Add DMABuf synchronization
This is a candidate for 1.14.
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