| Message ID | 20260731122703.111025-1-robert.mader@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert On Fri, Jul 31, 2026 at 02:27:03PM +0200, Robert Mader wrote: > Sync the output buffers in order to ensure data coherency. > > Compare e.g. DebayerCpu::process() > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 5ba76507a72e..a5a812668d2e 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -14,6 +14,7 @@ > > #include <libcamera/framebuffer.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include "libyuv/convert.h" > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > { > ASSERT(!scaledFrameDatas_.empty()); > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > + DmaSyncer::SyncType::Write); > + MappedFrameBuffer mappedFrameBuffer(buffer, > + MappedFrameBuffer::MapFlag::Write); > > const auto &planes = mappedFrameBuffer.planes(); > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 12cce9f7e8c2..95c189760e95 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -11,6 +11,7 @@ > > #include <libcamera/base/log.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include <libyuv/convert_from_argb.h> > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > int TestPatternGenerator::generateFrame(const Size &size, > const FrameBuffer *buffer) > { > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > + DmaSyncer::SyncType::Write); > MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); I wonder if a mappedFrameBuffer wouldn't need a DmaSyncer in most cases... For this patch: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > -- > 2.55.0 >
2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: > Sync the output buffers in order to ensure data coherency. > > Compare e.g. DebayerCpu::process() > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 5ba76507a72e..a5a812668d2e 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -14,6 +14,7 @@ > > #include <libcamera/framebuffer.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include "libyuv/convert.h" > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > { > ASSERT(!scaledFrameDatas_.empty()); > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > + DmaSyncer::SyncType::Write); Why only the first plane? `DmaBufAllocator` allocates on fd for every `FrameBuffer`, so this works, but do we want to add such a hidden dependency? > + MappedFrameBuffer mappedFrameBuffer(buffer, > + MappedFrameBuffer::MapFlag::Write); > > const auto &planes = mappedFrameBuffer.planes(); > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 12cce9f7e8c2..95c189760e95 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -11,6 +11,7 @@ > > #include <libcamera/base/log.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > #include "libcamera/internal/mapped_framebuffer.h" > > #include <libyuv/convert_from_argb.h> > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > int TestPatternGenerator::generateFrame(const Size &size, > const FrameBuffer *buffer) > { > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > + DmaSyncer::SyncType::Write); > MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); >
Hi Barnabás On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: > 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: > > Sync the output buffers in order to ensure data coherency. > > > > Compare e.g. DebayerCpu::process() > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > --- > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > index 5ba76507a72e..a5a812668d2e 100644 > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -14,6 +14,7 @@ > > #include <libcamera/framebuffer.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > > #include "libcamera/internal/mapped_framebuffer.h" > > #include "libyuv/convert.h" > > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > { > > ASSERT(!scaledFrameDatas_.empty()); > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > + DmaSyncer::SyncType::Write); > > Why only the first plane? `DmaBufAllocator` allocates on fd for every > `FrameBuffer`, so this works, but do we want to add such a hidden dependency? > Aren't all planes contiguous, mapped on the same fd at different offsets ? Is this what you meant ? As virtual uses DmaBufAllocator I wonder if there's a case for non-contiguous planes (which would need to be supported by patching DmaBufAllocator if I'm not mistaken). > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > > + MappedFrameBuffer::MapFlag::Write); > > const auto &planes = mappedFrameBuffer.planes(); > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 12cce9f7e8c2..95c189760e95 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -11,6 +11,7 @@ > > #include <libcamera/base/log.h> > > +#include "libcamera/internal/dma_buf_allocator.h" > > #include "libcamera/internal/mapped_framebuffer.h" > > #include <libyuv/convert_from_argb.h> > > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > > int TestPatternGenerator::generateFrame(const Size &size, > > const FrameBuffer *buffer) > > { > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > + DmaSyncer::SyncType::Write); > > MappedFrameBuffer mappedFrameBuffer(buffer, > > MappedFrameBuffer::MapFlag::Write); >
Hi Barnabás, On 03.08.26 11:35, Barnabás Pőcze wrote: > 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: >> Sync the output buffers in order to ensure data coherency. >> >> Compare e.g. DebayerCpu::process() >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- >> src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> index 5ba76507a72e..a5a812668d2e 100644 >> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >> @@ -14,6 +14,7 @@ >> #include <libcamera/framebuffer.h> >> +#include "libcamera/internal/dma_buf_allocator.h" >> #include "libcamera/internal/mapped_framebuffer.h" >> #include "libyuv/convert.h" >> @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const >> Size &size, const FrameBuffer *buff >> { >> ASSERT(!scaledFrameDatas_.empty()); >> - MappedFrameBuffer mappedFrameBuffer(buffer, >> MappedFrameBuffer::MapFlag::Write); >> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >> + DmaSyncer::SyncType::Write); > > Why only the first plane? `DmaBufAllocator` allocates on fd for every > `FrameBuffer`, so this works, but do we want to add such a hidden > dependency? good question. We make the same assumption in the swISP (at least in DebayerEGL - Debayer::dmaSyncBegin loops over the planes and would thus sync contiguous formats multiple times if I'm not mistaken), but then it only supports single-plane formats atm. If we ever plan to support multi-planar formats (I don't see a good reason to do so, but might be nice for testing) it certainly would be more correct to sync once for every distinct FD. I guess that would be another good reason to move the syncing into MappedFrameBuffer (I think we previously discussed that the sync would still need to be called explicitly). I'm just afraid to not have the brain cycles to work on that atm. Best regards > > >> + MappedFrameBuffer mappedFrameBuffer(buffer, >> + MappedFrameBuffer::MapFlag::Write); >> const auto &planes = mappedFrameBuffer.planes(); >> diff --git >> a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> index 12cce9f7e8c2..95c189760e95 100644 >> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >> @@ -11,6 +11,7 @@ >> #include <libcamera/base/log.h> >> +#include "libcamera/internal/dma_buf_allocator.h" >> #include "libcamera/internal/mapped_framebuffer.h" >> #include <libyuv/convert_from_argb.h> >> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; >> int TestPatternGenerator::generateFrame(const Size &size, >> const FrameBuffer *buffer) >> { >> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >> + DmaSyncer::SyncType::Write); >> MappedFrameBuffer mappedFrameBuffer(buffer, >> MappedFrameBuffer::MapFlag::Write); >
2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: >> 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: >>> Sync the output buffers in order to ensure data coherency. >>> >>> Compare e.g. DebayerCpu::process() >>> >>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>> --- >>> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- >>> src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ >>> 2 files changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>> index 5ba76507a72e..a5a812668d2e 100644 >>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>> @@ -14,6 +14,7 @@ >>> #include <libcamera/framebuffer.h> >>> +#include "libcamera/internal/dma_buf_allocator.h" >>> #include "libcamera/internal/mapped_framebuffer.h" >>> #include "libyuv/convert.h" >>> @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff >>> { >>> ASSERT(!scaledFrameDatas_.empty()); >>> - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); >>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>> + DmaSyncer::SyncType::Write); >> >> Why only the first plane? `DmaBufAllocator` allocates on fd for every >> `FrameBuffer`, so this works, but do we want to add such a hidden dependency? >> > > Aren't all planes contiguous, mapped on the same fd at different > offsets ? Is this what you meant ? Yes, but that's only because DmaBufAllocator is used, I was just wondering if we want to depend on this implementation detail. If the allocator is changed, and there are now two separate dma-bufs for the two separate planes, then it would be would be hard to spot the missing synchronization on the second plane. > > As virtual uses DmaBufAllocator I wonder if there's a case for > non-contiguous planes (which would need to be supported by patching > DmaBufAllocator if I'm not mistaken). And there is also the fact if one considers e.g. `FrameBuffer`, then it seems clear to me that there is a desire to avoid implying any contiguity between the planes. So if anything, it could be used for testing to ensure the users handle `FrameBuffer`s correctly. > > >> >>> + MappedFrameBuffer mappedFrameBuffer(buffer, >>> + MappedFrameBuffer::MapFlag::Write); >>> const auto &planes = mappedFrameBuffer.planes(); >>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>> index 12cce9f7e8c2..95c189760e95 100644 >>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>> @@ -11,6 +11,7 @@ >>> #include <libcamera/base/log.h> >>> +#include "libcamera/internal/dma_buf_allocator.h" >>> #include "libcamera/internal/mapped_framebuffer.h" >>> #include <libyuv/convert_from_argb.h> >>> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; >>> int TestPatternGenerator::generateFrame(const Size &size, >>> const FrameBuffer *buffer) >>> { >>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>> + DmaSyncer::SyncType::Write); >>> MappedFrameBuffer mappedFrameBuffer(buffer, >>> MappedFrameBuffer::MapFlag::Write); >>
Hi Barnabás On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: > 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: > > > 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: > > > > Sync the output buffers in order to ensure data coherency. > > > > > > > > Compare e.g. DebayerCpu::process() > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > --- > > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > > > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > index 5ba76507a72e..a5a812668d2e 100644 > > > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > @@ -14,6 +14,7 @@ > > > > #include <libcamera/framebuffer.h> > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > #include "libyuv/convert.h" > > > > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > > > { > > > > ASSERT(!scaledFrameDatas_.empty()); > > > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > + DmaSyncer::SyncType::Write); > > > > > > Why only the first plane? `DmaBufAllocator` allocates on fd for every > > > `FrameBuffer`, so this works, but do we want to add such a hidden dependency? > > > > > > > Aren't all planes contiguous, mapped on the same fd at different > > offsets ? Is this what you meant ? > > Yes, but that's only because DmaBufAllocator is used, I was just wondering > if we want to depend on this implementation detail. If the allocator is changed, > and there are now two separate dma-bufs for the two separate planes, then > it would be would be hard to spot the missing synchronization on the second plane. Should we then iterate on all planes and create one Syncer object for every fd != planes()[0].fd ? It seems a bit far fetched, I would be fine with a comment, but if Robert wants to go and implement this, better safe than sorry for sure. > > > > > > As virtual uses DmaBufAllocator I wonder if there's a case for > > non-contiguous planes (which would need to be supported by patching > > DmaBufAllocator if I'm not mistaken). > > And there is also the fact if one considers e.g. `FrameBuffer`, then > it seems clear to me that there is a desire to avoid implying any contiguity > between the planes. > > So if anything, it could be used for testing to ensure the users handle > `FrameBuffer`s correctly. > > > > > > > > > > > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > > > > + MappedFrameBuffer::MapFlag::Write); > > > > const auto &planes = mappedFrameBuffer.planes(); > > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > index 12cce9f7e8c2..95c189760e95 100644 > > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > @@ -11,6 +11,7 @@ > > > > #include <libcamera/base/log.h> > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > #include <libyuv/convert_from_argb.h> > > > > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > > > > int TestPatternGenerator::generateFrame(const Size &size, > > > > const FrameBuffer *buffer) > > > > { > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > + DmaSyncer::SyncType::Write); > > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > > MappedFrameBuffer::MapFlag::Write); > > > >
Hi Jacopo, On 05.08.26 10:30, Jacopo Mondi wrote: > Hi Barnabás > > On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: >> 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: >>>> 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: >>>>> Sync the output buffers in order to ensure data coherency. >>>>> >>>>> Compare e.g. DebayerCpu::process() >>>>> >>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>> --- >>>>> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- >>>>> src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ >>>>> 2 files changed, 8 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>> index 5ba76507a72e..a5a812668d2e 100644 >>>>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>> @@ -14,6 +14,7 @@ >>>>> #include <libcamera/framebuffer.h> >>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>> #include "libyuv/convert.h" >>>>> @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff >>>>> { >>>>> ASSERT(!scaledFrameDatas_.empty()); >>>>> - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); >>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>> + DmaSyncer::SyncType::Write); >>>> Why only the first plane? `DmaBufAllocator` allocates on fd for every >>>> `FrameBuffer`, so this works, but do we want to add such a hidden dependency? >>>> >>> Aren't all planes contiguous, mapped on the same fd at different >>> offsets ? Is this what you meant ? >> Yes, but that's only because DmaBufAllocator is used, I was just wondering >> if we want to depend on this implementation detail. If the allocator is changed, >> and there are now two separate dma-bufs for the two separate planes, then >> it would be would be hard to spot the missing synchronization on the second plane. > Should we then iterate on all planes and create one Syncer object for > every fd != planes()[0].fd ? > > It seems a bit far fetched, I would be fine with a comment, but if > Robert wants to go and implement this, better safe than sorry for sure. I'd be very much in favor of coming up with a centralized solution that would cover all the most common use-cases (another place still missing a solution is qcam, currently causing glitches on some arm devices) - however I don't think I'll have the brain cycles to work on it in the near future :( So if nobody else can take over I'd prefer to land the patch as is (happy to add a comment and send a v2 with that of course). And arguably having various open-coded places might increase the incentive to work on a centralized solution, as it would allow to remove/clean up code :) Regards > >> >>> As virtual uses DmaBufAllocator I wonder if there's a case for >>> non-contiguous planes (which would need to be supported by patching >>> DmaBufAllocator if I'm not mistaken). >> And there is also the fact if one considers e.g. `FrameBuffer`, then >> it seems clear to me that there is a desire to avoid implying any contiguity >> between the planes. >> >> So if anything, it could be used for testing to ensure the users handle >> `FrameBuffer`s correctly. >> >> >>> >>>>> + MappedFrameBuffer mappedFrameBuffer(buffer, >>>>> + MappedFrameBuffer::MapFlag::Write); >>>>> const auto &planes = mappedFrameBuffer.planes(); >>>>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>> index 12cce9f7e8c2..95c189760e95 100644 >>>>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>> @@ -11,6 +11,7 @@ >>>>> #include <libcamera/base/log.h> >>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>> #include <libyuv/convert_from_argb.h> >>>>> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; >>>>> int TestPatternGenerator::generateFrame(const Size &size, >>>>> const FrameBuffer *buffer) >>>>> { >>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>> + DmaSyncer::SyncType::Write); >>>>> MappedFrameBuffer mappedFrameBuffer(buffer, >>>>> MappedFrameBuffer::MapFlag::Write);
Hi Robert On Wed, Aug 05, 2026 at 10:55:27AM +0200, Robert Mader wrote: > Hi Jacopo, > > On 05.08.26 10:30, Jacopo Mondi wrote: > > Hi Barnabás > > > > On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: > > > 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: > > > > Hi Barnabás > > > > > > > > On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: > > > > > 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: > > > > > > Sync the output buffers in order to ensure data coherency. > > > > > > > > > > > > Compare e.g. DebayerCpu::process() > > > > > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > > > --- > > > > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > > > > > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > > > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > index 5ba76507a72e..a5a812668d2e 100644 > > > > > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > @@ -14,6 +14,7 @@ > > > > > > #include <libcamera/framebuffer.h> > > > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > > > #include "libyuv/convert.h" > > > > > > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > > > > > { > > > > > > ASSERT(!scaledFrameDatas_.empty()); > > > > > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > > > + DmaSyncer::SyncType::Write); > > > > > Why only the first plane? `DmaBufAllocator` allocates on fd for every > > > > > `FrameBuffer`, so this works, but do we want to add such a hidden dependency? > > > > > > > > > Aren't all planes contiguous, mapped on the same fd at different > > > > offsets ? Is this what you meant ? > > > Yes, but that's only because DmaBufAllocator is used, I was just wondering > > > if we want to depend on this implementation detail. If the allocator is changed, > > > and there are now two separate dma-bufs for the two separate planes, then > > > it would be would be hard to spot the missing synchronization on the second plane. > > Should we then iterate on all planes and create one Syncer object for > > every fd != planes()[0].fd ? > > > > It seems a bit far fetched, I would be fine with a comment, but if > > Robert wants to go and implement this, better safe than sorry for sure. > > I'd be very much in favor of coming up with a centralized solution that > would cover all the most common use-cases (another place still missing a > solution is qcam, currently causing glitches on some arm devices) - however > I don't think I'll have the brain cycles to work on it in the near future :( Don't worry, I was just suggesting to do it here locally, not asking for a global solution > > So if nobody else can take over I'd prefer to land the patch as is (happy to > add a comment and send a v2 with that of course). And arguably having Would you mind prefer adding the comment and resend, or should I simply add: /* * \todo Assume all planes are contiguous and mapped at * different offsets on the same fd. This holds true as long * as the implementation of DmaBufAllocator only allocates * contiguous planes. If this will change or a different allocator * will be used then we need to create one dma syncer for each * non-contiguous plane. */ When applying the patch ? > various open-coded places might increase the incentive to work on a > centralized solution, as it would allow to remove/clean up code :) > > Regards > > > > > > > > > > As virtual uses DmaBufAllocator I wonder if there's a case for > > > > non-contiguous planes (which would need to be supported by patching > > > > DmaBufAllocator if I'm not mistaken). > > > And there is also the fact if one considers e.g. `FrameBuffer`, then > > > it seems clear to me that there is a desire to avoid implying any contiguity > > > between the planes. > > > > > > So if anything, it could be used for testing to ensure the users handle > > > `FrameBuffer`s correctly. > > > > > > > > > > > > > > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > > > > > > + MappedFrameBuffer::MapFlag::Write); > > > > > > const auto &planes = mappedFrameBuffer.planes(); > > > > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > index 12cce9f7e8c2..95c189760e95 100644 > > > > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > @@ -11,6 +11,7 @@ > > > > > > #include <libcamera/base/log.h> > > > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > > > #include <libyuv/convert_from_argb.h> > > > > > > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > > > > > > int TestPatternGenerator::generateFrame(const Size &size, > > > > > > const FrameBuffer *buffer) > > > > > > { > > > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > > > + DmaSyncer::SyncType::Write); > > > > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > > > > MappedFrameBuffer::MapFlag::Write); > > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 >
On 05.08.26 11:43, Jacopo Mondi wrote: > Hi Robert > > On Wed, Aug 05, 2026 at 10:55:27AM +0200, Robert Mader wrote: >> Hi Jacopo, >> >> On 05.08.26 10:30, Jacopo Mondi wrote: >>> Hi Barnabás >>> >>> On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: >>>> 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: >>>>> Hi Barnabás >>>>> >>>>> On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: >>>>>> 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: >>>>>>> Sync the output buffers in order to ensure data coherency. >>>>>>> >>>>>>> Compare e.g. DebayerCpu::process() >>>>>>> >>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>>>> --- >>>>>>> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- >>>>>>> src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ >>>>>>> 2 files changed, 8 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>> index 5ba76507a72e..a5a812668d2e 100644 >>>>>>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>> @@ -14,6 +14,7 @@ >>>>>>> #include <libcamera/framebuffer.h> >>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>>>> #include "libyuv/convert.h" >>>>>>> @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff >>>>>>> { >>>>>>> ASSERT(!scaledFrameDatas_.empty()); >>>>>>> - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); >>>>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>>>> + DmaSyncer::SyncType::Write); >>>>>> Why only the first plane? `DmaBufAllocator` allocates on fd for every >>>>>> `FrameBuffer`, so this works, but do we want to add such a hidden dependency? >>>>>> >>>>> Aren't all planes contiguous, mapped on the same fd at different >>>>> offsets ? Is this what you meant ? >>>> Yes, but that's only because DmaBufAllocator is used, I was just wondering >>>> if we want to depend on this implementation detail. If the allocator is changed, >>>> and there are now two separate dma-bufs for the two separate planes, then >>>> it would be would be hard to spot the missing synchronization on the second plane. >>> Should we then iterate on all planes and create one Syncer object for >>> every fd != planes()[0].fd ? >>> >>> It seems a bit far fetched, I would be fine with a comment, but if >>> Robert wants to go and implement this, better safe than sorry for sure. >> I'd be very much in favor of coming up with a centralized solution that >> would cover all the most common use-cases (another place still missing a >> solution is qcam, currently causing glitches on some arm devices) - however >> I don't think I'll have the brain cycles to work on it in the near future :( > Don't worry, I was just suggesting to do it here locally, not asking > for a global solution > >> So if nobody else can take over I'd prefer to land the patch as is (happy to >> add a comment and send a v2 with that of course). And arguably having > Would you mind prefer adding the comment and resend, or should I > simply add: > > /* > * \todo Assume all planes are contiguous and mapped at > * different offsets on the same fd. This holds true as long > * as the implementation of DmaBufAllocator only allocates > * contiguous planes. If this will change or a different allocator > * will be used then we need to create one dma syncer for each > * non-contiguous plane. > */ > > When applying the patch ? Sounds great to me, thanks! (i.e. please feel free to apply the patch with that comment) > >> various open-coded places might increase the incentive to work on a >> centralized solution, as it would allow to remove/clean up code :) >> >> Regards >> >>>>> As virtual uses DmaBufAllocator I wonder if there's a case for >>>>> non-contiguous planes (which would need to be supported by patching >>>>> DmaBufAllocator if I'm not mistaken). >>>> And there is also the fact if one considers e.g. `FrameBuffer`, then >>>> it seems clear to me that there is a desire to avoid implying any contiguity >>>> between the planes. >>>> >>>> So if anything, it could be used for testing to ensure the users handle >>>> `FrameBuffer`s correctly. >>>> >>>> >>>>>>> + MappedFrameBuffer mappedFrameBuffer(buffer, >>>>>>> + MappedFrameBuffer::MapFlag::Write); >>>>>>> const auto &planes = mappedFrameBuffer.planes(); >>>>>>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>> index 12cce9f7e8c2..95c189760e95 100644 >>>>>>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>> @@ -11,6 +11,7 @@ >>>>>>> #include <libcamera/base/log.h> >>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>>>> #include <libyuv/convert_from_argb.h> >>>>>>> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; >>>>>>> int TestPatternGenerator::generateFrame(const Size &size, >>>>>>> const FrameBuffer *buffer) >>>>>>> { >>>>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>>>> + DmaSyncer::SyncType::Write); >>>>>>> MappedFrameBuffer mappedFrameBuffer(buffer, >>>>>>> MappedFrameBuffer::MapFlag::Write); >> -- >> Robert Mader >> Consultant Software Developer >> >> Collabora Ltd. >> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK >> Registered in England & Wales, no. 5513718 >>
Hi again On Wed, Aug 05, 2026 at 11:46:29AM +0200, Robert Mader wrote: > On 05.08.26 11:43, Jacopo Mondi wrote: > > Hi Robert > > > > On Wed, Aug 05, 2026 at 10:55:27AM +0200, Robert Mader wrote: > > > Hi Jacopo, > > > > > > On 05.08.26 10:30, Jacopo Mondi wrote: > > > > Hi Barnabás > > > > > > > > On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: > > > > > 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: > > > > > > Hi Barnabás > > > > > > > > > > > > On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: > > > > > > > 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: > > > > > > > > Sync the output buffers in order to ensure data coherency. > > > > > > > > > > > > > > > > Compare e.g. DebayerCpu::process() > > > > > > > > > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > > > > > --- > > > > > > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- > > > > > > > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ > > > > > > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > > > index 5ba76507a72e..a5a812668d2e 100644 > > > > > > > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > > > > > > @@ -14,6 +14,7 @@ > > > > > > > > #include <libcamera/framebuffer.h> > > > > > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > > > > > #include "libyuv/convert.h" > > > > > > > > @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > > > > > > > { > > > > > > > > ASSERT(!scaledFrameDatas_.empty()); > > > > > > > > - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > > > > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > > > > > + DmaSyncer::SyncType::Write); > > > > > > > Why only the first plane? `DmaBufAllocator` allocates on fd for every > > > > > > > `FrameBuffer`, so this works, but do we want to add such a hidden dependency? > > > > > > > > > > > > > Aren't all planes contiguous, mapped on the same fd at different > > > > > > offsets ? Is this what you meant ? > > > > > Yes, but that's only because DmaBufAllocator is used, I was just wondering > > > > > if we want to depend on this implementation detail. If the allocator is changed, > > > > > and there are now two separate dma-bufs for the two separate planes, then > > > > > it would be would be hard to spot the missing synchronization on the second plane. > > > > Should we then iterate on all planes and create one Syncer object for > > > > every fd != planes()[0].fd ? > > > > > > > > It seems a bit far fetched, I would be fine with a comment, but if > > > > Robert wants to go and implement this, better safe than sorry for sure. > > > I'd be very much in favor of coming up with a centralized solution that > > > would cover all the most common use-cases (another place still missing a > > > solution is qcam, currently causing glitches on some arm devices) - however > > > I don't think I'll have the brain cycles to work on it in the near future :( > > Don't worry, I was just suggesting to do it here locally, not asking > > for a global solution > > > > > So if nobody else can take over I'd prefer to land the patch as is (happy to > > > add a comment and send a v2 with that of course). And arguably having > > Would you mind prefer adding the comment and resend, or should I > > simply add: > > > > /* > > * \todo Assume all planes are contiguous and mapped at > > * different offsets on the same fd. This holds true as long > > * as the implementation of DmaBufAllocator only allocates > > * contiguous planes. If this will change or a different allocator > > * will be used then we need to create one dma syncer for each > > * non-contiguous plane. > > */ > > > > When applying the patch ? > Sounds great to me, thanks! (i.e. please feel free to apply the patch with > that comment) > > At the same time, I noticed: void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output) { for (const FrameBuffer::Plane &plane : input->planes()) dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read); if (output) { for (const FrameBuffer::Plane &plane : output->planes()) dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write); } } which, in case of contiguous planes would create multiple DmaSyncers on the same fd, leading to SYNC_START SYNC_START rw on mapped memory SYNC_END SYNC_END sequences, which I'm not sure about the consequences of: https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#cpu-access-to-dma-buffer-objects The original code in debayer_cpu.cpp was introduced by 1197fff4821e119c5aeca017fa095f2225aed292 so maybe the above issue went unnoticed or maybe it's not a problem at all ? I'm asking because once I re-looked at Debayer::dmaSyncBegin() that you mention in your comment I re-considered and thought we might want to do the same.But now I wonder if Debayer::dmaSyncBegin() is actually problematic. > > > various open-coded places might increase the incentive to work on a > > > centralized solution, as it would allow to remove/clean up code :) > > > > > > Regards > > > > > > > As virtual uses DmaBufAllocator I wonder if there's a case for > > > > > > non-contiguous planes (which would need to be supported by patching > > > > > > DmaBufAllocator if I'm not mistaken). > > > > > And there is also the fact if one considers e.g. `FrameBuffer`, then > > > > > it seems clear to me that there is a desire to avoid implying any contiguity > > > > > between the planes. > > > > > > > > > > So if anything, it could be used for testing to ensure the users handle > > > > > `FrameBuffer`s correctly. > > > > > > > > > > > > > > > > > > + MappedFrameBuffer mappedFrameBuffer(buffer, > > > > > > > > + MappedFrameBuffer::MapFlag::Write); > > > > > > > > const auto &planes = mappedFrameBuffer.planes(); > > > > > > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > > > index 12cce9f7e8c2..95c189760e95 100644 > > > > > > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > > > > > > @@ -11,6 +11,7 @@ > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > +#include "libcamera/internal/dma_buf_allocator.h" > > > > > > > > #include "libcamera/internal/mapped_framebuffer.h" > > > > > > > > #include <libyuv/convert_from_argb.h> > > > > > > > > @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; > > > > > > > > int TestPatternGenerator::generateFrame(const Size &size, > > > > > > > > const FrameBuffer *buffer) > > > > > > > > { > > > > > > > > + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, > > > > > > > > + DmaSyncer::SyncType::Write); > > > > > > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > > > > > > MappedFrameBuffer::MapFlag::Write); > > > -- > > > Robert Mader > > > Consultant Software Developer > > > > > > Collabora Ltd. > > > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > > > Registered in England & Wales, no. 5513718 > > > > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 >
Hi, On 06.08.26 09:32, Jacopo Mondi wrote: > Hi again > > On Wed, Aug 05, 2026 at 11:46:29AM +0200, Robert Mader wrote: >> On 05.08.26 11:43, Jacopo Mondi wrote: >>> Hi Robert >>> >>> On Wed, Aug 05, 2026 at 10:55:27AM +0200, Robert Mader wrote: >>>> Hi Jacopo, >>>> >>>> On 05.08.26 10:30, Jacopo Mondi wrote: >>>>> Hi Barnabás >>>>> >>>>> On Tue, Aug 04, 2026 at 01:34:41PM +0200, Barnabás Pőcze wrote: >>>>>> 2026. 08. 03. 11:56 keltezéssel, Jacopo Mondi írta: >>>>>>> Hi Barnabás >>>>>>> >>>>>>> On Mon, Aug 03, 2026 at 11:35:56AM +0200, Barnabás Pőcze wrote: >>>>>>>> 2026. 07. 31. 14:27 keltezéssel, Robert Mader írta: >>>>>>>>> Sync the output buffers in order to ensure data coherency. >>>>>>>>> >>>>>>>>> Compare e.g. DebayerCpu::process() >>>>>>>>> >>>>>>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>>>>>> --- >>>>>>>>> src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- >>>>>>>>> src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ >>>>>>>>> 2 files changed, 8 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>>>> index 5ba76507a72e..a5a812668d2e 100644 >>>>>>>>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>>>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp >>>>>>>>> @@ -14,6 +14,7 @@ >>>>>>>>> #include <libcamera/framebuffer.h> >>>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>>>>>> #include "libyuv/convert.h" >>>>>>>>> @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff >>>>>>>>> { >>>>>>>>> ASSERT(!scaledFrameDatas_.empty()); >>>>>>>>> - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); >>>>>>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>>>>>> + DmaSyncer::SyncType::Write); >>>>>>>> Why only the first plane? `DmaBufAllocator` allocates on fd for every >>>>>>>> `FrameBuffer`, so this works, but do we want to add such a hidden dependency? >>>>>>>> >>>>>>> Aren't all planes contiguous, mapped on the same fd at different >>>>>>> offsets ? Is this what you meant ? >>>>>> Yes, but that's only because DmaBufAllocator is used, I was just wondering >>>>>> if we want to depend on this implementation detail. If the allocator is changed, >>>>>> and there are now two separate dma-bufs for the two separate planes, then >>>>>> it would be would be hard to spot the missing synchronization on the second plane. >>>>> Should we then iterate on all planes and create one Syncer object for >>>>> every fd != planes()[0].fd ? >>>>> >>>>> It seems a bit far fetched, I would be fine with a comment, but if >>>>> Robert wants to go and implement this, better safe than sorry for sure. >>>> I'd be very much in favor of coming up with a centralized solution that >>>> would cover all the most common use-cases (another place still missing a >>>> solution is qcam, currently causing glitches on some arm devices) - however >>>> I don't think I'll have the brain cycles to work on it in the near future :( >>> Don't worry, I was just suggesting to do it here locally, not asking >>> for a global solution >>> >>>> So if nobody else can take over I'd prefer to land the patch as is (happy to >>>> add a comment and send a v2 with that of course). And arguably having >>> Would you mind prefer adding the comment and resend, or should I >>> simply add: >>> >>> /* >>> * \todo Assume all planes are contiguous and mapped at >>> * different offsets on the same fd. This holds true as long >>> * as the implementation of DmaBufAllocator only allocates >>> * contiguous planes. If this will change or a different allocator >>> * will be used then we need to create one dma syncer for each >>> * non-contiguous plane. >>> */ >>> >>> When applying the patch ? >> Sounds great to me, thanks! (i.e. please feel free to apply the patch with >> that comment) > At the same time, I noticed: > > void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *input, FrameBuffer *output) > { > for (const FrameBuffer::Plane &plane : input->planes()) > dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read); > > if (output) { > for (const FrameBuffer::Plane &plane : output->planes()) > dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write); > } > } > > which, in case of contiguous planes would create multiple DmaSyncers > on the same fd, leading to > > SYNC_START > SYNC_START > > rw on mapped memory > > SYNC_END > SYNC_END > > sequences, which I'm not sure about the consequences of: > https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#cpu-access-to-dma-buffer-objects > > The original code in debayer_cpu.cpp was introduced by > 1197fff4821e119c5aeca017fa095f2225aed292 > > so maybe the above issue went unnoticed or maybe it's not a problem at > all ? Yeah, I also only noticed that when looking at it again for this discussion. In fact the same behavior was already present in the previous code which I introduced myself in cbfe04f77ca6e. However: 1. Unlike in virtual we don't yet support multi-plane formats in the softIST so the code was never run in a loop. Further more DebayerEGL doesn't sync the output buffer (as it only accesses it with the GPU), so even if we start supporting YUV420 (we really should) it would only affect DebayerCPU. 2. Even if we ran the syncs as you describe above I wouldn't expect errors/glitches - just a performance drop from the redundant ioctls / syncs / cache flushes. So we should still get a better solution going forward. P.S.: now I wonder if Chromium and Gstreamer do similar mistakes regarding redundant syncs... > > I'm asking because once I re-looked at Debayer::dmaSyncBegin() that > you mention in your comment I re-considered and thought we might want > to do the same.But now I wonder if Debayer::dmaSyncBegin() is actually > problematic. > >>>> various open-coded places might increase the incentive to work on a >>>> centralized solution, as it would allow to remove/clean up code :) >>>> >>>> Regards >>>>>>> As virtual uses DmaBufAllocator I wonder if there's a case for >>>>>>> non-contiguous planes (which would need to be supported by patching >>>>>>> DmaBufAllocator if I'm not mistaken). >>>>>> And there is also the fact if one considers e.g. `FrameBuffer`, then >>>>>> it seems clear to me that there is a desire to avoid implying any contiguity >>>>>> between the planes. >>>>>> >>>>>> So if anything, it could be used for testing to ensure the users handle >>>>>> `FrameBuffer`s correctly. >>>>>> >>>>>> >>>>>>>>> + MappedFrameBuffer mappedFrameBuffer(buffer, >>>>>>>>> + MappedFrameBuffer::MapFlag::Write); >>>>>>>>> const auto &planes = mappedFrameBuffer.planes(); >>>>>>>>> diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>>>> index 12cce9f7e8c2..95c189760e95 100644 >>>>>>>>> --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>>>> +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp >>>>>>>>> @@ -11,6 +11,7 @@ >>>>>>>>> #include <libcamera/base/log.h> >>>>>>>>> +#include "libcamera/internal/dma_buf_allocator.h" >>>>>>>>> #include "libcamera/internal/mapped_framebuffer.h" >>>>>>>>> #include <libyuv/convert_from_argb.h> >>>>>>>>> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; >>>>>>>>> int TestPatternGenerator::generateFrame(const Size &size, >>>>>>>>> const FrameBuffer *buffer) >>>>>>>>> { >>>>>>>>> + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, >>>>>>>>> + DmaSyncer::SyncType::Write); >>>>>>>>> MappedFrameBuffer mappedFrameBuffer(buffer, >>>>>>>>> MappedFrameBuffer::MapFlag::Write); >>>> -- >>>> Robert Mader >>>> Consultant Software Developer >>>> >>>> Collabora Ltd. >>>> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK >>>> Registered in England & Wales, no. 5513718 >>>> >> -- >> Robert Mader >> Consultant Software Developer >> >> Collabora Ltd. >> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK >> Registered in England & Wales, no. 5513718 >>
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index 5ba76507a72e..a5a812668d2e 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -14,6 +14,7 @@ #include <libcamera/framebuffer.h> +#include "libcamera/internal/dma_buf_allocator.h" #include "libcamera/internal/mapped_framebuffer.h" #include "libyuv/convert.h" @@ -127,7 +128,10 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff { ASSERT(!scaledFrameDatas_.empty()); - MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, + DmaSyncer::SyncType::Write); + MappedFrameBuffer mappedFrameBuffer(buffer, + MappedFrameBuffer::MapFlag::Write); const auto &planes = mappedFrameBuffer.planes(); diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 12cce9f7e8c2..95c189760e95 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -11,6 +11,7 @@ #include <libcamera/base/log.h> +#include "libcamera/internal/dma_buf_allocator.h" #include "libcamera/internal/mapped_framebuffer.h" #include <libyuv/convert_from_argb.h> @@ -44,6 +45,8 @@ static const unsigned int kARGBSize = 4; int TestPatternGenerator::generateFrame(const Size &size, const FrameBuffer *buffer) { + DmaSyncer outDmaSyncer(buffer->planes()[0].fd, + DmaSyncer::SyncType::Write); MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write);
Sync the output buffers in order to ensure data coherency. Compare e.g. DebayerCpu::process() Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/libcamera/pipeline/virtual/image_frame_generator.cpp | 6 +++++- src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-)