[v2] DmaBufAllocator: Make DmaSyncer non-copyable

22286 diff mbox series

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.

Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class")
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/dma_buf_allocator.h |  5 +++++
 src/libcamera/dma_buf_allocator.cpp            | 10 ++++++++++
 2 files changed, 15 insertions(+)

Hi Harvey,

Thank you for the patch.

On Wed, Dec 11, 2024 at 04:16:08AM +0000, Harvey Yang wrote:
> 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.
> 
> Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class")
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/dma_buf_allocator.h |  5 +++++
>  src/libcamera/dma_buf_allocator.cpp            | 10 ++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index fc5de2c13edd..0cb20963098c 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 &&) = default;

We don't usually omit parameter names in either files.

	DmaSyncer(DmaSyncer &&other) = default;

> +	DmaSyncer &operator=(DmaSyncer &&) = default;

Same here, and same in the documentation below.

> +
>  	~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..5f517828d960 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -311,6 +311,16 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
>  	sync(DMA_BUF_SYNC_START);
>  }
>  
> +/**
> + * \fn DmaSyncer::DmaSyncer(DmaSyncer &&) = default;

drop ' = default;' here and below.

> + * \brief Enable move on class DmaSyncer

 * \param[in] other The other instance

same below.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +
> +/**
> + * \fn DmaSyncer::operator=(DmaSyncer &&) = default;
> + * \brief Enable move on class DmaSyncer
> + */
> +
>  DmaSyncer::~DmaSyncer()
>  {
>  	sync(DMA_BUF_SYNC_END);

Hi Laurent,

On Wed, Dec 11, 2024 at 4:00 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey,
>
> Thank you for the patch.
>
> On Wed, Dec 11, 2024 at 04:16:08AM +0000, Harvey Yang wrote:
> > 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.
> >
> > Fixes: 39482d59fe71 ("DmaBufAllocator: Add Dma Buffer synchronization function & helper class")
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/internal/dma_buf_allocator.h |  5 +++++
> >  src/libcamera/dma_buf_allocator.cpp            | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> > index fc5de2c13edd..0cb20963098c 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 &&) = default;
>
> We don't usually omit parameter names in either files.
>
>         DmaSyncer(DmaSyncer &&other) = default;
>
> > +     DmaSyncer &operator=(DmaSyncer &&) = default;
>
> Same here, and same in the documentation below.
>
> > +
> >       ~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..5f517828d960 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -311,6 +311,16 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)
> >       sync(DMA_BUF_SYNC_START);
> >  }
> >
> > +/**
> > + * \fn DmaSyncer::DmaSyncer(DmaSyncer &&) = default;
>
> drop ' = default;' here and below.
>
> > + * \brief Enable move on class DmaSyncer
>
>  * \param[in] other The other instance
>
> same below.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for the review.
Updated accordingly in v3. Please let me know if I missed anything.

BR,
Harvey

>
> > + */
> > +
> > +/**
> > + * \fn DmaSyncer::operator=(DmaSyncer &&) = default;
> > + * \brief Enable move on class DmaSyncer
> > + */
> > +
> >  DmaSyncer::~DmaSyncer()
> >  {
> >       sync(DMA_BUF_SYNC_END);
>
> --
> Regards,
>
> Laurent Pinchart