[v2,1/2] DmaBufAllocator: Add Dma Buffer synchronization function & helper class

21879 diff mbox series

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           | 90 +++++++++++++++++++
 2 files changed, 123 insertions(+)

Hi Harvey,

thank you for the update, it's fine from my side.

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>

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  .../libcamera/internal/dma_buf_allocator.h    | 33 +++++++
>  src/libcamera/dma_buf_allocator.cpp           | 90 +++++++++++++++++++
>  2 files changed, 123 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..f0f068cc1 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -78,6 +78,81 @@ 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 read and 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) {
> +		ret = errno;
> +		LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd
> +					    << ", err: " << strerror(ret)
> +					    << ", 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 +280,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 */

Quoting Harvey Yang (2024-11-13 05:54:32)
> 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.
> 

I think the idea of scoped dma buffer control handling sounds useful,
but some comments below on how I would simplify this
class/implementation. But beyond simplfying ... I am not sure if
DmaBufAllocator is the right place for this.

The documentation you've added talks about a 'map session' - and the
intent is to ensure a mapped access is correctly managed for any DMA buf
interaction when accessed by the CPU.

The usage in 2/2 also seems to show it's more directly associated with a
FrameBuffer (or a mapped frame buffer?) rather than the allocator ...

In MappedFrameBuffer - the user has also already specified with the
MapFlag if this is a Read,Write, or ReadWrite operation ...

Is the current user only using this associated with a MappedFrameBuffer?
Do you foresee other users that would need to use DmaSyncer that
wouldn't be tieing it to a MappedFrameBuffer?


I've added the 'simple DmaSyncer' after I'd added other review comments,
so it might look out of place if you read straight down.



> 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           | 90 +++++++++++++++++++
>  2 files changed, 123 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);

Why is sync() here as a static? Maybe it should be a function inside the
DmaSyncer? Then it would have direct access to the fd_ that's stored in
that class?

> +
>         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_;

Should this be a SharedFD ? 

The usage in patch 2/2 is coming from a FrameBuffer::Plane fd, so I
think we should just store that here if we need to store an fd_ ?

> +       DmaBufAllocator::SyncType type_;

I also wonder if SyncType should be defined in the DmaSyncer class
instead of in the DmaBufAllocator class too!

> +};
> +
>  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..f0f068cc1 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -78,6 +78,81 @@ 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

I think if the implementation moves into the DmaSyncer class, the 'step'
becomes a private implementation detail, as sync() is only called by the
constructor and destructor.

I think at that point you could remove the 'Step' and just pass
DMA_BUF_SYNC_START, or DMA_BUF_SYNC_END to sync() as the starting
initialisation of flags.

i.e.

DmaSyncer::sync(SyncType type, uint64_t flags)
{
	switch(type) {
	case DmaBufAllocator::SyncType::Read:
		flags |= DMA_BUF_SYNC_READ;
		break;
	}

	...
	...
}

In fact, now I see that - the type is then processed twice - so actually
it should do that in the constructor - store the flags and then use that
and this all gets shorter...


The following is not compiled... please expect bugs...

class DmaSyncer final
{
public:
	enum class SyncType {
		Read = 0,
		Write,
		ReadWrite,
	};

	explicit DmaSyncer(SharedFD fd, SyncType type = ReadWrite)
		: 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()
	{
		sync(DMA_BUF_SYNC_END);
	}

private:
	uint64_t flags = 0;

	void sync(uint64_t step)
	{
		struct dma_buf_sync sync = {
			.flags = flags_ | step
		};

		int ret;
		do {
			ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);
		} while (ret && (errno == EINTR || errno == EAGAIN));

		if (ret) {
			ret = errno;
			LOG(DmaBufAllocator, Error)
				<< "Unable to sync dma fd: " << fd
				<< ", err: " << strerror(ret)
				<< ", flags: " << sync.flags;
		}
	}
}


Depending on who would actually call this - and if they are already
expected to have full knowledge of the DMA_BUF_SYNC_{READ,WRITE,RW}
types - even that switch case could be removed from the constructor..
but it depends whether that's helpful/preferred to keep as a class enum
or not.


> + */
> +
> +/**
> + * \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 read and 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.

I think if this goes into the DmaSyncer class this method would become
private, and you won't need to document this. But some of the detail
might be suited to detail in the DmaSyncer class level documentation.

> + */
> +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;

	flags |= DMA_BUF_SYNC_READ;

same below.


> +               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) {
> +               ret = errno;
> +               LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd
> +                                           << ", err: " << strerror(ret)
> +                                           << ", 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 +280,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.

Earlier you've called it a 'map session'. I think this is where the
documentation needs detail about why someone should use this and what it
solves.

Talking about a map session makes me think this should be associated in
some form with a MappedFrameBuffer more than the DmaBufAllocator ?



> + */
> +
> +/**
> + * \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 */
> -- 
> 2.47.0.277.g8800431eea-goog
>

Hi Kieran,

On Wed, Nov 20, 2024 at 7:20 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-11-13 05:54:32)
> > 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.
> >
>
> I think the idea of scoped dma buffer control handling sounds useful,
> but some comments below on how I would simplify this
> class/implementation. But beyond simplfying ... I am not sure if
> DmaBufAllocator is the right place for this.
>
> The documentation you've added talks about a 'map session' - and the
> intent is to ensure a mapped access is correctly managed for any DMA buf
> interaction when accessed by the CPU.
>
> The usage in 2/2 also seems to show it's more directly associated with a
> FrameBuffer (or a mapped frame buffer?) rather than the allocator ...
>
> In MappedFrameBuffer - the user has also already specified with the
> MapFlag if this is a Read,Write, or ReadWrite operation ...
>
> Is the current user only using this associated with a MappedFrameBuffer?
> Do you foresee other users that would need to use DmaSyncer that
> wouldn't be tieing it to a MappedFrameBuffer?

