| Message ID | 20241112100051.4071443-2-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, thank you for the patch. 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> > --- > .../libcamera/internal/dma_buf_allocator.h | 33 +++++++ > src/libcamera/dma_buf_allocator.cpp | 88 +++++++++++++++++++ > 2 files changed, 121 insertions(+) > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > index d2a0a0d19..2b9b99678 100644 > --- a/include/libcamera/internal/dma_buf_allocator.h > +++ b/include/libcamera/internal/dma_buf_allocator.h > @@ -23,6 +23,19 @@ public: > > using DmaBufAllocatorFlags = Flags<DmaBufAllocatorFlag>; > > + enum class SyncStep { > + Start = 0, > + End > + }; > + > + enum class SyncType { > + Read = 0, > + Write, > + ReadWrite, > + }; > + > + static void sync(int fd, SyncStep step, SyncType type); > + > DmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap); > ~DmaBufAllocator(); > bool isValid() const { return providerHandle_.isValid(); } > @@ -35,6 +48,26 @@ private: > DmaBufAllocatorFlag type_; > }; > > +class DmaSyncer final > +{ > +public: > + explicit DmaSyncer(int fd, > + DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite) > + : fd_(fd), type_(type) > + { > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_); > + } > + > + ~DmaSyncer() > + { > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_); > + } > + > +private: > + int fd_; > + DmaBufAllocator::SyncType type_; > +}; > + > 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..3ae98c168 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -78,6 +78,79 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator) > * \brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values > */ > > +/** > + * \enum DmaBufAllocator::SyncStep > + * \brief Either start or end of the synchronization > + * \var DmaBufAllocator::Start > + * \brief Indicates the start of a map access session > + * \var DmaBufAllocator::End > + * \brief Indicates the end of a map access session > + */ > + > +/** > + * \enum DmaBufAllocator::SyncType > + * \brief Read and/or write access via the CPU map > + * \var DmaBufAllocator::Read > + * \brief Indicates that the mapped dma-buf will be read by the client via the > + * CPU map > + * \var DmaBufAllocator::Write > + * \brief Indicates that the mapped dm-buf will be written by the client via the > + * CPU map > + * \var DmaBufAllocator::ReadWrite > + * \brief Indicates that the mapped dma-buf will be rad andd written by the s/rad andd/read and/ > + * client via the CPU map > + */ > + > +/** > + * \brief Synchronize CPU access and hardware access > + * \param[in] fd The dma-buf's file descriptor to be synchronized > + * \param[in] step Either start or end of the synchronization > + * \param[in] type Read and/or write access during CPU mmap > + * > + * The client is expected to call this function with > + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the > + * start and end of buffer access respectively. > + */ > +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type) > +{ > + uint64_t flags = 0; > + switch (step) { > + case DmaBufAllocator::SyncStep::Start: > + flags = DMA_BUF_SYNC_START; > + break; > + case DmaBufAllocator::SyncStep::End: > + flags = DMA_BUF_SYNC_END; > + break; > + } > + > + switch (type) { > + case DmaBufAllocator::SyncType::Read: > + flags = flags | DMA_BUF_SYNC_READ; > + break; > + case DmaBufAllocator::SyncType::Write: > + flags = flags | DMA_BUF_SYNC_WRITE; > + break; > + case DmaBufAllocator::SyncType::ReadWrite: > + flags = flags | DMA_BUF_SYNC_RW; > + break; > + } > + > + struct dma_buf_sync sync = { > + .flags = flags > + }; > + > + int ret; > + do { > + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync); > + } while (!ret && (errno == EINTR || errno == EAGAIN)); Shouldn't it be `ret' rather than `!ret'? > + > + if (ret) { > + LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd > + << ", step: " << static_cast<int>(step) > + << ", type: " << static_cast<int>(type); Wouldn't it be useful to log errno? > + } > +} > + > /** > * \brief Construct a DmaBufAllocator of a given type > * \param[in] type The type(s) of the dma-buf providers to allocate from > @@ -205,4 +278,19 @@ 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. > + */ > + > +/** > + * \fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::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 > + */ > + > } /* namespace libcamera */
Hi Milan, On Tue, Nov 12, 2024 at 10:00 PM Milan Zamazal <mzamazal@redhat.com> wrote: > > Hi Harvey, > > thank you for the patch. > > 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> > > --- > > .../libcamera/internal/dma_buf_allocator.h | 33 +++++++ > > src/libcamera/dma_buf_allocator.cpp | 88 +++++++++++++++++++ > > 2 files changed, 121 insertions(+) > > > > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h > > index d2a0a0d19..2b9b99678 100644 > > --- a/include/libcamera/internal/dma_buf_allocator.h > > +++ b/include/libcamera/internal/dma_buf_allocator.h > > @@ -23,6 +23,19 @@ public: > > > > using DmaBufAllocatorFlags = Flags<DmaBufAllocatorFlag>; > > > > + enum class SyncStep { > > + Start = 0, > > + End > > + }; > > + > > + enum class SyncType { > > + Read = 0, > > + Write, > > + ReadWrite, > > + }; > > + > > + static void sync(int fd, SyncStep step, SyncType type); > > + > > DmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap); > > ~DmaBufAllocator(); > > bool isValid() const { return providerHandle_.isValid(); } > > @@ -35,6 +48,26 @@ private: > > DmaBufAllocatorFlag type_; > > }; > > > > +class DmaSyncer final > > +{ > > +public: > > + explicit DmaSyncer(int fd, > > + DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite) > > + : fd_(fd), type_(type) > > + { > > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_); > > + } > > + > > + ~DmaSyncer() > > + { > > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_); > > + } > > + > > +private: > > + int fd_; > > + DmaBufAllocator::SyncType type_; > > +}; > > + > > 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..3ae98c168 100644 > > --- a/src/libcamera/dma_buf_allocator.cpp > > +++ b/src/libcamera/dma_buf_allocator.cpp > > @@ -78,6 +78,79 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator) > > * \brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values > > */ > > > > +/** > > + * \enum DmaBufAllocator::SyncStep > > + * \brief Either start or end of the synchronization > > + * \var DmaBufAllocator::Start > > + * \brief Indicates the start of a map access session > > + * \var DmaBufAllocator::End > > + * \brief Indicates the end of a map access session > > + */ > > + > > +/** > > + * \enum DmaBufAllocator::SyncType > > + * \brief Read and/or write access via the CPU map > > + * \var DmaBufAllocator::Read > > + * \brief Indicates that the mapped dma-buf will be read by the client via the > > + * CPU map > > + * \var DmaBufAllocator::Write > > + * \brief Indicates that the mapped dm-buf will be written by the client via the > > + * CPU map > > + * \var DmaBufAllocator::ReadWrite > > + * \brief Indicates that the mapped dma-buf will be rad andd written by the > > s/rad andd/read and/ Done > > > + * client via the CPU map > > + */ > > + > > +/** > > + * \brief Synchronize CPU access and hardware access > > + * \param[in] fd The dma-buf's file descriptor to be synchronized > > + * \param[in] step Either start or end of the synchronization > > + * \param[in] type Read and/or write access during CPU mmap > > + * > > + * The client is expected to call this function with > > + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the > > + * start and end of buffer access respectively. > > + */ > > +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type) > > +{ > > + uint64_t flags = 0; > > + switch (step) { > > + case DmaBufAllocator::SyncStep::Start: > > + flags = DMA_BUF_SYNC_START; > > + break; > > + case DmaBufAllocator::SyncStep::End: > > + flags = DMA_BUF_SYNC_END; > > + break; > > + } > > + > > + switch (type) { > > + case DmaBufAllocator::SyncType::Read: > > + flags = flags | DMA_BUF_SYNC_READ; > > + break; > > + case DmaBufAllocator::SyncType::Write: > > + flags = flags | DMA_BUF_SYNC_WRITE; > > + break; > > + case DmaBufAllocator::SyncType::ReadWrite: > > + flags = flags | DMA_BUF_SYNC_RW; > > + break; > > + } > > + > > + struct dma_buf_sync sync = { > > + .flags = flags > > + }; > > + > > + int ret; > > + do { > > + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync); > > + } while (!ret && (errno == EINTR || errno == EAGAIN)); > > Shouldn't it be `ret' rather than `!ret'? Ah right! Thanks for the catch! > > > + > > + if (ret) { > > + LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd > > + << ", step: " << static_cast<int>(step) > > + << ", type: " << static_cast<int>(type); > > Wouldn't it be useful to log errno? Done BR, Harvey > > > + } > > +} > > + > > /** > > * \brief Construct a DmaBufAllocator of a given type > > * \param[in] type The type(s) of the dma-buf providers to allocate from > > @@ -205,4 +278,19 @@ 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. > > + */ > > + > > +/** > > + * \fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::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 > > + */ > > + > > } /* namespace libcamera */ >
diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h index d2a0a0d19..2b9b99678 100644 --- a/include/libcamera/internal/dma_buf_allocator.h +++ b/include/libcamera/internal/dma_buf_allocator.h @@ -23,6 +23,19 @@ public: using DmaBufAllocatorFlags = Flags<DmaBufAllocatorFlag>; + enum class SyncStep { + Start = 0, + End + }; + + enum class SyncType { + Read = 0, + Write, + ReadWrite, + }; + + static void sync(int fd, SyncStep step, SyncType type); + DmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap); ~DmaBufAllocator(); bool isValid() const { return providerHandle_.isValid(); } @@ -35,6 +48,26 @@ private: DmaBufAllocatorFlag type_; }; +class DmaSyncer final +{ +public: + explicit DmaSyncer(int fd, + DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite) + : fd_(fd), type_(type) + { + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_); + } + + ~DmaSyncer() + { + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_); + } + +private: + int fd_; + DmaBufAllocator::SyncType type_; +}; + 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..3ae98c168 100644 --- a/src/libcamera/dma_buf_allocator.cpp +++ b/src/libcamera/dma_buf_allocator.cpp @@ -78,6 +78,79 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator) * \brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values */ +/** + * \enum DmaBufAllocator::SyncStep + * \brief Either start or end of the synchronization + * \var DmaBufAllocator::Start + * \brief Indicates the start of a map access session + * \var DmaBufAllocator::End + * \brief Indicates the end of a map access session + */ + +/** + * \enum DmaBufAllocator::SyncType + * \brief Read and/or write access via the CPU map + * \var DmaBufAllocator::Read + * \brief Indicates that the mapped dma-buf will be read by the client via the + * CPU map + * \var DmaBufAllocator::Write + * \brief Indicates that the mapped dm-buf will be written by the client via the + * CPU map + * \var DmaBufAllocator::ReadWrite + * \brief Indicates that the mapped dma-buf will be rad andd written by the + * client via the CPU map + */ + +/** + * \brief Synchronize CPU access and hardware access + * \param[in] fd The dma-buf's file descriptor to be synchronized + * \param[in] step Either start or end of the synchronization + * \param[in] type Read and/or write access during CPU mmap + * + * The client is expected to call this function with + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the + * start and end of buffer access respectively. + */ +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type) +{ + uint64_t flags = 0; + switch (step) { + case DmaBufAllocator::SyncStep::Start: + flags = DMA_BUF_SYNC_START; + break; + case DmaBufAllocator::SyncStep::End: + flags = DMA_BUF_SYNC_END; + break; + } + + switch (type) { + case DmaBufAllocator::SyncType::Read: + flags = flags | DMA_BUF_SYNC_READ; + break; + case DmaBufAllocator::SyncType::Write: + flags = flags | DMA_BUF_SYNC_WRITE; + break; + case DmaBufAllocator::SyncType::ReadWrite: + flags = flags | DMA_BUF_SYNC_RW; + break; + } + + struct dma_buf_sync sync = { + .flags = flags + }; + + int ret; + do { + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync); + } while (!ret && (errno == EINTR || errno == EAGAIN)); + + if (ret) { + LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd + << ", step: " << static_cast<int>(step) + << ", type: " << static_cast<int>(type); + } +} + /** * \brief Construct a DmaBufAllocator of a given type * \param[in] type The type(s) of the dma-buf providers to allocate from @@ -205,4 +278,19 @@ 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. + */ + +/** + * \fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::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 + */ + } /* namespace libcamera */