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

[Cheng-Hao Yang]

Hi Kieran,

On Thu, Nov 21, 2024 at 8:20 PM Kieran Bingham
<kieran.bingham at 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 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.

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