[v2,0/2] Add DmaSyncer
mbox series

Message ID 20241113055524.3099340-1-chenghaoyang@chromium.org
Headers show
Series
  • Add DmaSyncer
Related show

Message

Cheng-Hao Yang Nov. 13, 2024, 5:54 a.m. UTC
Hi folks,

This series of patches follows the discussion with Kieran in patch [1],
which adds a helper function and a helper class to make synchronizing
DMA buffers easier.

The second patch updates debayer_cpu to utilize the new helper class.
The following mtkisp7 pipeline handler will also depend on the helper
class.

I put the function in DmaBufAllocator and the helper class in the same
file for now. Let me know if I should put them elsewhere.

This passes gitlab pipeline:
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1309296

v2:
- Fixed ioctl return value check when being interrupted.

BR,
Harvey

[1]: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs


Harvey Yang (2):
  DmaBufAllocator: Add Dma Buffer synchronization function & helper
    class
  debayer_cpu: Replace syncing DMABUFs with DmaSyncer

 .../libcamera/internal/dma_buf_allocator.h    | 33 +++++++
 src/libcamera/dma_buf_allocator.cpp           | 90 +++++++++++++++++++
 src/libcamera/software_isp/debayer_cpu.cpp    | 29 ++----
 3 files changed, 131 insertions(+), 21 deletions(-)

Comments

Kieran Bingham Nov. 13, 2024, 10 a.m. UTC | #1
Quoting Harvey Yang (2024-11-13 05:54:31)
> Hi folks,
> 
> This series of patches follows the discussion with Kieran in patch [1],

Aha, ok so I was a bit confused here. I didn't recall suggesting anytime
we should make this into an object, but what you mean is that it follows
"Roberts" implementation - not 'Mine'.

I don't mind a helper object here ... Just clarifying (to my self) that
this was not produced from something I said ;-)

Moving syncBufferForCPU to a helper in dma_buf_allocator.h does indeed
seem like a good idea to promote re-usabilty though! So lets follow the
reviews on the patches ;-)

--
Kieran

> which adds a helper function and a helper class to make synchronizing
> DMA buffers easier.
> 
> The second patch updates debayer_cpu to utilize the new helper class.
> The following mtkisp7 pipeline handler will also depend on the helper
> class.
> 
> I put the function in DmaBufAllocator and the helper class in the same
> file for now. Let me know if I should put them elsewhere.
> 
> This passes gitlab pipeline:
> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1309296
> 
> v2:
> - Fixed ioctl return value check when being interrupted.
> 
> BR,
> Harvey
> 
> [1]: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs
> 
> 
> Harvey Yang (2):
>   DmaBufAllocator: Add Dma Buffer synchronization function & helper
>     class
>   debayer_cpu: Replace syncing DMABUFs with DmaSyncer
> 
>  .../libcamera/internal/dma_buf_allocator.h    | 33 +++++++
>  src/libcamera/dma_buf_allocator.cpp           | 90 +++++++++++++++++++
>  src/libcamera/software_isp/debayer_cpu.cpp    | 29 ++----
>  3 files changed, 131 insertions(+), 21 deletions(-)
> 
> -- 
> 2.47.0.277.g8800431eea-goog
>
Cheng-Hao Yang Nov. 13, 2024, 10:10 a.m. UTC | #2
Hi Kieran,

On Wed, Nov 13, 2024 at 6:00 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-11-13 05:54:31)
> > Hi folks,
> >
> > This series of patches follows the discussion with Kieran in patch [1],
>
> Aha, ok so I was a bit confused here. I didn't recall suggesting anytime
> we should make this into an object, but what you mean is that it follows
> "Roberts" implementation - not 'Mine'.
>
> I don't mind a helper object here ... Just clarifying (to my self) that
> this was not produced from something I said ;-)

Ah I didn't mean that the current version is the conclusion of our
discussion or your suggestion. Just saying that there's such a
discussion, and this series is what I think a first step to work on with.

Sorry if the message looks to mean otherwise :)

BR,
Harvey

>
> Moving syncBufferForCPU to a helper in dma_buf_allocator.h does indeed
> seem like a good idea to promote re-usabilty though! So lets follow the
> reviews on the patches ;-)
>
> --
> Kieran
>
> > which adds a helper function and a helper class to make synchronizing
> > DMA buffers easier.
> >
> > The second patch updates debayer_cpu to utilize the new helper class.
> > The following mtkisp7 pipeline handler will also depend on the helper
> > class.
> >
> > I put the function in DmaBufAllocator and the helper class in the same
> > file for now. Let me know if I should put them elsewhere.
> >
> > This passes gitlab pipeline:
> > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1309296
> >
> > v2:
> > - Fixed ioctl return value check when being interrupted.
> >
> > BR,
> > Harvey
> >
> > [1]: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs
> >
> >
> > Harvey Yang (2):
> >   DmaBufAllocator: Add Dma Buffer synchronization function & helper
> >     class
> >   debayer_cpu: Replace syncing DMABUFs with DmaSyncer
> >
> >  .../libcamera/internal/dma_buf_allocator.h    | 33 +++++++
> >  src/libcamera/dma_buf_allocator.cpp           | 90 +++++++++++++++++++
> >  src/libcamera/software_isp/debayer_cpu.cpp    | 29 ++----
> >  3 files changed, 131 insertions(+), 21 deletions(-)
> >
> > --
> > 2.47.0.277.g8800431eea-goog
> >