| Message ID | 20251120094919.12138-1-robert.mader@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: > Use the DmaSyncer handler to ensure the data from the buffer is > coherent. This is required by the spec - from > https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: > >> When a DMA buffer is accessed from the CPU via mmap, it is not always >> possible to guarantee coherency between the CPU-visible map and >> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used >> to bracket any CPU access to give the kernel the chance to shuffle memory >> around if needed. > Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? Regards, Barnabás Pőcze > This was reported to fix glitches with the upcoming GPU-ISP, in which > case the accessed data in written by the GPU. > > Note that in the GPU-ISP case we are effectively seeing a round-trip of > the buffer contents - from GPU synced to CPU, copied to GPU again. We > could avoid that in the future by implementing direct dmabuf import in > qcam. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- > src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp > index f31956ff0..0121c97ad 100644 > --- a/src/apps/qcam/viewfinder_gl.cpp > +++ b/src/apps/qcam/viewfinder_gl.cpp > @@ -9,14 +9,16 @@ > > #include <array> > > +#include <libcamera/formats.h> > + > +#include "libcamera/internal/dma_buf_allocator.h" > + > #include <QByteArray> > #include <QFile> > #include <QImage> > #include <QMatrix4x4> > #include <QStringList> > > -#include <libcamera/formats.h> > - > #include "../common/image.h" > > static const QList<libcamera::PixelFormat> supportedFormats{ > @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() > /* Stride of the first plane, in pixels. */ > unsigned int stridePixels; > > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + > switch (format_) { > case libcamera::formats::NV12: > case libcamera::formats::NV21: > diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp > index 1a238922b..4d00f154d 100644 > --- a/src/apps/qcam/viewfinder_qt.cpp > +++ b/src/apps/qcam/viewfinder_qt.cpp > @@ -11,6 +11,7 @@ > #include <stdint.h> > #include <utility> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include <libcamera/formats.h> > > #include <QImage> > @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) > * Otherwise, convert the format and release the frame > * buffer immediately. > */ > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + > converter_.convert(image, size, &image_); > } > } > @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) > > /* If we have an image, draw it, with black letterbox rectangles. */ > if (!image_.isNull()) { > + if (buffer_) { > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + } > + > if (place_.width() < width()) { > QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; > painter.drawRect(rect);
On 20.11.25 10:58, Barnabás Pőcze wrote: > Hi > > 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >> Use the DmaSyncer handler to ensure the data from the buffer is >> coherent. This is required by the spec - from >> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >> >>> When a DMA buffer is accessed from the CPU via mmap, it is not always >>> possible to guarantee coherency between the CPU-visible map and >>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used >>> to bracket any CPU access to give the kernel the chance to shuffle >>> memory >>> around if needed. >> > > Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? We discussed that in the thread for "[PATCH] libcamera: debayer_cpu: Sync output buffer", which was eventually landed as "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the following commit back then: https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 However we eventually decided that more fine-grained control would be useful, as the sync operation can be very expensive. We could reconsider that now - but I think we should have a fix in 0.6 either way, and this here is the approach with the least churn. > > > Regards, > Barnabás Pőcze > > >> This was reported to fix glitches with the upcoming GPU-ISP, in which >> case the accessed data in written by the GPU. >> >> Note that in the GPU-ISP case we are effectively seeing a round-trip of >> the buffer contents - from GPU synced to CPU, copied to GPU again. We >> could avoid that in the future by implementing direct dmabuf import in >> qcam. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >> 2 files changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/src/apps/qcam/viewfinder_gl.cpp >> b/src/apps/qcam/viewfinder_gl.cpp >> index f31956ff0..0121c97ad 100644 >> --- a/src/apps/qcam/viewfinder_gl.cpp >> +++ b/src/apps/qcam/viewfinder_gl.cpp >> @@ -9,14 +9,16 @@ >> #include <array> >> +#include <libcamera/formats.h> >> + >> +#include "libcamera/internal/dma_buf_allocator.h" >> + >> #include <QByteArray> >> #include <QFile> >> #include <QImage> >> #include <QMatrix4x4> >> #include <QStringList> >> -#include <libcamera/formats.h> >> - >> #include "../common/image.h" >> static const QList<libcamera::PixelFormat> supportedFormats{ >> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >> /* Stride of the first plane, in pixels. */ >> unsigned int stridePixels; >> + std::vector<libcamera::DmaSyncer> dmaSyncers; >> + for (const libcamera::FrameBuffer::Plane &plane : >> buffer_->planes()) >> + dmaSyncers.emplace_back(plane.fd, >> libcamera::DmaSyncer::SyncType::Read); >> + >> switch (format_) { >> case libcamera::formats::NV12: >> case libcamera::formats::NV21: >> diff --git a/src/apps/qcam/viewfinder_qt.cpp >> b/src/apps/qcam/viewfinder_qt.cpp >> index 1a238922b..4d00f154d 100644 >> --- a/src/apps/qcam/viewfinder_qt.cpp >> +++ b/src/apps/qcam/viewfinder_qt.cpp >> @@ -11,6 +11,7 @@ >> #include <stdint.h> >> #include <utility> >> +#include "libcamera/internal/dma_buf_allocator.h" >> #include <libcamera/formats.h> >> #include <QImage> >> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer >> *buffer, Image *image) >> * Otherwise, convert the format and release the frame >> * buffer immediately. >> */ >> + std::vector<libcamera::DmaSyncer> dmaSyncers; >> + for (const libcamera::FrameBuffer::Plane &plane : >> buffer->planes()) >> + dmaSyncers.emplace_back(plane.fd, >> libcamera::DmaSyncer::SyncType::Read); >> + >> converter_.convert(image, size, &image_); >> } >> } >> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >> /* If we have an image, draw it, with black letterbox >> rectangles. */ >> if (!image_.isNull()) { >> + if (buffer_) { >> + std::vector<libcamera::DmaSyncer> dmaSyncers; >> + for (const libcamera::FrameBuffer::Plane &plane : >> buffer_->planes()) >> + dmaSyncers.emplace_back(plane.fd, >> libcamera::DmaSyncer::SyncType::Read); >> + } >> + >> if (place_.width() < width()) { >> QRect rect{ 0, 0, (width() - place_.width()) / 2, >> height() }; >> painter.drawRect(rect); >
2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: > On 20.11.25 10:58, Barnabás Pőcze wrote: >> Hi >> >> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >>> Use the DmaSyncer handler to ensure the data from the buffer is >>> coherent. This is required by the spec - from >>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >>> >>>> When a DMA buffer is accessed from the CPU via mmap, it is not always >>>> possible to guarantee coherency between the CPU-visible map and >>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used >>>> to bracket any CPU access to give the kernel the chance to shuffle memory >>>> around if needed. >>> >> >> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? > > We discussed that in the thread for "[PATCH] libcamera: debayer_cpu: Sync output buffer", which was eventually landed as "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the following commit back then: > > https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 > > However we eventually decided that more fine-grained control would be useful, as the sync operation can be very expensive. > > We could reconsider that now - but I think we should have a fix in 0.6 either way, and this here is the approach with the least churn. But every use of MappedFrameBuffer / Image, I think, pretty much implies that data will be accessed from the CPU, no? It's also not clear to me why the same change is not done in the `cam` app as well? On a related note, pipewire marks the libcamera buffers as SPA_DATA_FLAG_MAPPABLE, implying that they can just be mmapped and used. I assume this has to change? There does not seem to be anything in the application developers' guide explicitly mentioning when/how/if dma buf sync is needed; it would be good to make the requirements crystal clear. Regards, Barnabás Pőcze > >> >> >> Regards, >> Barnabás Pőcze >> >> >>> This was reported to fix glitches with the upcoming GPU-ISP, in which >>> case the accessed data in written by the GPU. >>> >>> Note that in the GPU-ISP case we are effectively seeing a round-trip of >>> the buffer contents - from GPU synced to CPU, copied to GPU again. We >>> could avoid that in the future by implementing direct dmabuf import in >>> qcam. >>> >>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>> --- >>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >>> 2 files changed, 19 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp >>> index f31956ff0..0121c97ad 100644 >>> --- a/src/apps/qcam/viewfinder_gl.cpp >>> +++ b/src/apps/qcam/viewfinder_gl.cpp >>> @@ -9,14 +9,16 @@ >>> #include <array> >>> +#include <libcamera/formats.h> >>> + >>> +#include "libcamera/internal/dma_buf_allocator.h" >>> + >>> #include <QByteArray> >>> #include <QFile> >>> #include <QImage> >>> #include <QMatrix4x4> >>> #include <QStringList> >>> -#include <libcamera/formats.h> >>> - >>> #include "../common/image.h" >>> static const QList<libcamera::PixelFormat> supportedFormats{ >>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >>> /* Stride of the first plane, in pixels. */ >>> unsigned int stridePixels; >>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>> + >>> switch (format_) { >>> case libcamera::formats::NV12: >>> case libcamera::formats::NV21: >>> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp >>> index 1a238922b..4d00f154d 100644 >>> --- a/src/apps/qcam/viewfinder_qt.cpp >>> +++ b/src/apps/qcam/viewfinder_qt.cpp >>> @@ -11,6 +11,7 @@ >>> #include <stdint.h> >>> #include <utility> >>> +#include "libcamera/internal/dma_buf_allocator.h" >>> #include <libcamera/formats.h> >>> #include <QImage> >>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) >>> * Otherwise, convert the format and release the frame >>> * buffer immediately. >>> */ >>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) >>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>> + >>> converter_.convert(image, size, &image_); >>> } >>> } >>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >>> /* If we have an image, draw it, with black letterbox rectangles. */ >>> if (!image_.isNull()) { >>> + if (buffer_) { >>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>> + } >>> + >>> if (place_.width() < width()) { >>> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; >>> painter.drawRect(rect); >>
Hi, On 20.11.25 13:35, Barnabás Pőcze wrote: > 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: >> On 20.11.25 10:58, Barnabás Pőcze wrote: >>> Hi >>> >>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >>>> Use the DmaSyncer handler to ensure the data from the buffer is >>>> coherent. This is required by the spec - from >>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >>>> >>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always >>>>> possible to guarantee coherency between the CPU-visible map and >>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be >>>>> used >>>>> to bracket any CPU access to give the kernel the chance to shuffle >>>>> memory >>>>> around if needed. >>>> >>> >>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? >> >> We discussed that in the thread for "[PATCH] libcamera: debayer_cpu: >> Sync output buffer", which was eventually landed as "libcamera: >> debayer_cpu: Sync DMABUFs" a year ago. I wrote the following commit >> back then: >> >> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 >> >> >> However we eventually decided that more fine-grained control would be >> useful, as the sync operation can be very expensive. >> >> We could reconsider that now - but I think we should have a fix in >> 0.6 either way, and this here is the approach with the least churn. > > But every use of MappedFrameBuffer / Image, I think, pretty much > implies that data > will be accessed from the CPU, no? Generally yes, but it's not necessarily optimal in every situation, which is why the kernel doesn't hide away the sync in the map call in the first place. It can e.g. be a good choice to keep a buffer mapped and only sync it when required, reducing some overhead. I'm not aware of any such usage in libcamera yet, however it's one of the things that we *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on that once the main series has landed (Gstreamer even has helpers for that, see e.g. https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) In the end it's a preference of choice I'd say. > > It's also not clear to me why the same change is not done in the `cam` > app as well? Should certainly be done in a follow-up, assuming SDL uses an upload-from-CPU approach as well. Haven't looked into it - I just quickly typed this patch down as it fixed glitches Hans was seeing on IPU7 (IIRC - or was it IPU6). > On a related note, pipewire marks the libcamera buffers as > SPA_DATA_FLAG_MAPPABLE, implying > that they can just be mmapped and used. I assume this has to change? > There does not seem to > be anything in the application developers' guide explicitly mentioning > when/how/if dma buf > sync is needed; it would be good to make the requirements crystal clear. Pipewire clients - or really any client doing anything with dmabufs - indeed have to know that if they use buffers with the SPA_DATA_DmaBuf type they have to sync them before accessing from the CPU. It's a fundamental part of the kernel API. When using Gstreamer that is hidden within the GstBuffer implementation, but other clients need to handle it indeed manually. In fact a quick look suggests that libwebrtc doesn't do it yet, which would also need to get fixed. > > Regards, > Barnabás Pőcze Regards > >> >>> >>> >>> Regards, >>> Barnabás Pőcze >>> >>> >>>> This was reported to fix glitches with the upcoming GPU-ISP, in which >>>> case the accessed data in written by the GPU. >>>> >>>> Note that in the GPU-ISP case we are effectively seeing a >>>> round-trip of >>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We >>>> could avoid that in the future by implementing direct dmabuf import in >>>> qcam. >>>> >>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>> --- >>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >>>> 2 files changed, 19 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp >>>> b/src/apps/qcam/viewfinder_gl.cpp >>>> index f31956ff0..0121c97ad 100644 >>>> --- a/src/apps/qcam/viewfinder_gl.cpp >>>> +++ b/src/apps/qcam/viewfinder_gl.cpp >>>> @@ -9,14 +9,16 @@ >>>> #include <array> >>>> +#include <libcamera/formats.h> >>>> + >>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>> + >>>> #include <QByteArray> >>>> #include <QFile> >>>> #include <QImage> >>>> #include <QMatrix4x4> >>>> #include <QStringList> >>>> -#include <libcamera/formats.h> >>>> - >>>> #include "../common/image.h" >>>> static const QList<libcamera::PixelFormat> supportedFormats{ >>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >>>> /* Stride of the first plane, in pixels. */ >>>> unsigned int stridePixels; >>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>> buffer_->planes()) >>>> + dmaSyncers.emplace_back(plane.fd, >>>> libcamera::DmaSyncer::SyncType::Read); >>>> + >>>> switch (format_) { >>>> case libcamera::formats::NV12: >>>> case libcamera::formats::NV21: >>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp >>>> b/src/apps/qcam/viewfinder_qt.cpp >>>> index 1a238922b..4d00f154d 100644 >>>> --- a/src/apps/qcam/viewfinder_qt.cpp >>>> +++ b/src/apps/qcam/viewfinder_qt.cpp >>>> @@ -11,6 +11,7 @@ >>>> #include <stdint.h> >>>> #include <utility> >>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>> #include <libcamera/formats.h> >>>> #include <QImage> >>>> @@ -114,6 +115,10 @@ void >>>> ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) >>>> * Otherwise, convert the format and release the frame >>>> * buffer immediately. >>>> */ >>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>> buffer->planes()) >>>> + dmaSyncers.emplace_back(plane.fd, >>>> libcamera::DmaSyncer::SyncType::Read); >>>> + >>>> converter_.convert(image, size, &image_); >>>> } >>>> } >>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >>>> /* If we have an image, draw it, with black letterbox >>>> rectangles. */ >>>> if (!image_.isNull()) { >>>> + if (buffer_) { >>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>> buffer_->planes()) >>>> + dmaSyncers.emplace_back(plane.fd, >>>> libcamera::DmaSyncer::SyncType::Read); >>>> + } >>>> + >>>> if (place_.width() < width()) { >>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, >>>> height() }; >>>> painter.drawRect(rect); >>> >
2025. 11. 20. 14:26 keltezéssel, Robert Mader írta: > Hi, > > On 20.11.25 13:35, Barnabás Pőcze wrote: >> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: >>> On 20.11.25 10:58, Barnabás Pőcze wrote: >>>> Hi >>>> >>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >>>>> Use the DmaSyncer handler to ensure the data from the buffer is >>>>> coherent. This is required by the spec - from >>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >>>>> >>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always >>>>>> possible to guarantee coherency between the CPU-visible map and >>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used >>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory >>>>>> around if needed. >>>>> >>>> >>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? >>> >>> We discussed that in the thread for "[PATCH] libcamera: debayer_cpu: Sync output buffer", which was eventually landed as "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the following commit back then: >>> >>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 >>> >>> However we eventually decided that more fine-grained control would be useful, as the sync operation can be very expensive. >>> >>> We could reconsider that now - but I think we should have a fix in 0.6 either way, and this here is the approach with the least churn. >> >> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data >> will be accessed from the CPU, no? > > Generally yes, but it's not necessarily optimal in every situation, which is why the kernel doesn't hide away the sync in the map call in the first place. > > It can e.g. be a good choice to keep a buffer mapped and only sync it when required, reducing some overhead. I'm not aware of any such usage in libcamera yet, however it's one of the things that we *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on that once the main series has landed (Gstreamer even has helpers for that, see e.g. https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) > > In the end it's a preference of choice I'd say. It seems to me that extending `MappedFrameBuffer` / `Image` is then the way forward, not necessarily adding it to the constructor/destructor but maybe adding extra member functions. > >> >> It's also not clear to me why the same change is not done in the `cam` app as well? > Should certainly be done in a follow-up, assuming SDL uses an upload-from-CPU approach as well. Haven't looked into it - I just quickly typed this patch down as it fixed glitches Hans was seeing on IPU7 (IIRC - or was it IPU6). It is passed the mmap-ed pointer from an `Image` object, so I can't imagine it does anything different. >> On a related note, pipewire marks the libcamera buffers as SPA_DATA_FLAG_MAPPABLE, implying >> that they can just be mmapped and used. I assume this has to change? There does not seem to >> be anything in the application developers' guide explicitly mentioning when/how/if dma buf >> sync is needed; it would be good to make the requirements crystal clear. > Pipewire clients - or really any client doing anything with dmabufs - indeed have to know that if they use buffers with the SPA_DATA_DmaBuf type they have to sync them before accessing from the CPU. It's a fundamental part of the kernel API. When using Gstreamer that is hidden within the GstBuffer implementation, but other clients need to handle it indeed manually. So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not correct there. > In fact a quick look suggests that libwebrtc doesn't do it yet, which would also need to get fixed. As far as I can tell it does: * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476 * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99 at least the libwebrtc copy in firefox. It actually mmap/munmap every frame, which is likely not ideal. >> >> Regards, >> Barnabás Pőcze > Regards >> >>> >>>> >>>> >>>> Regards, >>>> Barnabás Pőcze >>>> >>>> >>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which >>>>> case the accessed data in written by the GPU. >>>>> >>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of >>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We >>>>> could avoid that in the future by implementing direct dmabuf import in >>>>> qcam. >>>>> >>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>> --- >>>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >>>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >>>>> 2 files changed, 19 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp >>>>> index f31956ff0..0121c97ad 100644 >>>>> --- a/src/apps/qcam/viewfinder_gl.cpp >>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp >>>>> @@ -9,14 +9,16 @@ >>>>> #include <array> >>>>> +#include <libcamera/formats.h> >>>>> + >>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>> + >>>>> #include <QByteArray> >>>>> #include <QFile> >>>>> #include <QImage> >>>>> #include <QMatrix4x4> >>>>> #include <QStringList> >>>>> -#include <libcamera/formats.h> >>>>> - >>>>> #include "../common/image.h" >>>>> static const QList<libcamera::PixelFormat> supportedFormats{ >>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >>>>> /* Stride of the first plane, in pixels. */ >>>>> unsigned int stridePixels; >>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>> + >>>>> switch (format_) { >>>>> case libcamera::formats::NV12: >>>>> case libcamera::formats::NV21: >>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp >>>>> index 1a238922b..4d00f154d 100644 >>>>> --- a/src/apps/qcam/viewfinder_qt.cpp >>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp >>>>> @@ -11,6 +11,7 @@ >>>>> #include <stdint.h> >>>>> #include <utility> >>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>> #include <libcamera/formats.h> >>>>> #include <QImage> >>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) >>>>> * Otherwise, convert the format and release the frame >>>>> * buffer immediately. >>>>> */ >>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) >>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>> + >>>>> converter_.convert(image, size, &image_); >>>>> } >>>>> } >>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >>>>> /* If we have an image, draw it, with black letterbox rectangles. */ >>>>> if (!image_.isNull()) { >>>>> + if (buffer_) { >>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>> + } >>>>> + >>>>> if (place_.width() < width()) { >>>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; >>>>> painter.drawRect(rect); >>>> >>
On 20.11.25 14:54, Barnabás Pőcze wrote: > 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta: >> Hi, >> >> On 20.11.25 13:35, Barnabás Pőcze wrote: >>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: >>>> On 20.11.25 10:58, Barnabás Pőcze wrote: >>>>> Hi >>>>> >>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >>>>>> Use the DmaSyncer handler to ensure the data from the buffer is >>>>>> coherent. This is required by the spec - from >>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >>>>>> >>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not >>>>>>> always >>>>>>> possible to guarantee coherency between the CPU-visible map and >>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must >>>>>>> be used >>>>>>> to bracket any CPU access to give the kernel the chance to >>>>>>> shuffle memory >>>>>>> around if needed. >>>>>> >>>>> >>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? >>>> >>>> We discussed that in the thread for "[PATCH] libcamera: >>>> debayer_cpu: Sync output buffer", which was eventually landed as >>>> "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the >>>> following commit back then: >>>> >>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 >>>> >>>> >>>> However we eventually decided that more fine-grained control would >>>> be useful, as the sync operation can be very expensive. >>>> >>>> We could reconsider that now - but I think we should have a fix in >>>> 0.6 either way, and this here is the approach with the least churn. >>> >>> But every use of MappedFrameBuffer / Image, I think, pretty much >>> implies that data >>> will be accessed from the CPU, no? >> >> Generally yes, but it's not necessarily optimal in every situation, >> which is why the kernel doesn't hide away the sync in the map call in >> the first place. >> >> It can e.g. be a good choice to keep a buffer mapped and only sync it >> when required, reducing some overhead. I'm not aware of any such >> usage in libcamera yet, however it's one of the things that we >> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on >> that once the main series has landed (Gstreamer even has helpers for >> that, see e.g. >> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) >> >> In the end it's a preference of choice I'd say. > > It seems to me that extending `MappedFrameBuffer` / `Image` is then > the way forward, not > necessarily adding it to the constructor/destructor but maybe adding > extra member functions. Unfortunately I most likely won't have time to into that again the next two weeks, so if we want qcam fixed in 0.6, somebody else would need to take that over. > >> >>> >>> It's also not clear to me why the same change is not done in the >>> `cam` app as well? >> Should certainly be done in a follow-up, assuming SDL uses an >> upload-from-CPU approach as well. Haven't looked into it - I just >> quickly typed this patch down as it fixed glitches Hans was seeing on >> IPU7 (IIRC - or was it IPU6). > > It is passed the mmap-ed pointer from an `Image` object, so I can't > imagine it does anything different. > >>> On a related note, pipewire marks the libcamera buffers as >>> SPA_DATA_FLAG_MAPPABLE, implying >>> that they can just be mmapped and used. I assume this has to change? >>> There does not seem to >>> be anything in the application developers' guide explicitly >>> mentioning when/how/if dma buf >>> sync is needed; it would be good to make the requirements crystal >>> clear. >> Pipewire clients - or really any client doing anything with dmabufs - >> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf >> type they have to sync them before accessing from the CPU. It's a >> fundamental part of the kernel API. When using Gstreamer that is >> hidden within the GstBuffer implementation, but other clients need to >> handle it indeed manually. > > So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not > correct there. I'd say it's still close enough. AFAIK not having the flag is usually interpreted as "you have to import via GL/VK" - like in the case of modifiers. But that should probably be discussed in a PW issue. > >> In fact a quick look suggests that libwebrtc doesn't do it yet, which >> would also need to get fixed. > > As far as I can tell it does: > > * > https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476 > * > https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99 > > at least the libwebrtc copy in firefox. It actually mmap/munmap every > frame, which is likely not ideal. Ah nice, thanks, missed that. Good, so both libwebrtc and Gstreamer based apps are fine. >>> >>> Regards, >>> Barnabás Pőcze >> Regards >>> >>>> >>>>> >>>>> >>>>> Regards, >>>>> Barnabás Pőcze >>>>> >>>>> >>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in >>>>>> which >>>>>> case the accessed data in written by the GPU. >>>>>> >>>>>> Note that in the GPU-ISP case we are effectively seeing a >>>>>> round-trip of >>>>>> the buffer contents - from GPU synced to CPU, copied to GPU >>>>>> again. We >>>>>> could avoid that in the future by implementing direct dmabuf >>>>>> import in >>>>>> qcam. >>>>>> >>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>>> --- >>>>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >>>>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >>>>>> 2 files changed, 19 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp >>>>>> b/src/apps/qcam/viewfinder_gl.cpp >>>>>> index f31956ff0..0121c97ad 100644 >>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp >>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp >>>>>> @@ -9,14 +9,16 @@ >>>>>> #include <array> >>>>>> +#include <libcamera/formats.h> >>>>>> + >>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>> + >>>>>> #include <QByteArray> >>>>>> #include <QFile> >>>>>> #include <QImage> >>>>>> #include <QMatrix4x4> >>>>>> #include <QStringList> >>>>>> -#include <libcamera/formats.h> >>>>>> - >>>>>> #include "../common/image.h" >>>>>> static const QList<libcamera::PixelFormat> supportedFormats{ >>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >>>>>> /* Stride of the first plane, in pixels. */ >>>>>> unsigned int stridePixels; >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>>>> buffer_->planes()) >>>>>> + dmaSyncers.emplace_back(plane.fd, >>>>>> libcamera::DmaSyncer::SyncType::Read); >>>>>> + >>>>>> switch (format_) { >>>>>> case libcamera::formats::NV12: >>>>>> case libcamera::formats::NV21: >>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp >>>>>> b/src/apps/qcam/viewfinder_qt.cpp >>>>>> index 1a238922b..4d00f154d 100644 >>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp >>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp >>>>>> @@ -11,6 +11,7 @@ >>>>>> #include <stdint.h> >>>>>> #include <utility> >>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>> #include <libcamera/formats.h> >>>>>> #include <QImage> >>>>>> @@ -114,6 +115,10 @@ void >>>>>> ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) >>>>>> * Otherwise, convert the format and release the frame >>>>>> * buffer immediately. >>>>>> */ >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>>>> buffer->planes()) >>>>>> + dmaSyncers.emplace_back(plane.fd, >>>>>> libcamera::DmaSyncer::SyncType::Read); >>>>>> + >>>>>> converter_.convert(image, size, &image_); >>>>>> } >>>>>> } >>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >>>>>> /* If we have an image, draw it, with black letterbox >>>>>> rectangles. */ >>>>>> if (!image_.isNull()) { >>>>>> + if (buffer_) { >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : >>>>>> buffer_->planes()) >>>>>> + dmaSyncers.emplace_back(plane.fd, >>>>>> libcamera::DmaSyncer::SyncType::Read); >>>>>> + } >>>>>> + >>>>>> if (place_.width() < width()) { >>>>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, >>>>>> height() }; >>>>>> painter.drawRect(rect); >>>>> >>> >
On Thu, Nov 20, 2025 at 10:49:19AM +0100, Robert Mader wrote: > Use the DmaSyncer handler to ensure the data from the buffer is > coherent. This is required by the spec - from > https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: > > > When a DMA buffer is accessed from the CPU via mmap, it is not always > > possible to guarantee coherency between the CPU-visible map and > > underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used > > to bracket any CPU access to give the kernel the chance to shuffle memory > > around if needed. > > This was reported to fix glitches with the upcoming GPU-ISP, in which > case the accessed data in written by the GPU. > > Note that in the GPU-ISP case we are effectively seeing a round-trip of > the buffer contents - from GPU synced to CPU, copied to GPU again. We > could avoid that in the future by implementing direct dmabuf import in > qcam. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- > src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp > index f31956ff0..0121c97ad 100644 > --- a/src/apps/qcam/viewfinder_gl.cpp > +++ b/src/apps/qcam/viewfinder_gl.cpp > @@ -9,14 +9,16 @@ > > #include <array> > > +#include <libcamera/formats.h> > + > +#include "libcamera/internal/dma_buf_allocator.h" As the name implies, this is an internal header. It should not be used in qcam. > + > #include <QByteArray> > #include <QFile> > #include <QImage> > #include <QMatrix4x4> > #include <QStringList> > > -#include <libcamera/formats.h> > - > #include "../common/image.h" > > static const QList<libcamera::PixelFormat> supportedFormats{ > @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() > /* Stride of the first plane, in pixels. */ > unsigned int stridePixels; > > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + > switch (format_) { > case libcamera::formats::NV12: > case libcamera::formats::NV21: > diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp > index 1a238922b..4d00f154d 100644 > --- a/src/apps/qcam/viewfinder_qt.cpp > +++ b/src/apps/qcam/viewfinder_qt.cpp > @@ -11,6 +11,7 @@ > #include <stdint.h> > #include <utility> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include <libcamera/formats.h> > > #include <QImage> > @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) > * Otherwise, convert the format and release the frame > * buffer immediately. > */ > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + > converter_.convert(image, size, &image_); > } > } > @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) > > /* If we have an image, draw it, with black letterbox rectangles. */ > if (!image_.isNull()) { > + if (buffer_) { > + std::vector<libcamera::DmaSyncer> dmaSyncers; > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > + } > + > if (place_.width() < width()) { > QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; > painter.drawRect(rect);
On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote: > On 20.11.25 14:54, Barnabás Pőcze wrote: > > 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta: > >> On 20.11.25 13:35, Barnabás Pőcze wrote: > >>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: > >>>> On 20.11.25 10:58, Barnabás Pőcze wrote: > >>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: > >>>>>> Use the DmaSyncer handler to ensure the data from the buffer is > >>>>>> coherent. This is required by the spec - from > >>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: > >>>>>> > >>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always > >>>>>>> possible to guarantee coherency between the CPU-visible map and > >>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used > >>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory > >>>>>>> around if needed. > >>>>> > >>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? > >>>> > >>>> We discussed that in the thread for "[PATCH] libcamera: > >>>> debayer_cpu: Sync output buffer", which was eventually landed as > >>>> "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the > >>>> following commit back then: > >>>> > >>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 > >>>> > >>>> However we eventually decided that more fine-grained control would > >>>> be useful, as the sync operation can be very expensive. > >>>> > >>>> We could reconsider that now - but I think we should have a fix in > >>>> 0.6 either way, and this here is the approach with the least churn. > >>> > >>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data > >>> will be accessed from the CPU, no? > >> > >> Generally yes, but it's not necessarily optimal in every situation, > >> which is why the kernel doesn't hide away the sync in the map call in > >> the first place. > >> > >> It can e.g. be a good choice to keep a buffer mapped and only sync it > >> when required, reducing some overhead. I'm not aware of any such > >> usage in libcamera yet, however it's one of the things that we > >> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on If you're talking about the images produced by the GPU ISP, then I think the buffers should not be mapped by libcamera at all. > >> that once the main series has landed (Gstreamer even has helpers for > >> that, see e.g. > >> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) > >> > >> In the end it's a preference of choice I'd say. > > > > It seems to me that extending `MappedFrameBuffer` / `Image` is then > > the way forward, not > > necessarily adding it to the constructor/destructor but maybe adding > > extra member functions. That sounds like a good idea. Especially given that qcam shouldn't use the internal dma_buf_allocator.h header, implementing cache sync in the Image class would be cleaner. > Unfortunately I most likely won't have time to into that again the next > two weeks, so if we want qcam fixed in 0.6, somebody else would need to > take that over. > > >>> It's also not clear to me why the same change is not done in the > >>> `cam` app as well? > >> > >> Should certainly be done in a follow-up, assuming SDL uses an > >> upload-from-CPU approach as well. Haven't looked into it - I just > >> quickly typed this patch down as it fixed glitches Hans was seeing on > >> IPU7 (IIRC - or was it IPU6). > > > > It is passed the mmap-ed pointer from an `Image` object, so I can't > > imagine it does anything different. > > > >>> On a related note, pipewire marks the libcamera buffers as > >>> SPA_DATA_FLAG_MAPPABLE, implying > >>> that they can just be mmapped and used. I assume this has to change? > >>> There does not seem to > >>> be anything in the application developers' guide explicitly > >>> mentioning when/how/if dma buf > >>> sync is needed; it would be good to make the requirements crystal > >>> clear. > >> > >> Pipewire clients - or really any client doing anything with dmabufs - > >> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf > >> type they have to sync them before accessing from the CPU. It's a > >> fundamental part of the kernel API. When using Gstreamer that is > >> hidden within the GstBuffer implementation, but other clients need to > >> handle it indeed manually. > > > > So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not > > correct there. > > I'd say it's still close enough. AFAIK not having the flag is usually > interpreted as "you have to import via GL/VK" - like in the case of > modifiers. But that should probably be discussed in a PW issue. > > >> In fact a quick look suggests that libwebrtc doesn't do it yet, which > >> would also need to get fixed. > > > > As far as I can tell it does: > > > > * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476 > > * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99 > > > > at least the libwebrtc copy in firefox. It actually mmap/munmap every > > frame, which is likely not ideal. > > Ah nice, thanks, missed that. > > Good, so both libwebrtc and Gstreamer based apps are fine. > > >>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which > >>>>>> case the accessed data in written by the GPU. > >>>>>> > >>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of > >>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We > >>>>>> could avoid that in the future by implementing direct dmabuf import in > >>>>>> qcam. > >>>>>> > >>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> > >>>>>> --- > >>>>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- > >>>>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ > >>>>>> 2 files changed, 19 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp > >>>>>> b/src/apps/qcam/viewfinder_gl.cpp > >>>>>> index f31956ff0..0121c97ad 100644 > >>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp > >>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp > >>>>>> @@ -9,14 +9,16 @@ > >>>>>> #include <array> > >>>>>> +#include <libcamera/formats.h> > >>>>>> + > >>>>>> +#include "libcamera/internal/dma_buf_allocator.h" > >>>>>> + > >>>>>> #include <QByteArray> > >>>>>> #include <QFile> > >>>>>> #include <QImage> > >>>>>> #include <QMatrix4x4> > >>>>>> #include <QStringList> > >>>>>> -#include <libcamera/formats.h> > >>>>>> - > >>>>>> #include "../common/image.h" > >>>>>> static const QList<libcamera::PixelFormat> supportedFormats{ > >>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() > >>>>>> /* Stride of the first plane, in pixels. */ > >>>>>> unsigned int stridePixels; > >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > >>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>> + > >>>>>> switch (format_) { > >>>>>> case libcamera::formats::NV12: > >>>>>> case libcamera::formats::NV21: > >>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp > >>>>>> b/src/apps/qcam/viewfinder_qt.cpp > >>>>>> index 1a238922b..4d00f154d 100644 > >>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp > >>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp > >>>>>> @@ -11,6 +11,7 @@ > >>>>>> #include <stdint.h> > >>>>>> #include <utility> > >>>>>> +#include "libcamera/internal/dma_buf_allocator.h" > >>>>>> #include <libcamera/formats.h> > >>>>>> #include <QImage> > >>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) > >>>>>> * Otherwise, convert the format and release the frame > >>>>>> * buffer immediately. > >>>>>> */ > >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) > >>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>> + > >>>>>> converter_.convert(image, size, &image_); > >>>>>> } > >>>>>> } > >>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) > >>>>>> /* If we have an image, draw it, with black letterbox rectangles. */ > >>>>>> if (!image_.isNull()) { > >>>>>> + if (buffer_) { > >>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > >>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>> + } This is wrong, dmaSyncers will be destroyed as soon as you exit from the scope, not after finishing accessing the data. > >>>>>> + > >>>>>> if (place_.width() < width()) { > >>>>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; > >>>>>> painter.drawRect(rect);
Hi On 21.11.25 04:52, Laurent Pinchart wrote: > On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote: >> On 20.11.25 14:54, Barnabás Pőcze wrote: >>> 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta: >>>> On 20.11.25 13:35, Barnabás Pőcze wrote: >>>>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: >>>>>> On 20.11.25 10:58, Barnabás Pőcze wrote: >>>>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: >>>>>>>> Use the DmaSyncer handler to ensure the data from the buffer is >>>>>>>> coherent. This is required by the spec - from >>>>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: >>>>>>>> >>>>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always >>>>>>>>> possible to guarantee coherency between the CPU-visible map and >>>>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used >>>>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory >>>>>>>>> around if needed. >>>>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? >>>>>> We discussed that in the thread for "[PATCH] libcamera: >>>>>> debayer_cpu: Sync output buffer", which was eventually landed as >>>>>> "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the >>>>>> following commit back then: >>>>>> >>>>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 >>>>>> >>>>>> However we eventually decided that more fine-grained control would >>>>>> be useful, as the sync operation can be very expensive. >>>>>> >>>>>> We could reconsider that now - but I think we should have a fix in >>>>>> 0.6 either way, and this here is the approach with the least churn. >>>>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data >>>>> will be accessed from the CPU, no? >>>> Generally yes, but it's not necessarily optimal in every situation, >>>> which is why the kernel doesn't hide away the sync in the map call in >>>> the first place. >>>> >>>> It can e.g. be a good choice to keep a buffer mapped and only sync it >>>> when required, reducing some overhead. I'm not aware of any such >>>> usage in libcamera yet, however it's one of the things that we >>>> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on > If you're talking about the images produced by the GPU ISP, then I think > the buffers should not be mapped by libcamera at all. No, I meant the the input/v4l2 buffers. Even there we'll hopefully be able to avoid CPU access for most frames in the future (direct GPU import + only collecting stats on every so many frames) - but there might still be a small benefit in keeping the buffers mapped. > >>>> that once the main series has landed (Gstreamer even has helpers for >>>> that, see e.g. >>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) >>>> >>>> In the end it's a preference of choice I'd say. >>> It seems to me that extending `MappedFrameBuffer` / `Image` is then >>> the way forward, not >>> necessarily adding it to the constructor/destructor but maybe adding >>> extra member functions. > That sounds like a good idea. Especially given that qcam shouldn't use > the internal dma_buf_allocator.h header, implementing cache sync in the > Image class would be cleaner. > >> Unfortunately I most likely won't have time to into that again the next >> two weeks, so if we want qcam fixed in 0.6, somebody else would need to >> take that over. >> >>>>> It's also not clear to me why the same change is not done in the >>>>> `cam` app as well? >>>> Should certainly be done in a follow-up, assuming SDL uses an >>>> upload-from-CPU approach as well. Haven't looked into it - I just >>>> quickly typed this patch down as it fixed glitches Hans was seeing on >>>> IPU7 (IIRC - or was it IPU6). >>> It is passed the mmap-ed pointer from an `Image` object, so I can't >>> imagine it does anything different. >>> >>>>> On a related note, pipewire marks the libcamera buffers as >>>>> SPA_DATA_FLAG_MAPPABLE, implying >>>>> that they can just be mmapped and used. I assume this has to change? >>>>> There does not seem to >>>>> be anything in the application developers' guide explicitly >>>>> mentioning when/how/if dma buf >>>>> sync is needed; it would be good to make the requirements crystal >>>>> clear. >>>> Pipewire clients - or really any client doing anything with dmabufs - >>>> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf >>>> type they have to sync them before accessing from the CPU. It's a >>>> fundamental part of the kernel API. When using Gstreamer that is >>>> hidden within the GstBuffer implementation, but other clients need to >>>> handle it indeed manually. >>> So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not >>> correct there. >> I'd say it's still close enough. AFAIK not having the flag is usually >> interpreted as "you have to import via GL/VK" - like in the case of >> modifiers. But that should probably be discussed in a PW issue. >> >>>> In fact a quick look suggests that libwebrtc doesn't do it yet, which >>>> would also need to get fixed. >>> As far as I can tell it does: >>> >>> * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476 >>> * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99 >>> >>> at least the libwebrtc copy in firefox. It actually mmap/munmap every >>> frame, which is likely not ideal. >> Ah nice, thanks, missed that. >> >> Good, so both libwebrtc and Gstreamer based apps are fine. >> >>>>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which >>>>>>>> case the accessed data in written by the GPU. >>>>>>>> >>>>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of >>>>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We >>>>>>>> could avoid that in the future by implementing direct dmabuf import in >>>>>>>> qcam. >>>>>>>> >>>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>>>>> --- >>>>>>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- >>>>>>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ >>>>>>>> 2 files changed, 19 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp >>>>>>>> b/src/apps/qcam/viewfinder_gl.cpp >>>>>>>> index f31956ff0..0121c97ad 100644 >>>>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp >>>>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp >>>>>>>> @@ -9,14 +9,16 @@ >>>>>>>> #include <array> >>>>>>>> +#include <libcamera/formats.h> >>>>>>>> + >>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>>> + >>>>>>>> #include <QByteArray> >>>>>>>> #include <QFile> >>>>>>>> #include <QImage> >>>>>>>> #include <QMatrix4x4> >>>>>>>> #include <QStringList> >>>>>>>> -#include <libcamera/formats.h> >>>>>>>> - >>>>>>>> #include "../common/image.h" >>>>>>>> static const QList<libcamera::PixelFormat> supportedFormats{ >>>>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() >>>>>>>> /* Stride of the first plane, in pixels. */ >>>>>>>> unsigned int stridePixels; >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>>>>> + >>>>>>>> switch (format_) { >>>>>>>> case libcamera::formats::NV12: >>>>>>>> case libcamera::formats::NV21: >>>>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp >>>>>>>> b/src/apps/qcam/viewfinder_qt.cpp >>>>>>>> index 1a238922b..4d00f154d 100644 >>>>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp >>>>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp >>>>>>>> @@ -11,6 +11,7 @@ >>>>>>>> #include <stdint.h> >>>>>>>> #include <utility> >>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>>> #include <libcamera/formats.h> >>>>>>>> #include <QImage> >>>>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) >>>>>>>> * Otherwise, convert the format and release the frame >>>>>>>> * buffer immediately. >>>>>>>> */ >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>>>>> + >>>>>>>> converter_.convert(image, size, &image_); >>>>>>>> } >>>>>>>> } >>>>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) >>>>>>>> /* If we have an image, draw it, with black letterbox rectangles. */ >>>>>>>> if (!image_.isNull()) { >>>>>>>> + if (buffer_) { >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); >>>>>>>> + } > This is wrong, dmaSyncers will be destroyed as soon as you exit from the > scope, not after finishing accessing the data. Whoops, indeed! Shouldn't impact the result - once synced the data should be concurrent and won't change until the buffer is recycled - but from an API perspective it's not fully correct. >>>>>>>> + >>>>>>>> if (place_.width() < width()) { >>>>>>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; >>>>>>>> painter.drawRect(rect);
On Fri, Nov 21, 2025 at 10:49:31AM +0100, Robert Mader wrote: > On 21.11.25 04:52, Laurent Pinchart wrote: > > On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote: > >> On 20.11.25 14:54, Barnabás Pőcze wrote: > >>> 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta: > >>>> On 20.11.25 13:35, Barnabás Pőcze wrote: > >>>>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta: > >>>>>> On 20.11.25 10:58, Barnabás Pőcze wrote: > >>>>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta: > >>>>>>>> Use the DmaSyncer handler to ensure the data from the buffer is > >>>>>>>> coherent. This is required by the spec - from > >>>>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync: > >>>>>>>> > >>>>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always > >>>>>>>>> possible to guarantee coherency between the CPU-visible map and > >>>>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used > >>>>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory > >>>>>>>>> around if needed. > >>>>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then? > >>>>>> We discussed that in the thread for "[PATCH] libcamera: > >>>>>> debayer_cpu: Sync output buffer", which was eventually landed as > >>>>>> "libcamera: debayer_cpu: Sync DMABUFs" a year ago. I wrote the > >>>>>> following commit back then: > >>>>>> > >>>>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 > >>>>>> > >>>>>> However we eventually decided that more fine-grained control would > >>>>>> be useful, as the sync operation can be very expensive. > >>>>>> > >>>>>> We could reconsider that now - but I think we should have a fix in > >>>>>> 0.6 either way, and this here is the approach with the least churn. > >>>>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data > >>>>> will be accessed from the CPU, no? > >>>> Generally yes, but it's not necessarily optimal in every situation, > >>>> which is why the kernel doesn't hide away the sync in the map call in > >>>> the first place. > >>>> > >>>> It can e.g. be a good choice to keep a buffer mapped and only sync it > >>>> when required, reducing some overhead. I'm not aware of any such > >>>> usage in libcamera yet, however it's one of the things that we > >>>> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on > > If you're talking about the images produced by the GPU ISP, then I think > > the buffers should not be mapped by libcamera at all. > > No, I meant the the input/v4l2 buffers. Even there we'll hopefully be > able to avoid CPU access for most frames in the future (direct GPU > import + only collecting stats on every so many frames) - but there > might still be a small benefit in keeping the buffers mapped. For the CPU-based soft ISP we'll obviously need to map the buffers. For the GPU-based soft ISP we will also need a CPU mapping until we find a way to compute statistics with the GPU. As far as I understand, that's not the priority, so we'll need to deal with caches for a while. It would be nice not to though. > >>>> that once the main series has landed (Gstreamer even has helpers for > >>>> that, see e.g. > >>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135) > >>>> > >>>> In the end it's a preference of choice I'd say. > >>> It seems to me that extending `MappedFrameBuffer` / `Image` is then > >>> the way forward, not > >>> necessarily adding it to the constructor/destructor but maybe adding > >>> extra member functions. > > That sounds like a good idea. Especially given that qcam shouldn't use > > the internal dma_buf_allocator.h header, implementing cache sync in the > > Image class would be cleaner. > > > >> Unfortunately I most likely won't have time to into that again the next > >> two weeks, so if we want qcam fixed in 0.6, somebody else would need to > >> take that over. > >> > >>>>> It's also not clear to me why the same change is not done in the > >>>>> `cam` app as well? > >>>> Should certainly be done in a follow-up, assuming SDL uses an > >>>> upload-from-CPU approach as well. Haven't looked into it - I just > >>>> quickly typed this patch down as it fixed glitches Hans was seeing on > >>>> IPU7 (IIRC - or was it IPU6). > >>> It is passed the mmap-ed pointer from an `Image` object, so I can't > >>> imagine it does anything different. > >>> > >>>>> On a related note, pipewire marks the libcamera buffers as > >>>>> SPA_DATA_FLAG_MAPPABLE, implying > >>>>> that they can just be mmapped and used. I assume this has to change? > >>>>> There does not seem to > >>>>> be anything in the application developers' guide explicitly > >>>>> mentioning when/how/if dma buf > >>>>> sync is needed; it would be good to make the requirements crystal > >>>>> clear. > >>>> Pipewire clients - or really any client doing anything with dmabufs - > >>>> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf > >>>> type they have to sync them before accessing from the CPU. It's a > >>>> fundamental part of the kernel API. When using Gstreamer that is > >>>> hidden within the GstBuffer implementation, but other clients need to > >>>> handle it indeed manually. > >>> So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not > >>> correct there. > >> I'd say it's still close enough. AFAIK not having the flag is usually > >> interpreted as "you have to import via GL/VK" - like in the case of > >> modifiers. But that should probably be discussed in a PW issue. > >> > >>>> In fact a quick look suggests that libwebrtc doesn't do it yet, which > >>>> would also need to get fixed. > >>> As far as I can tell it does: > >>> > >>> * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476 > >>> * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99 > >>> > >>> at least the libwebrtc copy in firefox. It actually mmap/munmap every > >>> frame, which is likely not ideal. > >> Ah nice, thanks, missed that. > >> > >> Good, so both libwebrtc and Gstreamer based apps are fine. > >> > >>>>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which > >>>>>>>> case the accessed data in written by the GPU. > >>>>>>>> > >>>>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of > >>>>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We > >>>>>>>> could avoid that in the future by implementing direct dmabuf import in > >>>>>>>> qcam. > >>>>>>>> > >>>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> > >>>>>>>> --- > >>>>>>>> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++-- > >>>>>>>> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++ > >>>>>>>> 2 files changed, 19 insertions(+), 2 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp > >>>>>>>> b/src/apps/qcam/viewfinder_gl.cpp > >>>>>>>> index f31956ff0..0121c97ad 100644 > >>>>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp > >>>>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp > >>>>>>>> @@ -9,14 +9,16 @@ > >>>>>>>> #include <array> > >>>>>>>> +#include <libcamera/formats.h> > >>>>>>>> + > >>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" > >>>>>>>> + > >>>>>>>> #include <QByteArray> > >>>>>>>> #include <QFile> > >>>>>>>> #include <QImage> > >>>>>>>> #include <QMatrix4x4> > >>>>>>>> #include <QStringList> > >>>>>>>> -#include <libcamera/formats.h> > >>>>>>>> - > >>>>>>>> #include "../common/image.h" > >>>>>>>> static const QList<libcamera::PixelFormat> supportedFormats{ > >>>>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() > >>>>>>>> /* Stride of the first plane, in pixels. */ > >>>>>>>> unsigned int stridePixels; > >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>>>> + > >>>>>>>> switch (format_) { > >>>>>>>> case libcamera::formats::NV12: > >>>>>>>> case libcamera::formats::NV21: > >>>>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp > >>>>>>>> b/src/apps/qcam/viewfinder_qt.cpp > >>>>>>>> index 1a238922b..4d00f154d 100644 > >>>>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp > >>>>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp > >>>>>>>> @@ -11,6 +11,7 @@ > >>>>>>>> #include <stdint.h> > >>>>>>>> #include <utility> > >>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" > >>>>>>>> #include <libcamera/formats.h> > >>>>>>>> #include <QImage> > >>>>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) > >>>>>>>> * Otherwise, convert the format and release the frame > >>>>>>>> * buffer immediately. > >>>>>>>> */ > >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) > >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>>>> + > >>>>>>>> converter_.convert(image, size, &image_); > >>>>>>>> } > >>>>>>>> } > >>>>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) > >>>>>>>> /* If we have an image, draw it, with black letterbox rectangles. */ > >>>>>>>> if (!image_.isNull()) { > >>>>>>>> + if (buffer_) { > >>>>>>>> + std::vector<libcamera::DmaSyncer> dmaSyncers; > >>>>>>>> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) > >>>>>>>> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); > >>>>>>>> + } > > This is wrong, dmaSyncers will be destroyed as soon as you exit from the > > scope, not after finishing accessing the data. > Whoops, indeed! Shouldn't impact the result - once synced the data > should be concurrent and won't change until the buffer is recycled - but > from an API perspective it's not fully correct. > >>>>>>>> + > >>>>>>>> if (place_.width() < width()) { > >>>>>>>> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; > >>>>>>>> painter.drawRect(rect);
diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp index f31956ff0..0121c97ad 100644 --- a/src/apps/qcam/viewfinder_gl.cpp +++ b/src/apps/qcam/viewfinder_gl.cpp @@ -9,14 +9,16 @@ #include <array> +#include <libcamera/formats.h> + +#include "libcamera/internal/dma_buf_allocator.h" + #include <QByteArray> #include <QFile> #include <QImage> #include <QMatrix4x4> #include <QStringList> -#include <libcamera/formats.h> - #include "../common/image.h" static const QList<libcamera::PixelFormat> supportedFormats{ @@ -542,6 +544,10 @@ void ViewFinderGL::doRender() /* Stride of the first plane, in pixels. */ unsigned int stridePixels; + std::vector<libcamera::DmaSyncer> dmaSyncers; + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); + switch (format_) { case libcamera::formats::NV12: case libcamera::formats::NV21: diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp index 1a238922b..4d00f154d 100644 --- a/src/apps/qcam/viewfinder_qt.cpp +++ b/src/apps/qcam/viewfinder_qt.cpp @@ -11,6 +11,7 @@ #include <stdint.h> #include <utility> +#include "libcamera/internal/dma_buf_allocator.h" #include <libcamera/formats.h> #include <QImage> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image) * Otherwise, convert the format and release the frame * buffer immediately. */ + std::vector<libcamera::DmaSyncer> dmaSyncers; + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes()) + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); + converter_.convert(image, size, &image_); } } @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *) /* If we have an image, draw it, with black letterbox rectangles. */ if (!image_.isNull()) { + if (buffer_) { + std::vector<libcamera::DmaSyncer> dmaSyncers; + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes()) + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read); + } + if (place_.width() < width()) { QRect rect{ 0, 0, (width() - place_.width()) / 2, height() }; painter.drawRect(rect);