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

[Kieran Bingham]

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 at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at 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
>