<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 23, 2024 at 5:03 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</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">Quoting Cheng-Hao Yang (2024-09-23 08:52:32)<br>
> Hi Robert and developers,<br>
> <br>
> Sorry to be late to the party, as I saw the v5 patch is already accepted<br>
> [1].<br>
> <br>
> However, I'm wondering if it makes sense to add the DMABUF<br>
> synchronization as a generic function in the DmaBufAllocator,<br>
> like CrOS had done [2]. CrOS mtkisp7 needs the sync as well,<br>
> and maybe we can save some duplicated code this way.<br>
> <br>
> Whether the input is a single fd or a FrameBuffer (that we sync<br>
> all fds in all planes) can be discussed.<br>
> <br>
> I'd be happy to add the new patch, and update the usages in SoftwareISP<br>
> if this is the way to go.<br>
<br>
Working on a clean generic helper to save duplications is worthwhile. We<br>
did discuss that in the patches, but went for the simple/local merged<br>
version as it only currently affected the soft-isp, and we can't put it<br>
in a code-path that is used by all other pipelines all the time.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I don't know where it should go yet though. Will the allocator be the<br>
right place for it ? What happens if the buffers come from an external<br>
source ? (Or maybe in that case, the provider is responsible?)<br></blockquote><div><br></div><div>Yeah that's a question that we need to figure out indeed.</div><div><br></div><div>Maybe an option is to keep the information that the fd is allocated by</div><div>DmaBufAllocator in UniqueFD/SharedFD, and the sync helper</div><div>function/class would abort if the input is not from the allocator?</div><div>We may also be able to make it a function in SharedFD in this way.</div><div><br></div><div>I'm also fine with the idea that the provider is responsible though.</div><div><br></div><div>BTW, CrOS mtkisp7 also has a class DmaSyncer [1], which manages</div><div>the sync with the instance's lifetime. I think it's worth a thought as well.</div><div><br></div><div>[1]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674883">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674883</a></div><div><br></div><div>BR,<br>Harvey</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> Thanks!<br>
> Harvey<br>
> <br>
> [1]: <a href="https://patchwork.libcamera.org/patch/21290/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/21290/</a><br>
> [2]:<br>
> <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79" rel="noreferrer" target="_blank">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/4714763/21/src/libcamera/dma_heaps.cpp#79</a><br>
> <br>
> On Sat, Sep 21, 2024 at 12:52 AM Nicolas Dufresne <<a href="mailto:nicolas@ndufresne.ca" target="_blank">nicolas@ndufresne.ca</a>><br>
> wrote:<br>
> <br>
> > 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<br>
> > 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<br>
> > would be<br>
> > nice. I'd see something similar to the mutex locker, something you can't<br>
> > 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<br>
> > 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,<br>
> > 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<br>
> > flags "<br>
> > > + << syncFlags << " failed: " <<<br>
> > 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,<br>
> > DebayerParams params)<br>
> > > {<br>
> > > timespec frameStartTime;<br>
> > > @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input,<br>
> > 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,<br>
> > 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>
> ><br>
> <br>
> -- <br>
> BR,<br>
> Harvey Yang<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></div>