| Message ID | 20241121055436.2502314-2-chenghaoyang@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Harvey Yang <chenghaoyang@chromium.org> writes: > To synchronize CPU access with mmap and hardware access on DMA buffers, > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > a helper class to allow users to sync buffers more easily. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> OK for me. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > 2 files changed, 96 insertions(+) > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > index d2a0a0d19..e4073f668 100644 > --- a/include/libcamera/internal/dma_buf_allocator.h > +++ b/include/libcamera/internal/dma_buf_allocator.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <libcamera/base/flags.h> > +#include <libcamera/base/shared_fd.h> > #include <libcamera/base/unique_fd.h> > > namespace libcamera { > @@ -35,6 +36,26 @@ private: > DmaBufAllocatorFlag type_; > }; > > +class DmaSyncer final > +{ > +public: > + enum class SyncType { > + Read = 0, > + Write, > + ReadWrite, > + }; > + > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > + > + ~DmaSyncer(); > + > +private: > + void sync(uint64_t step); > + > + SharedFD fd_; > + uint64_t flags_ = 0; > +}; > + > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > } /* namespace libcamera */ > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index be6efb89f..c1c2103d6 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > return allocFromHeap(name, size); > } > > +/** > + * \class DmaSyncer > + * \brief Helper class for dma-buf's synchronization > + * > + * This class wraps a userspace dma-buf's synchronization process with an > + * object's lifetime. > + * > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > + * ISP. > + */ > + > +/** > + * \enum DmaSyncer::SyncType > + * \brief Read and/or write access via the CPU map > + * \var DmaSyncer::Read > + * \brief Indicates that the mapped dma-buf will be read by the client via the > + * CPU map > + * \var DmaSyncer::Write > + * \brief Indicates that the mapped dm-buf will be written by the client via the > + * CPU map > + * \var DmaSyncer::ReadWrite > + * \brief Indicates that the mapped dma-buf will be read and written by the > + * client via the CPU map > + */ > + > +/** > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) Is this line needed when the docstring is attached to the constructor? > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > + * \param[in] fd The dma-buf's file descriptor to synchronize > + * \param[in] type Read and/or write access via the CPU map > + */ > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > + : fd_(fd) > +{ > + switch (type) { > + case SyncType::Read: > + flags_ = DMA_BUF_SYNC_READ; > + break; > + case SyncType::Write: > + flags_ = DMA_BUF_SYNC_WRITE; > + break; > + case SyncType::ReadWrite: > + flags_ = DMA_BUF_SYNC_RW; > + break; > + } > + > + sync(DMA_BUF_SYNC_START); > +} > + > +DmaSyncer::~DmaSyncer() > +{ > + sync(DMA_BUF_SYNC_END); > +} > + > +void DmaSyncer::sync(uint64_t step) > +{ > + struct dma_buf_sync sync = { > + .flags = flags_ | step > + }; > + > + int ret; > + do { > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > + } while (ret && (errno == EINTR || errno == EAGAIN)); > + > + if (ret) { > + ret = errno; > + LOG(DmaBufAllocator, Error) > + << "Unable to sync dma fd: " << fd_.get() > + << ", err: " << strerror(ret) > + << ", flags: " << sync.flags; > + } > +} > + > } /* namespace libcamera */
Hi Milan, On Thu, Nov 21, 2024 at 5:23 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > Harvey Yang <chenghaoyang@chromium.org> writes: > > > To synchronize CPU access with mmap and hardware access on DMA buffers, > > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > > a helper class to allow users to sync buffers more easily. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > OK for me. > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > > 2 files changed, 96 insertions(+) > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > index d2a0a0d19..e4073f668 100644 > > --- a/include/libcamera/internal/dma_buf_allocator.h > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <libcamera/base/flags.h> > > +#include <libcamera/base/shared_fd.h> > > #include <libcamera/base/unique_fd.h> > > > > namespace libcamera { > > @@ -35,6 +36,26 @@ private: > > DmaBufAllocatorFlag type_; > > }; > > > > +class DmaSyncer final > > +{ > > +public: > > + enum class SyncType { > > + Read = 0, > > + Write, > > + ReadWrite, > > + }; > > + > > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > > + > > + ~DmaSyncer(); > > + > > +private: > > + void sync(uint64_t step); > > + > > + SharedFD fd_; > > + uint64_t flags_ = 0; > > +}; > > + > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > index be6efb89f..c1c2103d6 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > return allocFromHeap(name, size); > > } > > > > +/** > > + * \class DmaSyncer > > + * \brief Helper class for dma-buf's synchronization > > + * > > + * This class wraps a userspace dma-buf's synchronization process with an > > + * object's lifetime. > > + * > > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > > + * ISP. > > + */ > > + > > +/** > > + * \enum DmaSyncer::SyncType > > + * \brief Read and/or write access via the CPU map > > + * \var DmaSyncer::Read > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > + * CPU map > > + * \var DmaSyncer::Write > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > + * CPU map > > + * \var DmaSyncer::ReadWrite > > + * \brief Indicates that the mapped dma-buf will be read and written by the > > + * client via the CPU map > > + */ > > + > > +/** > > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > > Is this line needed when the docstring is attached to the constructor? No, sorry... I'll remove it in the next version. Let's wait for others' comments though to prevent the spam. BR, Harvey > > > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > > + * \param[in] fd The dma-buf's file descriptor to synchronize > > + * \param[in] type Read and/or write access via the CPU map > > + */ > > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > > + : fd_(fd) > > +{ > > + switch (type) { > > + case SyncType::Read: > > + flags_ = DMA_BUF_SYNC_READ; > > + break; > > + case SyncType::Write: > > + flags_ = DMA_BUF_SYNC_WRITE; > > + break; > > + case SyncType::ReadWrite: > > + flags_ = DMA_BUF_SYNC_RW; > > + break; > > + } > > + > > + sync(DMA_BUF_SYNC_START); > > +} > > + > > +DmaSyncer::~DmaSyncer() > > +{ > > + sync(DMA_BUF_SYNC_END); > > +} > > + > > +void DmaSyncer::sync(uint64_t step) > > +{ > > + struct dma_buf_sync sync = { > > + .flags = flags_ | step > > + }; > > + > > + int ret; > > + do { > > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > > + } while (ret && (errno == EINTR || errno == EAGAIN)); > > + > > + if (ret) { > > + ret = errno; > > + LOG(DmaBufAllocator, Error) > > + << "Unable to sync dma fd: " << fd_.get() > > + << ", err: " << strerror(ret) > > + << ", flags: " << sync.flags; > > + } > > +} > > + > > } /* namespace libcamera */ >
Quoting Harvey Yang (2024-11-21 05:51:31) > To synchronize CPU access with mmap and hardware access on DMA buffers, > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > a helper class to allow users to sync buffers more easily. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> I think I'm biased here from the previous review, but I think this looks better ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > 2 files changed, 96 insertions(+) > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > index d2a0a0d19..e4073f668 100644 > --- a/include/libcamera/internal/dma_buf_allocator.h > +++ b/include/libcamera/internal/dma_buf_allocator.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <libcamera/base/flags.h> > +#include <libcamera/base/shared_fd.h> > #include <libcamera/base/unique_fd.h> > > namespace libcamera { > @@ -35,6 +36,26 @@ private: > DmaBufAllocatorFlag type_; > }; > > +class DmaSyncer final > +{ > +public: > + enum class SyncType { > + Read = 0, > + Write, > + ReadWrite, > + }; > + > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > + > + ~DmaSyncer(); > + > +private: > + void sync(uint64_t step); > + > + SharedFD fd_; > + uint64_t flags_ = 0; > +}; > + > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > } /* namespace libcamera */ > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index be6efb89f..c1c2103d6 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > return allocFromHeap(name, size); > } > > +/** > + * \class DmaSyncer > + * \brief Helper class for dma-buf's synchronization > + * > + * This class wraps a userspace dma-buf's synchronization process with an > + * object's lifetime. > + * > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > + * ISP. > + */ > + > +/** > + * \enum DmaSyncer::SyncType > + * \brief Read and/or write access via the CPU map > + * \var DmaSyncer::Read > + * \brief Indicates that the mapped dma-buf will be read by the client via the > + * CPU map > + * \var DmaSyncer::Write > + * \brief Indicates that the mapped dm-buf will be written by the client via the > + * CPU map > + * \var DmaSyncer::ReadWrite > + * \brief Indicates that the mapped dma-buf will be read and written by the > + * client via the CPU map > + */ > + > +/** > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > + * \param[in] fd The dma-buf's file descriptor to synchronize > + * \param[in] type Read and/or write access via the CPU map > + */ > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > + : fd_(fd) > +{ > + switch (type) { > + case SyncType::Read: > + flags_ = DMA_BUF_SYNC_READ; > + break; > + case SyncType::Write: > + flags_ = DMA_BUF_SYNC_WRITE; > + break; > + case SyncType::ReadWrite: > + flags_ = DMA_BUF_SYNC_RW; > + break; > + } > + > + sync(DMA_BUF_SYNC_START); > +} > + > +DmaSyncer::~DmaSyncer() > +{ > + sync(DMA_BUF_SYNC_END); > +} > + > +void DmaSyncer::sync(uint64_t step) > +{ > + struct dma_buf_sync sync = { > + .flags = flags_ | step > + }; > + > + int ret; > + do { > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > + } while (ret && (errno == EINTR || errno == EAGAIN)); > + > + if (ret) { > + ret = errno; > + LOG(DmaBufAllocator, Error) > + << "Unable to sync dma fd: " << fd_.get() > + << ", err: " << strerror(ret) > + << ", flags: " << sync.flags; > + } > +} > + > } /* namespace libcamera */ > -- > 2.47.0.338.g60cca15819-goog >
Quoting Cheng-Hao Yang (2024-11-21 09:27:31) > Hi Milan, > > On Thu, Nov 21, 2024 at 5:23 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > > > Harvey Yang <chenghaoyang@chromium.org> writes: > > > > > To synchronize CPU access with mmap and hardware access on DMA buffers, > > > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > > > a helper class to allow users to sync buffers more easily. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > OK for me. > > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > > > --- > > > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > > > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > > > 2 files changed, 96 insertions(+) > > > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > > index d2a0a0d19..e4073f668 100644 > > > --- a/include/libcamera/internal/dma_buf_allocator.h > > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > > @@ -8,6 +8,7 @@ > > > #pragma once > > > > > > #include <libcamera/base/flags.h> > > > +#include <libcamera/base/shared_fd.h> > > > #include <libcamera/base/unique_fd.h> > > > > > > namespace libcamera { > > > @@ -35,6 +36,26 @@ private: > > > DmaBufAllocatorFlag type_; > > > }; > > > > > > +class DmaSyncer final > > > +{ > > > +public: > > > + enum class SyncType { > > > + Read = 0, > > > + Write, > > > + ReadWrite, > > > + }; > > > + > > > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > > > + > > > + ~DmaSyncer(); > > > + > > > +private: > > > + void sync(uint64_t step); > > > + > > > + SharedFD fd_; > > > + uint64_t flags_ = 0; > > > +}; > > > + > > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > > index be6efb89f..c1c2103d6 100644 > > > --- a/src/libcamera/dma_buf_allocator.cpp > > > +++ b/src/libcamera/dma_buf_allocator.cpp > > > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > > return allocFromHeap(name, size); > > > } > > > > > > +/** > > > + * \class DmaSyncer > > > + * \brief Helper class for dma-buf's synchronization > > > + * > > > + * This class wraps a userspace dma-buf's synchronization process with an > > > + * object's lifetime. > > > + * > > > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > > > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > > > + * ISP. > > > + */ > > > + > > > +/** > > > + * \enum DmaSyncer::SyncType > > > + * \brief Read and/or write access via the CPU map > > > + * \var DmaSyncer::Read > > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > > + * CPU map > > > + * \var DmaSyncer::Write > > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > > + * CPU map > > > + * \var DmaSyncer::ReadWrite > > > + * \brief Indicates that the mapped dma-buf will be read and written by the > > > + * client via the CPU map > > > + */ > > > + > > > +/** > > > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > > > > Is this line needed when the docstring is attached to the constructor? > > No, sorry... > I'll remove it in the next version. I do'nt think it breaks, but it could be redundant and a source of something that could bitrot so perhaps easier to remove the line. > > Let's wait for others' comments though to prevent the spam. If there's no other comments, it could be removed while applying. Lets give it a day or so to see. -- Kieran > > BR, > Harvey > > > > > > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > > > + * \param[in] fd The dma-buf's file descriptor to synchronize > > > + * \param[in] type Read and/or write access via the CPU map > > > + */ > > > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > > > + : fd_(fd) > > > +{ > > > + switch (type) { > > > + case SyncType::Read: > > > + flags_ = DMA_BUF_SYNC_READ; > > > + break; > > > + case SyncType::Write: > > > + flags_ = DMA_BUF_SYNC_WRITE; > > > + break; > > > + case SyncType::ReadWrite: > > > + flags_ = DMA_BUF_SYNC_RW; > > > + break; > > > + } > > > + > > > + sync(DMA_BUF_SYNC_START); > > > +} > > > + > > > +DmaSyncer::~DmaSyncer() > > > +{ > > > + sync(DMA_BUF_SYNC_END); > > > +} > > > + > > > +void DmaSyncer::sync(uint64_t step) > > > +{ > > > + struct dma_buf_sync sync = { > > > + .flags = flags_ | step > > > + }; > > > + > > > + int ret; > > > + do { > > > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > > > + } while (ret && (errno == EINTR || errno == EAGAIN)); > > > + > > > + if (ret) { > > > + ret = errno; > > > + LOG(DmaBufAllocator, Error) > > > + << "Unable to sync dma fd: " << fd_.get() > > > + << ", err: " << strerror(ret) > > > + << ", flags: " << sync.flags; > > > + } > > > +} > > > + > > > } /* namespace libcamera */ > > > > > -- > BR, > Harvey Yang
Hi Kieran, On Thu, Nov 21, 2024 at 8:20 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Cheng-Hao Yang (2024-11-21 09:27:31) > > Hi Milan, > > > > On Thu, Nov 21, 2024 at 5:23 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > > > > > Harvey Yang <chenghaoyang@chromium.org> writes: > > > > > > > To synchronize CPU access with mmap and hardware access on DMA buffers, > > > > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > > > > a helper class to allow users to sync buffers more easily. > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > OK for me. > > > > > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > > > > > --- > > > > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > > > > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > > > > 2 files changed, 96 insertions(+) > > > > > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > > > index d2a0a0d19..e4073f668 100644 > > > > --- a/include/libcamera/internal/dma_buf_allocator.h > > > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > > > @@ -8,6 +8,7 @@ > > > > #pragma once > > > > > > > > #include <libcamera/base/flags.h> > > > > +#include <libcamera/base/shared_fd.h> > > > > #include <libcamera/base/unique_fd.h> > > > > > > > > namespace libcamera { > > > > @@ -35,6 +36,26 @@ private: > > > > DmaBufAllocatorFlag type_; > > > > }; > > > > > > > > +class DmaSyncer final > > > > +{ > > > > +public: > > > > + enum class SyncType { > > > > + Read = 0, > > > > + Write, > > > > + ReadWrite, > > > > + }; > > > > + > > > > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > > > > + > > > > + ~DmaSyncer(); > > > > + > > > > +private: > > > > + void sync(uint64_t step); > > > > + > > > > + SharedFD fd_; > > > > + uint64_t flags_ = 0; > > > > +}; > > > > + > > > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > > > > > > > } /* namespace libcamera */ > > > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > > > index be6efb89f..c1c2103d6 100644 > > > > --- a/src/libcamera/dma_buf_allocator.cpp > > > > +++ b/src/libcamera/dma_buf_allocator.cpp > > > > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > > > return allocFromHeap(name, size); > > > > } > > > > > > > > +/** > > > > + * \class DmaSyncer > > > > + * \brief Helper class for dma-buf's synchronization > > > > + * > > > > + * This class wraps a userspace dma-buf's synchronization process with an > > > > + * object's lifetime. > > > > + * > > > > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > > > > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > > > > + * ISP. > > > > + */ > > > > + > > > > +/** > > > > + * \enum DmaSyncer::SyncType > > > > + * \brief Read and/or write access via the CPU map > > > > + * \var DmaSyncer::Read > > > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > > > + * CPU map > > > > + * \var DmaSyncer::Write > > > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > > > + * CPU map > > > > + * \var DmaSyncer::ReadWrite > > > > + * \brief Indicates that the mapped dma-buf will be read and written by the > > > > + * client via the CPU map > > > > + */ > > > > + > > > > +/** > > > > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > > > > > > Is this line needed when the docstring is attached to the constructor? > > > > No, sorry... > > I'll remove it in the next version. > > I do'nt think it breaks, but it could be redundant and a source of > something that could bitrot so perhaps easier to remove the line. > > > > > Let's wait for others' comments though to prevent the spam. > > If there's no other comments, it could be removed while applying. Lets > give it a day or so to see. Yeah, please help remove it while applying to prevent the spam, if there's no other comemnts. Thanks! BR, Harvey > > -- > Kieran > > > > > BR, > > Harvey > > > > > > > > > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > > > > + * \param[in] fd The dma-buf's file descriptor to synchronize > > > > + * \param[in] type Read and/or write access via the CPU map > > > > + */ > > > > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > > > > + : fd_(fd) > > > > +{ > > > > + switch (type) { > > > > + case SyncType::Read: > > > > + flags_ = DMA_BUF_SYNC_READ; > > > > + break; > > > > + case SyncType::Write: > > > > + flags_ = DMA_BUF_SYNC_WRITE; > > > > + break; > > > > + case SyncType::ReadWrite: > > > > + flags_ = DMA_BUF_SYNC_RW; > > > > + break; > > > > + } > > > > + > > > > + sync(DMA_BUF_SYNC_START); > > > > +} > > > > + > > > > +DmaSyncer::~DmaSyncer() > > > > +{ > > > > + sync(DMA_BUF_SYNC_END); > > > > +} > > > > + > > > > +void DmaSyncer::sync(uint64_t step) > > > > +{ > > > > + struct dma_buf_sync sync = { > > > > + .flags = flags_ | step > > > > + }; > > > > + > > > > + int ret; > > > > + do { > > > > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > > > > + } while (ret && (errno == EINTR || errno == EAGAIN)); > > > > + > > > > + if (ret) { > > > > + ret = errno; > > > > + LOG(DmaBufAllocator, Error) > > > > + << "Unable to sync dma fd: " << fd_.get() > > > > + << ", err: " << strerror(ret) > > > > + << ", flags: " << sync.flags; > > > > + } > > > > +} > > > > + > > > > } /* namespace libcamera */ > > > > > > > > > -- > > BR, > > Harvey Yang
On Thu, Nov 21, 2024 at 05:51:31AM +0000, Harvey Yang wrote: > To synchronize CPU access with mmap and hardware access on DMA buffers, > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > a helper class to allow users to sync buffers more easily. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > 2 files changed, 96 insertions(+) > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > index d2a0a0d19..e4073f668 100644 > --- a/include/libcamera/internal/dma_buf_allocator.h > +++ b/include/libcamera/internal/dma_buf_allocator.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <libcamera/base/flags.h> > +#include <libcamera/base/shared_fd.h> > #include <libcamera/base/unique_fd.h> > > namespace libcamera { > @@ -35,6 +36,26 @@ private: > DmaBufAllocatorFlag type_; > }; > > +class DmaSyncer final > +{ > +public: > + enum class SyncType { > + Read = 0, > + Write, > + ReadWrite, > + }; > + > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); Why is this calss copyable ? > + > + ~DmaSyncer(); > + > +private: > + void sync(uint64_t step); > + > + SharedFD fd_; > + uint64_t flags_ = 0; > +}; > + > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > } /* namespace libcamera */ > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index be6efb89f..c1c2103d6 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > return allocFromHeap(name, size); > } > > +/** > + * \class DmaSyncer > + * \brief Helper class for dma-buf's synchronization > + * > + * This class wraps a userspace dma-buf's synchronization process with an > + * object's lifetime. > + * > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > + * ISP. > + */ > + > +/** > + * \enum DmaSyncer::SyncType > + * \brief Read and/or write access via the CPU map > + * \var DmaSyncer::Read > + * \brief Indicates that the mapped dma-buf will be read by the client via the > + * CPU map > + * \var DmaSyncer::Write > + * \brief Indicates that the mapped dm-buf will be written by the client via the > + * CPU map > + * \var DmaSyncer::ReadWrite > + * \brief Indicates that the mapped dma-buf will be read and written by the > + * client via the CPU map > + */ > + > +/** > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > + * \param[in] fd The dma-buf's file descriptor to synchronize > + * \param[in] type Read and/or write access via the CPU map > + */ > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > + : fd_(fd) > +{ > + switch (type) { > + case SyncType::Read: > + flags_ = DMA_BUF_SYNC_READ; > + break; > + case SyncType::Write: > + flags_ = DMA_BUF_SYNC_WRITE; > + break; > + case SyncType::ReadWrite: > + flags_ = DMA_BUF_SYNC_RW; > + break; > + } > + > + sync(DMA_BUF_SYNC_START); > +} > + > +DmaSyncer::~DmaSyncer() > +{ > + sync(DMA_BUF_SYNC_END); > +} > + > +void DmaSyncer::sync(uint64_t step) > +{ > + struct dma_buf_sync sync = { > + .flags = flags_ | step > + }; > + > + int ret; > + do { > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > + } while (ret && (errno == EINTR || errno == EAGAIN)); > + > + if (ret) { > + ret = errno; > + LOG(DmaBufAllocator, Error) > + << "Unable to sync dma fd: " << fd_.get() > + << ", err: " << strerror(ret) > + << ", flags: " << sync.flags; > + } > +} > + > } /* namespace libcamera */
Hi Laurent, On Fri, Nov 29, 2024 at 3:49 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Nov 21, 2024 at 05:51:31AM +0000, Harvey Yang wrote: > > To synchronize CPU access with mmap and hardware access on DMA buffers, > > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > > a helper class to allow users to sync buffers more easily. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > > 2 files changed, 96 insertions(+) > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > index d2a0a0d19..e4073f668 100644 > > --- a/include/libcamera/internal/dma_buf_allocator.h > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > @@ -8,6 +8,7 @@ > > #pragma once > > > > #include <libcamera/base/flags.h> > > +#include <libcamera/base/shared_fd.h> > > #include <libcamera/base/unique_fd.h> > > > > namespace libcamera { > > @@ -35,6 +36,26 @@ private: > > DmaBufAllocatorFlag type_; > > }; > > > > +class DmaSyncer final > > +{ > > +public: > > + enum class SyncType { > > + Read = 0, > > + Write, > > + ReadWrite, > > + }; > > + > > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > > Why is this calss copyable ? Thanks for the catch. Uploaded patch `DmaBufAllocator: Make DmaSyncer non-copyable`. Please take a look. BR, Harvey > > > + > > + ~DmaSyncer(); > > + > > +private: > > + void sync(uint64_t step); > > + > > + SharedFD fd_; > > + uint64_t flags_ = 0; > > +}; > > + > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > index be6efb89f..c1c2103d6 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > return allocFromHeap(name, size); > > } > > > > +/** > > + * \class DmaSyncer > > + * \brief Helper class for dma-buf's synchronization > > + * > > + * This class wraps a userspace dma-buf's synchronization process with an > > + * object's lifetime. > > + * > > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > > + * ISP. > > + */ > > + > > +/** > > + * \enum DmaSyncer::SyncType > > + * \brief Read and/or write access via the CPU map > > + * \var DmaSyncer::Read > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > + * CPU map > > + * \var DmaSyncer::Write > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > + * CPU map > > + * \var DmaSyncer::ReadWrite > > + * \brief Indicates that the mapped dma-buf will be read and written by the > > + * client via the CPU map > > + */ > > + > > +/** > > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > > + * \param[in] fd The dma-buf's file descriptor to synchronize > > + * \param[in] type Read and/or write access via the CPU map > > + */ > > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > > + : fd_(fd) > > +{ > > + switch (type) { > > + case SyncType::Read: > > + flags_ = DMA_BUF_SYNC_READ; > > + break; > > + case SyncType::Write: > > + flags_ = DMA_BUF_SYNC_WRITE; > > + break; > > + case SyncType::ReadWrite: > > + flags_ = DMA_BUF_SYNC_RW; > > + break; > > + } > > + > > + sync(DMA_BUF_SYNC_START); > > +} > > + > > +DmaSyncer::~DmaSyncer() > > +{ > > + sync(DMA_BUF_SYNC_END); > > +} > > + > > +void DmaSyncer::sync(uint64_t step) > > +{ > > + struct dma_buf_sync sync = { > > + .flags = flags_ | step > > + }; > > + > > + int ret; > > + do { > > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > > + } while (ret && (errno == EINTR || errno == EAGAIN)); > > + > > + if (ret) { > > + ret = errno; > > + LOG(DmaBufAllocator, Error) > > + << "Unable to sync dma fd: " << fd_.get() > > + << ", err: " << strerror(ret) > > + << ", flags: " << sync.flags; > > + } > > +} > > + > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
On Sat, Nov 30, 2024 at 02:56:27AM +0800, Cheng-Hao Yang wrote: > On Fri, Nov 29, 2024 at 3:49 AM Laurent Pinchart wrote: > > On Thu, Nov 21, 2024 at 05:51:31AM +0000, Harvey Yang wrote: > > > To synchronize CPU access with mmap and hardware access on DMA buffers, > > > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and > > > a helper class to allow users to sync buffers more easily. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > .../libcamera/internal/dma_buf_allocator.h | 21 ++++++ > > > src/libcamera/dma_buf_allocator.cpp | 75 +++++++++++++++++++ > > > 2 files changed, 96 insertions(+) > > > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > > index d2a0a0d19..e4073f668 100644 > > > --- a/include/libcamera/internal/dma_buf_allocator.h > > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > > @@ -8,6 +8,7 @@ > > > #pragma once > > > > > > #include <libcamera/base/flags.h> > > > +#include <libcamera/base/shared_fd.h> > > > #include <libcamera/base/unique_fd.h> > > > > > > namespace libcamera { > > > @@ -35,6 +36,26 @@ private: > > > DmaBufAllocatorFlag type_; > > > }; > > > > > > +class DmaSyncer final > > > +{ > > > +public: > > > + enum class SyncType { > > > + Read = 0, > > > + Write, > > > + ReadWrite, > > > + }; > > > + > > > + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); > > > > Why is this calss copyable ? > > Thanks for the catch. > Uploaded patch `DmaBufAllocator: Make DmaSyncer non-copyable`. > Please take a look. Thank you, I'll review that. > > > + > > > + ~DmaSyncer(); > > > + > > > +private: > > > + void sync(uint64_t step); > > > + > > > + SharedFD fd_; > > > + uint64_t flags_ = 0; > > > +}; > > > + > > > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > > > index be6efb89f..c1c2103d6 100644 > > > --- a/src/libcamera/dma_buf_allocator.cpp > > > +++ b/src/libcamera/dma_buf_allocator.cpp > > > @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) > > > return allocFromHeap(name, size); > > > } > > > > > > +/** > > > + * \class DmaSyncer > > > + * \brief Helper class for dma-buf's synchronization > > > + * > > > + * This class wraps a userspace dma-buf's synchronization process with an > > > + * object's lifetime. > > > + * > > > + * It's used when the user needs to access a dma-buf with CPU, mostly mapped > > > + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and > > > + * ISP. > > > + */ > > > + > > > +/** > > > + * \enum DmaSyncer::SyncType > > > + * \brief Read and/or write access via the CPU map > > > + * \var DmaSyncer::Read > > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > > + * CPU map > > > + * \var DmaSyncer::Write > > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > > + * CPU map > > > + * \var DmaSyncer::ReadWrite > > > + * \brief Indicates that the mapped dma-buf will be read and written by the > > > + * client via the CPU map > > > + */ > > > + > > > +/** > > > + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) > > > + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type > > > + * \param[in] fd The dma-buf's file descriptor to synchronize > > > + * \param[in] type Read and/or write access via the CPU map > > > + */ > > > +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) > > > + : fd_(fd) > > > +{ > > > + switch (type) { > > > + case SyncType::Read: > > > + flags_ = DMA_BUF_SYNC_READ; > > > + break; > > > + case SyncType::Write: > > > + flags_ = DMA_BUF_SYNC_WRITE; > > > + break; > > > + case SyncType::ReadWrite: > > > + flags_ = DMA_BUF_SYNC_RW; > > > + break; > > > + } > > > + > > > + sync(DMA_BUF_SYNC_START); > > > +} > > > + > > > +DmaSyncer::~DmaSyncer() > > > +{ > > > + sync(DMA_BUF_SYNC_END); > > > +} > > > + > > > +void DmaSyncer::sync(uint64_t step) > > > +{ > > > + struct dma_buf_sync sync = { > > > + .flags = flags_ | step > > > + }; > > > + > > > + int ret; > > > + do { > > > + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); > > > + } while (ret && (errno == EINTR || errno == EAGAIN)); > > > + > > > + if (ret) { > > > + ret = errno; > > > + LOG(DmaBufAllocator, Error) > > > + << "Unable to sync dma fd: " << fd_.get() > > > + << ", err: " << strerror(ret) > > > + << ", flags: " << sync.flags; > > > + } > > > +} > > > + > > > } /* namespace libcamera */
diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h index d2a0a0d19..e4073f668 100644 --- a/include/libcamera/internal/dma_buf_allocator.h +++ b/include/libcamera/internal/dma_buf_allocator.h @@ -8,6 +8,7 @@ #pragma once #include <libcamera/base/flags.h> +#include <libcamera/base/shared_fd.h> #include <libcamera/base/unique_fd.h> namespace libcamera { @@ -35,6 +36,26 @@ private: DmaBufAllocatorFlag type_; }; +class DmaSyncer final +{ +public: + enum class SyncType { + Read = 0, + Write, + ReadWrite, + }; + + explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite); + + ~DmaSyncer(); + +private: + void sync(uint64_t step); + + SharedFD fd_; + uint64_t flags_ = 0; +}; + LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag) } /* namespace libcamera */ diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp index be6efb89f..c1c2103d6 100644 --- a/src/libcamera/dma_buf_allocator.cpp +++ b/src/libcamera/dma_buf_allocator.cpp @@ -205,4 +205,79 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size) return allocFromHeap(name, size); } +/** + * \class DmaSyncer + * \brief Helper class for dma-buf's synchronization + * + * This class wraps a userspace dma-buf's synchronization process with an + * object's lifetime. + * + * It's used when the user needs to access a dma-buf with CPU, mostly mapped + * with MappedFrameBuffer, so that the buffer is synchronized between CPU and + * ISP. + */ + +/** + * \enum DmaSyncer::SyncType + * \brief Read and/or write access via the CPU map + * \var DmaSyncer::Read + * \brief Indicates that the mapped dma-buf will be read by the client via the + * CPU map + * \var DmaSyncer::Write + * \brief Indicates that the mapped dm-buf will be written by the client via the + * CPU map + * \var DmaSyncer::ReadWrite + * \brief Indicates that the mapped dma-buf will be read and written by the + * client via the CPU map + */ + +/** + * \fn DmaSyncer::DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite) + * \brief Construct a DmaSyncer with a dma-buf's fd and the access type + * \param[in] fd The dma-buf's file descriptor to synchronize + * \param[in] type Read and/or write access via the CPU map + */ +DmaSyncer::DmaSyncer(SharedFD fd, SyncType type) + : fd_(fd) +{ + switch (type) { + case SyncType::Read: + flags_ = DMA_BUF_SYNC_READ; + break; + case SyncType::Write: + flags_ = DMA_BUF_SYNC_WRITE; + break; + case SyncType::ReadWrite: + flags_ = DMA_BUF_SYNC_RW; + break; + } + + sync(DMA_BUF_SYNC_START); +} + +DmaSyncer::~DmaSyncer() +{ + sync(DMA_BUF_SYNC_END); +} + +void DmaSyncer::sync(uint64_t step) +{ + struct dma_buf_sync sync = { + .flags = flags_ | step + }; + + int ret; + do { + ret = ioctl(fd_.get(), DMA_BUF_IOCTL_SYNC, &sync); + } while (ret && (errno == EINTR || errno == EAGAIN)); + + if (ret) { + ret = errno; + LOG(DmaBufAllocator, Error) + << "Unable to sync dma fd: " << fd_.get() + << ", err: " << strerror(ret) + << ", flags: " << sync.flags; + } +} + } /* namespace libcamera */