[PATCH v3 1/2] DmaBufAllocator: Add Dma Buffer synchronization function & helper class

[Cheng-Hao Yang]

Hi Milan,

On Thu, Nov 21, 2024 at 5:23 PM Milan Zamazal <mzamazal at redhat.com> wrote:
>
> Harvey Yang <chenghaoyang at 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 at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
>
> OK for me.
>
> Reviewed-by: Milan Zamazal <mzamazal at 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 */
>


-- 
BR,
Harvey Yang