[v3] libcamera: debayer_cpu: Sync DMABUFs
diff mbox series

Message ID 20240916141125.212973-1-robert.mader@collabora.com
State Accepted
Headers show
Series
  • [v3] libcamera: debayer_cpu: Sync DMABUFs
Related show

Commit Message

Robert Mader Sept. 16, 2024, 2:11 p.m. UTC 
Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure
correct output. Not doing so currently results in occasional tearing
and/or backlashes in GL/VK clients that use the buffers directly for
rendering.

An alternative approach to have the sync code in `MappedFrameBuffer` was
considered but rejected for now, in order to allow clients more
flexibility.

Signed-off-by: Robert Mader <robert.mader@collabora.com>

---

Changes in v3:
 - add syncBufferForCPU() helper function

Changes in v2:
 - sync input buffer as well
 - update commit title accordingly
 - small changes to the commit message
 - linting fixes, should pass now
---
 src/libcamera/software_isp/debayer_cpu.cpp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Kieran Bingham Sept. 16, 2024, 3:10 p.m. UTC | #1 
Quoting Robert Mader (2024-09-16 15:11:25)
> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure
> correct output. Not doing so currently results in occasional tearing
> and/or backlashes in GL/VK clients that use the buffers directly for
> rendering.
> 
> An alternative approach to have the sync code in `MappedFrameBuffer` was
> considered but rejected for now, in order to allow clients more
> flexibility.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> 
> ---
> 
> Changes in v3:
>  - add syncBufferForCPU() helper function
> 
> Changes in v2:
>  - sync input buffer as well
>  - update commit title accordingly
>  - small changes to the commit message
>  - linting fixes, should pass now
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4b..3748bd7c 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -12,8 +12,11 @@
>  #include "debayer_cpu.h"
>  
>  #include <stdlib.h>
> +#include <sys/ioctl.h>
>  #include <time.h>
>  
> +#include <linux/dma-buf.h>
> +
>  #include <libcamera/formats.h>
>  
>  #include "libcamera/internal/bayer_format.h"
> @@ -718,6 +721,17 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)
>         }
>  }
>  
> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)

I think these functions (including timeDiff below) should be wrapped in
an annoymous namespace for the libcamera code style.

But as the timeDiff is already in this form - they can both be converted
after. (or before if you want to do the fix up)

> +{
> +       for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +               const int fd = plane.fd.get();
> +               struct dma_buf_sync sync = { syncFlags };
> +
> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;

This could be rewrapped easily

			LOG(Debayer, Error)
				<< "Syncing buffer FD " << fd << "failed: " << errno;

But we can't reference errno directly from LOG() either, as mentioned in
the previous version:

Easiest cleanup is probably:

		int ret;

		ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);
		if (ret < 0) {
			ret = errno;
			LOG(Debayer, Error)
				<< "Syncing buffer FD " << fd << "failed: "
				<< strerror(-ret);

		}

Could also print the sync flags too to know which one has failed?

