<div dir="ltr">Hi Robert and developers,<div><br></div><div>Sorry to be late to the party, as I saw the v5 patch is already accepted [1].</div><div><br></div><div>However, I'm wondering if it makes sense to add the DMABUF</div><div>synchronization as a generic function in the DmaBufAllocator,</div><div>like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,</div><div>and maybe we can save some duplicated code this way.</div><div><br></div><div>Whether the input is a single fd or a FrameBuffer (that we sync</div><div>all fds in all planes) can be discussed.</div><div><br></div><div>I'd be happy to add the new patch, and update the usages in SoftwareISP</div><div>if this is the way to go.</div><div><br></div><div>Thanks!</div><div>Harvey</div><div><br></div><div>[1]: <a href="https://patchwork.libcamera.org/patch/21290/">https://patchwork.libcamera.org/patch/21290/</a></div><div>[2]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <<a href="mailto:nicolas@ndufresne.ca" target="_blank">nicolas@ndufresne.ca</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
Le vendredi 20 septembre 2024 à 12:49 +0100, Kieran Bingham a écrit :<br>
> From: Robert Mader <<a href="mailto:robert.mader@collabora.com" target="_blank">robert.mader@collabora.com</a>><br>
> <br>
> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure<br>
> correct output. Not doing so currently results in occasional tearing<br>
> and/or backlashes in GL/VK clients that use the buffers directly for<br>
> rendering.<br>
> <br>
> An alternative approach to have the sync code in `MappedFrameBuffer` was<br>
> considered but rejected for now, in order to allow clients more<br>
> flexibility.<br>
> <br>
> While the new helper is added to an annoymous namespace, add<br>
> timeDiff to the same namespace and remove the static definition as a<br>
> drive by.<br>
> <br>
> Signed-off-by: Robert Mader <<a href="mailto:robert.mader@collabora.com" target="_blank">robert.mader@collabora.com</a>><br>
> Tested-by: Milan Zamazal <<a href="mailto:mzamazal@redhat.com" target="_blank">mzamazal@redhat.com</a>> # Debix<br>
> Tested-by: Hans de Goede <<a href="mailto:hdegoede@redhat.com" target="_blank">hdegoede@redhat.com</a>> # IPU6 + ov2740<br>
> Tested-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> # Lenovo X13s + OV5675<br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
If we generalize this in the future, a more C++ friendly implementation would be<br>
nice. I'd see something similar to the mutex locker, something you can't forget<br>
to close.<br>
<br>
Reviewed-by: Nicolas Dufresne <<a href="mailto:nicolas.dufresne@collabora.com" target="_blank">nicolas.dufresne@collabora.com</a>><br>
<br>
> Signed-off-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> ---<br>
> v5: Kieran:<br>
> - Remove static<br>
> - Fix ret negation that I suggested incorrectly.<br>
> <br>
> src/libcamera/software_isp/debayer_cpu.cpp | 32 +++++++++++++++++++++-<br>
> 1 file changed, 31 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp<br>
> index 077f7f4bc45b..8a757fe9e02d 100644<br>
> --- a/src/libcamera/software_isp/debayer_cpu.cpp<br>
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp<br>
> @@ -12,8 +12,11 @@<br>
> #include "debayer_cpu.h"<br>
> <br>
> #include <stdlib.h><br>
> +#include <sys/ioctl.h><br>
> #include <time.h><br>
> <br>
> +#include <linux/dma-buf.h><br>
> +<br>
> #include <libcamera/formats.h><br>
> <br>
> #include "libcamera/internal/bayer_format.h"<br>
> @@ -718,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)<br>
> }<br>
> }<br>
> <br>
> -static inline int64_t timeDiff(timespec &after, timespec &before)<br>
> +namespace {<br>
> +<br>
> +void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)<br>
> +{<br>
> + for (const FrameBuffer::Plane &plane : buffer->planes()) {<br>
> + const int fd = plane.fd.get();<br>
> + struct dma_buf_sync sync = { syncFlags };<br>
> + int ret;<br>
> +<br>
> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);<br>
> + if (ret < 0) {<br>
> + ret = errno;<br>
> + LOG(Debayer, Error)<br>
> + << "Syncing buffer FD " << fd << " with flags "<br>
> + << syncFlags << " failed: " << strerror(ret);<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> +inline int64_t timeDiff(timespec &after, timespec &before)<br>
> {<br>
> return (after.tv_sec - before.tv_sec) * 1000000000LL +<br>
> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;<br>
> }<br>
> <br>
> +} /* namespace */<br>
> +<br>
> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)<br>
> {<br>
> timespec frameStartTime;<br>
> @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams<br>
> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);<br>
> }<br>
> <br>
> + syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);<br>
> + syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);<br>
> +<br>
> green_ = params.green;<br>
> red_ = swapRedBlueGains_ ? params.blue : params.red;<br>
> blue_ = swapRedBlueGains_ ? params.red : params.blue;<br>
> @@ -760,6 +787,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams<br>
> <br>
> metadata.planes()[0].bytesused = out.planes()[0].size();<br>
> <br>
> + syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);<br>
> + syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);<br>
> +<br>
> /* Measure before emitting signals */<br>
> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&<br>
> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {<br>
<br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>BR,</div>Harvey Yang</div></div>