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

[Kieran Bingham]

Quoting Cheng-Hao Yang (2024-11-21 09:27:31)
> 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.

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