There's only one case in the upcoming mtkisp7 [1].

Seems that the file descriptors and memory address is preset. Memory
address is set by InfoFramePool's custom mmap [2].

[1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/imgsys/single_device.cpp;l=141
[2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/info_frame.cpp;l=109

>
>
> I've added the 'simple DmaSyncer' after I'd added other review comments,
> so it might look out of place if you read straight down.
>
>
>
> > 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           | 90 +++++++++++++++++++
> >  2 files changed, 123 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);
>
> Why is sync() here as a static? Maybe it should be a function inside the
> DmaSyncer? Then it would have direct access to the fd_ that's stored in
> that class?

Yeah that makes more sense. [1] uses the statis sync function directly,
while there's no reason not to use DmaSyncer there.

>
> > +
> >         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_;
>
> Should this be a SharedFD ?
>
> The usage in patch 2/2 is coming from a FrameBuffer::Plane fd, so I
> think we should just store that here if we need to store an fd_ ?

[1] Uses a preset integer file descriptor, while we can worry about it
later if you insist this should be a SharedFD.

>
> > +       DmaBufAllocator::SyncType type_;
>
> I also wonder if SyncType should be defined in the DmaSyncer class
> instead of in the DmaBufAllocator class too!

Definitely :)

>
> > +};
> > +
> >  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..f0f068cc1 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -78,6 +78,81 @@ 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
>
> I think if the implementation moves into the DmaSyncer class, the 'step'
> becomes a private implementation detail, as sync() is only called by the
> constructor and destructor.
>
> I think at that point you could remove the 'Step' and just pass
> DMA_BUF_SYNC_START, or DMA_BUF_SYNC_END to sync() as the starting
> initialisation of flags.
>
> i.e.
>
> DmaSyncer::sync(SyncType type, uint64_t flags)
> {
>         switch(type) {
>         case DmaBufAllocator::SyncType::Read:
>                 flags |= DMA_BUF_SYNC_READ;
>                 break;
>         }
>
>         ...
>         ...
> }
>
> In fact, now I see that - the type is then processed twice - so actually
> it should do that in the constructor - store the flags and then use that
> and this all gets shorter...
>
>
> The following is not compiled... please expect bugs...
>
> class DmaSyncer final
> {
> public:
>         enum class SyncType {
>                 Read = 0,
>                 Write,
>                 ReadWrite,
>         };
>
>         explicit DmaSyncer(SharedFD fd, SyncType type = ReadWrite)
>                 : 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()
>         {
>                 sync(DMA_BUF_SYNC_END);
>         }
>
> private:
>         uint64_t flags = 0;
>
>         void sync(uint64_t step)
>         {
>                 struct dma_buf_sync sync = {
>                         .flags = flags_ | step
>                 };
>
>                 int ret;
>                 do {
>                         ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);
>                 } while (ret && (errno == EINTR || errno == EAGAIN));
>
>                 if (ret) {
>                         ret = errno;
>                         LOG(DmaBufAllocator, Error)
>                                 << "Unable to sync dma fd: " << fd
>                                 << ", err: " << strerror(ret)
>                                 << ", flags: " << sync.flags;
>                 }
>         }
> }
>
>
> Depending on who would actually call this - and if they are already
> expected to have full knowledge of the DMA_BUF_SYNC_{READ,WRITE,RW}
> types - even that switch case could be removed from the constructor..
> but it depends whether that's helpful/preferred to keep as a class enum
> or not.

Thanks! Mostly adopted in the next version.

>
>
> > + */
> > +
> > +/**
> > + * \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 read and 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.
>
> I think if this goes into the DmaSyncer class this method would become
> private, and you won't need to document this. But some of the detail
> might be suited to detail in the DmaSyncer class level documentation.

Done, while I'm very bad at documentation. Please correct/amend me
if necessary :p.

>
> > + */
> > +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;
>
>         flags |= DMA_BUF_SYNC_READ;
>
> same below.
>
>
> > +               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) {
> > +               ret = errno;
> > +               LOG(DmaBufAllocator, Error) << "Unable to sync dma fd: " << fd
> > +                                           << ", err: " << strerror(ret)
> > +                                           << ", 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 +280,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.
>
> Earlier you've called it a 'map session'. I think this is where the
> documentation needs detail about why someone should use this and what it
> solves.

Added a short description.

>
> Talking about a map session makes me think this should be associated in
> some form with a MappedFrameBuffer more than the DmaBufAllocator ?

Apart from the usage with the upcoming InfoFramePool, I'm fine with
putting it aside MappedFrameBuffer, or even in a new file. Just let me
know your preference.

BR,
Harvey

>
>
>
> > + */
> > +
> > +/**
> > + * \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 */
> > --
> > 2.47.0.277.g8800431eea-goog
> >