[PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable

[Milan Zamazal]

Hi,

Harvey Yang <chenghaoyang at chromium.org> writes:

> As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer
> instance would trigger sync end earlier than expected. This patch makes
> it non-copyable to avoid the issue.

After this patch, software ISP still works but reports the following
error on each frame:

  ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor, flags: 5

DmaSyncer is used only in debayer_cpu.cpp:

  std::vector<DmaSyncer> dmaSyncers;
  for (const FrameBuffer::Plane &plane : input->planes())
  	dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);
  
  for (const FrameBuffer::Plane &plane : output->planes())
  	dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);
  
  green_ = params.green;
  red_ = swapRedBlueGains_ ? params.blue : params.red;
  blue_ = swapRedBlueGains_ ? params.red : params.blue;
  
  /* Copy metadata from the input buffer */
  FrameMetadata &metadata = output->_d()->metadata();
  metadata.status = input->metadata().status;
  metadata.sequence = input->metadata().sequence;
  metadata.timestamp = input->metadata().timestamp;
  
  MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);
  MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);
  if (!in.isValid() || !out.isValid()) {
  	LOG(Debayer, Error) << "mmap-ing buffer(s) failed";
  	metadata.status = FrameMetadata::FrameError;
  	return;
  }
  
  stats_->startFrame();
  
  if (inputConfig_.patternSize.height == 2)
  	process2(in.planes()[0].data(), out.planes()[0].data());
  else
  	process4(in.planes()[0].data(), out.planes()[0].data());
  
  metadata.planes()[0].bytesused = out.planes()[0].size();
  
  dmaSyncers.clear();

Any idea what could be wrong?

> Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class")
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/internal/dma_buf_allocator.h |  5 +++++
>  src/libcamera/dma_buf_allocator.cpp            | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index fc5de2c13edd..d26f8a74f4c6 100644
> --- a/include/libcamera/internal/dma_buf_allocator.h
> +++ b/include/libcamera/internal/dma_buf_allocator.h
> @@ -60,9 +60,14 @@ public:
>  
>  	explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);
>  
> +	DmaSyncer(DmaSyncer &&other) = default;
> +	DmaSyncer &operator=(DmaSyncer &&other) = default;
> +
>  	~DmaSyncer();
>  
>  private:
> +	LIBCAMERA_DISABLE_COPY(DmaSyncer)
> +
>  	void sync(uint64_t step);
>  
>  	SharedFD fd_;
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index 3cc52f9686b0..a014c3b4263c 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
>  	sync(DMA_BUF_SYNC_START);
>  }
>  
> +/**
> + * \fn DmaSyncer::DmaSyncer(DmaSyncer &&other);
> + * \param[in] other The other instance
> + * \brief Enable move on class DmaSyncer
> + */
> +
> +/**
> + * \fn DmaSyncer::operator=(DmaSyncer &&other);
> + * \param[in] other The other instance
> + * \brief Enable move on class DmaSyncer
> + */
> +
>  DmaSyncer::~DmaSyncer()
>  {
>  	sync(DMA_BUF_SYNC_END);