| 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); >>>>> >>> >
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);