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