With that handled:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       }
> +}
> +
>  static inline int64_t timeDiff(timespec &after, timespec &before)
>  {
>         return (after.tv_sec - before.tv_sec) * 1000000000LL +
> @@ -733,6 +747,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>                 clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>         }
>  
> +       syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
> +       syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);
> +
>         green_ = params.green;
>         red_ = swapRedBlueGains_ ? params.blue : params.red;
>         blue_ = swapRedBlueGains_ ? params.red : params.blue;
> @@ -760,6 +777,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>  
>         metadata.planes()[0].bytesused = out.planes()[0].size();
>  
> +       syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
> +       syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
> +
>         /* Measure before emitting signals */
>         if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>             ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
> -- 
> 2.46.0
>
Robert Mader Sept. 16, 2024, 4:15 p.m. UTC | #2 
On 16.09.24 17:10, Kieran Bingham wrote:
> Quoting Robert Mader (2024-09-16 15:11:25)
>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure
>> correct output. Not doing so currently results in occasional tearing
>> and/or backlashes in GL/VK clients that use the buffers directly for
>> rendering.
>>
>> An alternative approach to have the sync code in `MappedFrameBuffer` was
>> considered but rejected for now, in order to allow clients more
>> flexibility.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>
>> ---
>>
>> Changes in v3:
>>   - add syncBufferForCPU() helper function
>>
>> Changes in v2:
>>   - sync input buffer as well
>>   - update commit title accordingly
>>   - small changes to the commit message
>>   - linting fixes, should pass now
>> ---
>>   src/libcamera/software_isp/debayer_cpu.cpp | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 077f7f4b..3748bd7c 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -12,8 +12,11 @@
>>   #include "debayer_cpu.h"
>>   
>>   #include <stdlib.h>
>> +#include <sys/ioctl.h>
>>   #include <time.h>
>>   
>> +#include <linux/dma-buf.h>
>> +
>>   #include <libcamera/formats.h>
>>   
>>   #include "libcamera/internal/bayer_format.h"
>> @@ -718,6 +721,17 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)
>>          }
>>   }
>>   
>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
> I think these functions (including timeDiff below) should be wrapped in
> an annoymous namespace for the libcamera code style.
>
> But as the timeDiff is already in this form - they can both be converted
> after. (or before if you want to do the fix up)
>
>> +{
>> +       for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> +               const int fd = plane.fd.get();
>> +               struct dma_buf_sync sync = { syncFlags };
>> +
>> +               if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>> +                       LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
> This could be rewrapped easily
>
> 			LOG(Debayer, Error)
> 				<< "Syncing buffer FD " << fd << "failed: " << errno;
>
> But we can't reference errno directly from LOG() either, as mentioned in
> the previous version:
>
> Easiest cleanup is probably:
>
> 		int ret;
>
> 		ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);
> 		if (ret < 0) {
> 			ret = errno;
> 			LOG(Debayer, Error)
> 				<< "Syncing buffer FD " << fd << "failed: "
> 				<< strerror(-ret);
>
> 		}
>
> Could also print the sync flags too to know which one has failed?
>
> With that handled:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks, addressed in v4, hopefully correctly (not quite sure about how 
to best print the flags, therefore didn't add the R-b).

>
>> +       }
>> +}
>> +
>>   static inline int64_t timeDiff(timespec &after, timespec &before)
>>   {
>>          return (after.tv_sec - before.tv_sec) * 1000000000LL +
>> @@ -733,6 +747,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>                  clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>          }
>>   
>> +       syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
>> +       syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);
>> +
>>          green_ = params.green;
>>          red_ = swapRedBlueGains_ ? params.blue : params.red;
>>          blue_ = swapRedBlueGains_ ? params.red : params.blue;
>> @@ -760,6 +777,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>   
>>          metadata.planes()[0].bytesused = out.planes()[0].size();
>>   
>> +       syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
>> +       syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
>> +
>>          /* Measure before emitting signals */
>>          if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>>              ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
>> -- 
>> 2.46.0
>>

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 077f7f4b..3748bd7c 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -12,8 +12,11 @@ 
 #include "debayer_cpu.h"
 
 #include <stdlib.h>
+#include <sys/ioctl.h>
 #include <time.h>
 
+#include <linux/dma-buf.h>
+
 #include <libcamera/formats.h>
 
 #include "libcamera/internal/bayer_format.h"
@@ -718,6 +721,17 @@  void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)
 	}
 }
 
+static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)
+{
+	for (const FrameBuffer::Plane &plane : buffer->planes()) {
+		const int fd = plane.fd.get();
+		struct dma_buf_sync sync = { syncFlags };
+
+		if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
+			LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
+	}
+}
+
 static inline int64_t timeDiff(timespec &after, timespec &before)
 {
 	return (after.tv_sec - before.tv_sec) * 1000000000LL +
@@ -733,6 +747,9 @@  void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
 		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
 	}
 
+	syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);
+	syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);
+
 	green_ = params.green;
 	red_ = swapRedBlueGains_ ? params.blue : params.red;
 	blue_ = swapRedBlueGains_ ? params.red : params.blue;
@@ -760,6 +777,9 @@  void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
 
 	metadata.planes()[0].bytesused = out.planes()[0].size();
 
+	syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);
+	syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);
+
 	/* Measure before emitting signals */
 	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
 	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {