| Message ID | 20241129185533.3308449-2-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Harvey Yang (2024-11-29 18:53:28) > This fixes commit 39482d59fe7160740ea5dd61f3ed965a88d848ce. Have you seen how we style fixes tags to match the linux kernel? Taken from: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes """ If your patch fixes a bug in a specific commit, e.g. you found an issue using git bisect, please use the ‘Fixes:’ tag with the first 12 characters of the SHA-1 ID, and the one line summary. Do not split the tag across multiple lines, tags are exempt from the “wrap at 75 columns” rule in order to simplify parsing scripts. For example: Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed") The following git config settings can be used to add a pretty format for outputting the above style in the git log or git show commands: [core] abbrev = 12 [pretty] fixes = Fixes: %h (\"%s\") An example call: $ git log -1 --pretty=fixes 54a4f0239f2e Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed") """ > > 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. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> With the fixes tag corrected 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 fc5de2c13..0cb209630 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; > + DmaSyncer &operator=(DmaSyncer &&) = 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 3cc52f968..5f517828d 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; > + * \brief Enable move on class DmaSyncer > + */ > + > +/** > + * \fn DmaSyncer::operator=(DmaSyncer &&) = default; > + * \brief Enable move on class DmaSyncer > + */ > + > DmaSyncer::~DmaSyncer() > { > sync(DMA_BUF_SYNC_END); > -- > 2.47.0.338.g60cca15819-goog >
Hi Kieran, On Wed, Dec 11, 2024 at 1:25 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Harvey Yang (2024-11-29 18:53:28) > > This fixes commit 39482d59fe7160740ea5dd61f3ed965a88d848ce. > > Have you seen how we style fixes tags to match the linux kernel? > > Taken from: https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > > > """ > > If your patch fixes a bug in a specific commit, e.g. you found an issue > using git bisect, please use the ‘Fixes:’ tag with the first 12 > characters of the SHA-1 ID, and the one line summary. Do not split the > tag across multiple lines, tags are exempt from the “wrap at 75 columns” > rule in order to simplify parsing scripts. For example: > > Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed") > > The following git config settings can be used to add a pretty format for > outputting the above style in the git log or git show commands: > > [core] > abbrev = 12 > [pretty] > fixes = Fixes: %h (\"%s\") > > An example call: > > $ git log -1 --pretty=fixes 54a4f0239f2e > Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed") > Thanks for the suggestion! Added in my git config, and sent a new version. BR, Harvey > """ > > > > > > > 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. > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > With the fixes tag corrected > > > 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 fc5de2c13..0cb209630 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; > > + DmaSyncer &operator=(DmaSyncer &&) = 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 3cc52f968..5f517828d 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; > > + * \brief Enable move on class DmaSyncer > > + */ > > + > > +/** > > + * \fn DmaSyncer::operator=(DmaSyncer &&) = default; > > + * \brief Enable move on class DmaSyncer > > + */ > > + > > DmaSyncer::~DmaSyncer() > > { > > sync(DMA_BUF_SYNC_END); > > -- > > 2.47.0.338.g60cca15819-goog > >
diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h index fc5de2c13..0cb209630 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; + DmaSyncer &operator=(DmaSyncer &&) = 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 3cc52f968..5f517828d 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; + * \brief Enable move on class DmaSyncer + */ + +/** + * \fn DmaSyncer::operator=(DmaSyncer &&) = default; + * \brief Enable move on class DmaSyncer + */ + DmaSyncer::~DmaSyncer() { sync(DMA_BUF_SYNC_END);
This fixes commit 39482d59fe7160740ea5dd61f3ed965a88d848ce. 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. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- include/libcamera/internal/dma_buf_allocator.h | 5 +++++ src/libcamera/dma_buf_allocator.cpp | 10 ++++++++++ 2 files changed, 15 insertions(+)