[PATCH] libcamera: debayer_cpu: Sync output buffer

[Robert Mader]

On 01.09.24 13:07, Laurent Pinchart wrote:
> On Sun, Sep 01, 2024 at 12:56:10PM +0200, Hans de Goede wrote:
>> Hi Robert,
>>
>> On 8/31/24 9:05 PM, Robert Mader wrote:
>>> Using `DMA_BUF_IOCTL_SYNC` is required for dmabufs in order to ensure
>>> correct results. Not doing so currently results in occasional tearing
>>> and/or backlashes in GL/VK clients that use the buffers directly for
>>> rendering.
>>>
>>> Signed-off-by: Robert Mader <robert.mader at collabora.com>
>> Thank you for your patch.
>>
>> I'm not a dmabuf expert, with that said the suggested change looks
>> reasonable to me.
> Hans, would you be able to test this on an IPU6-based device, and check
> the performance impact ? I don't expect expensive cache management
> operations on an x86 device.
>
> Bryan, could you do the same with camss ?

Just to let you know: I directly submitted this yesterday to start a 
discussion, however after a bit more thinking I figured we can probably 
do even better by moving these calls to MappedFrameBuffer. I wrote 
https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 
for that if you want to have look (will submit early next week).

>> One small code-style remark inline below.
>>
>>> ---
>>>   src/libcamera/software_isp/debayer_cpu.cpp | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>> index 077f7f4b..6c953b03 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>> @@ -11,7 +11,9 @@
>>>   
>>>   #include "debayer_cpu.h"
>>>   
>>> +#include <linux/dma-buf.h>
>>>   #include <stdlib.h>
>>> +#include <sys/ioctl.h>
>>>   #include <time.h>
>>>   
>>>   #include <libcamera/formats.h>
>>> @@ -733,6 +735,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>>   		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>>   	}
>>>   
>>> +	for (const FrameBuffer::Plane &plane : output->planes()) {
>>> +		const int fd = plane.fd.get();
>>> +		struct dma_buf_sync sync = { DMA_BUF_SYNC_START };
>>> +
>>> +		sync.flags |= DMA_BUF_SYNC_WRITE;
>> These 2 lines are a weird way to write:
>>
>> 		struct dma_buf_sync sync = { DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE };
>>
>> ?
>>
>>> +
>>> +		if (ioctl (fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>> +			LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>> +	}
>>> +
>>>   	green_ = params.green;
>>>   	red_ = swapRedBlueGains_ ? params.blue : params.red;
>>>   	blue_ = swapRedBlueGains_ ? params.red : params.blue;
>>> @@ -760,6 +772,16 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>>>   
>>>   	metadata.planes()[0].bytesused = out.planes()[0].size();
>>>   
>>> +	for (const FrameBuffer::Plane &plane : output->planes()) {
>>> +		const int fd = plane.fd.get();
>>> +		struct dma_buf_sync sync = { DMA_BUF_SYNC_END };
>>> +
>>> +		sync.flags |= DMA_BUF_SYNC_WRITE;
>> Same here, why not use:
>>
>> 		struct dma_buf_sync sync = { DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE };
>>
>> ?
>>
>>> +
>>> +		if (ioctl (fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)
>>> +			LOG(Debayer, Error) << "Syncing buffer FD " << fd << "failed: " << errno;
>>> +	}
>>> +
>>>   	/* Measure before emitting signals */
>>>   	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>>>   	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718