[{"id":36932,"web_url":"https://patchwork.libcamera.org/comment/36932/","msgid":"<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>","date":"2025-11-20T09:58:43","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n> Use the DmaSyncer handler to ensure the data from the buffer is\n> coherent. This is required by the spec - from\n> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> \n>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>> possible to guarantee coherency between the CPU-visible map and\n>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n>> to bracket any CPU access to give the kernel the chance to shuffle memory\n>> around if needed.\n> \n\nShouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> This was reported to fix glitches with the upcoming GPU-ISP, in which\n> case the accessed data in written by the GPU.\n> \n> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> could avoid that in the future by implementing direct dmabuf import in\n> qcam.\n> \n> Signed-off-by: Robert Mader \n> ---\n> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> 2 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n> index f31956ff0..0121c97ad 100644\n> --- a/src/apps/qcam/viewfinder_gl.cpp\n> +++ b/src/apps/qcam/viewfinder_gl.cpp\n> @@ -9,14 +9,16 @@\n> \n> #include \n> \n> +#include \n> +\n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> -#include \n> -\n> #include \"../common/image.h\"\n> \n> static const QList supportedFormats{\n> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> \t/* Stride of the first plane, in pixels. */\n> \tunsigned int stridePixels;\n> \n> +\tstd::vector dmaSyncers;\n> +\tfor (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> +\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> \tswitch (format_) {\n> \tcase libcamera::formats::NV12:\n> \tcase libcamera::formats::NV21:\n> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n> index 1a238922b..4d00f154d 100644\n> --- a/src/apps/qcam/viewfinder_qt.cpp\n> +++ b/src/apps/qcam/viewfinder_qt.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \n> \n> #include \n> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> \t\t\t * Otherwise, convert the format and release the frame\n> \t\t\t * buffer immediately.\n> \t\t\t */\n> +\t\t\tstd::vector dmaSyncers;\n> +\t\t\tfor (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> +\t\t\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> \t\t\tconverter_.convert(image, size, &image_);\n> \t\t}\n> \t}\n> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> \n> \t/* If we have an image, draw it, with black letterbox rectangles. */\n> \tif (!image_.isNull()) {\n> +\t\tif (buffer_) {\n> +\t\t\tstd::vector dmaSyncers;\n> +\t\t\tfor (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> +\t\t\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\t\t}\n> +\n> \t\tif (place_.width() < width()) {\n> \t\t\tQRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> \t\t\tpainter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A92C7BD80A\n\tfor ;\n\tThu, 20 Nov 2025 09:58:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF51A60A80;\n\tThu, 20 Nov 2025 10:58:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0F08606E6\n\tfor ;\n\tThu, 20 Nov 2025 10:58:49 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8DE5B5;\n\tThu, 20 Nov 2025 10:56:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iUhpyfsQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763632604;\n\tbh=QQBfGJZ/0Nz0iL1YxZua7kUu9v8+e6yTozZvXvQtqlc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=iUhpyfsQqSiLjXQnYa0g8ptSIaMVFG32Rp00k3Pi3h4p4Q3rOpwynaD0MM2ap/A2o\n\tkmQvUdACTTWpjnzo8xfa5DA3WdSsLSQvstcaL1f2yOr1ij/L5AHL8DaP0v0LWZvX4t\n\tTSv6V8zBcdWC9BVS1tr2kbezxj0qjakkfA0wxCjU=","Message-ID":"<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>","Date":"Thu, 20 Nov 2025 10:58:43 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251120094919.12138-1-robert.mader@collabora.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36933,"web_url":"https://patchwork.libcamera.org/comment/36933/","msgid":"<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>","date":"2025-11-20T10:14:05","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 20.11.25 10:58, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>> Use the DmaSyncer handler to ensure the data from the buffer is\n>> coherent. This is required by the spec - from\n>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>\n>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>>> possible to guarantee coherency between the CPU-visible map and\n>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n>>> to bracket any CPU access to give the kernel the chance to shuffle \n>>> memory\n>>> around if needed.\n>>\n>\n> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n\nWe discussed that in the thread for \"[PATCH] libcamera: debayer_cpu: \nSync output buffer\", which was eventually landed as \"libcamera: \ndebayer_cpu: Sync DMABUFs\" a year ago. I wrote the following commit back \nthen:\n\nhttps://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7\n\nHowever we eventually decided that more fine-grained control would be \nuseful, as the sync operation can be very expensive.\n\nWe could reconsider that now - but I think we should have a fix in 0.6 \neither way, and this here is the approach with the least churn.\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n>> case the accessed data in written by the GPU.\n>>\n>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n>> could avoid that in the future by implementing direct dmabuf import in\n>> qcam.\n>>\n>> Signed-off-by: Robert Mader \n>> ---\n>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/apps/qcam/viewfinder_gl.cpp \n>> b/src/apps/qcam/viewfinder_gl.cpp\n>> index f31956ff0..0121c97ad 100644\n>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>> @@ -9,14 +9,16 @@\n>>     #include \n>>   +#include \n>> +\n>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>> +\n>>   #include \n>>   #include \n>>   #include \n>>   #include \n>>   #include \n>>   -#include \n>> -\n>>   #include \"../common/image.h\"\n>>     static const QList supportedFormats{\n>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>       /* Stride of the first plane, in pixels. */\n>>       unsigned int stridePixels;\n>>   +    std::vector dmaSyncers;\n>> +    for (const libcamera::FrameBuffer::Plane &plane : \n>> buffer_->planes())\n>> +        dmaSyncers.emplace_back(plane.fd, \n>> libcamera::DmaSyncer::SyncType::Read);\n>> +\n>>       switch (format_) {\n>>       case libcamera::formats::NV12:\n>>       case libcamera::formats::NV21:\n>> diff --git a/src/apps/qcam/viewfinder_qt.cpp \n>> b/src/apps/qcam/viewfinder_qt.cpp\n>> index 1a238922b..4d00f154d 100644\n>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>> @@ -11,6 +11,7 @@\n>>   #include \n>>   #include \n>>   +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>   #include \n>>     #include \n>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer \n>> *buffer, Image *image)\n>>                * Otherwise, convert the format and release the frame\n>>                * buffer immediately.\n>>                */\n>> +            std::vector dmaSyncers;\n>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>> buffer->planes())\n>> +                dmaSyncers.emplace_back(plane.fd, \n>> libcamera::DmaSyncer::SyncType::Read);\n>> +\n>>               converter_.convert(image, size, &image_);\n>>           }\n>>       }\n>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>         /* If we have an image, draw it, with black letterbox \n>> rectangles. */\n>>       if (!image_.isNull()) {\n>> +        if (buffer_) {\n>> +            std::vector dmaSyncers;\n>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>> buffer_->planes())\n>> +                dmaSyncers.emplace_back(plane.fd, \n>> libcamera::DmaSyncer::SyncType::Read);\n>> +        }\n>> +\n>>           if (place_.width() < width()) {\n>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, \n>> height() };\n>>               painter.drawRect(rect);\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9F46CC0F1B\n\tfor ;\n\tThu, 20 Nov 2025 10:14:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D73B260A80;\n\tThu, 20 Nov 2025 11:14:13 +0100 (CET)","from sender3-pp-f112.zoho.com (sender3-pp-f112.zoho.com\n\t[136.143.184.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C430E606E6\n\tfor ;\n\tThu, 20 Nov 2025 11:14:12 +0100 (CET)","by mx.zohomail.com with SMTPS id 1763633648183581.2689072612092;\n\tThu, 20 Nov 2025 02:14:08 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"JP+rjRW9\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1763633649; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=KiEjLIkFygb2vD3fdTJNUQiGrwQYzT4/JqQr/JLxHSoJsemCeYjDFpOu5HLFhyYzaMf+ayNfp6bt8OOn25XeUi1LjBbTzBsHZsH9x3GqxNFksXMHft1pKBr8cmCGCzsv8vwLCI+v5cp5QnanXBij4UsEe2ORSeco7BlFRUZ4MYE=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1763633649;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=PbSLvjHM8B6pf0lTwg0Aj0oxBCzvDugwbbOWRiEmCrc=; \n\tb=ZIpV/FCL5qar+upDQfua3wxhIglmbxtCNAFDRWUMTEIO06+5aDj1XemNgLjrsl9ctW1cO7KLXNUE6GRwjayjLIb0GIShPw7CeFBhWqD1N6oeNZx6NKFWWwuZrdTnkPQKr4zSE0HcJpOH4lQbUEI4eVykRKd4Qoah/jiNu4Rvseg=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1763633649;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=PbSLvjHM8B6pf0lTwg0Aj0oxBCzvDugwbbOWRiEmCrc=;\n\tb=JP+rjRW912Cm8wRSGtT4sTvtIXCUG2rHiJ1dKtFatxpYkzGNdElRmgzIqdrTTW/u\n\tJUmvsJP4PBUROtFH8XWDeNB1RMBjOzcH+tmTihjEirmCXXqSuMLbiHQhHY9qIE7YWRs\n\tbD9Z7YlHV/2/QrFBCxa1TmReAqljd4PHzYJHlFG8=","Message-ID":"<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>","Date":"Thu, 20 Nov 2025 11:14:05 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36934,"web_url":"https://patchwork.libcamera.org/comment/36934/","msgid":"<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>","date":"2025-11-20T12:35:10","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n> On 20.11.25 10:58, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>>> Use the DmaSyncer handler to ensure the data from the buffer is\n>>> coherent. This is required by the spec - from\n>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>>\n>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>>>> possible to guarantee coherency between the CPU-visible map and\n>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n>>>> to bracket any CPU access to give the kernel the chance to shuffle memory\n>>>> around if needed.\n>>>\n>>\n>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n> \n> We discussed that in the thread for \"[PATCH] libcamera: debayer_cpu: Sync output buffer\", which was eventually landed as \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the following commit back then:\n> \n> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7\n> \n> However we eventually decided that more fine-grained control would be useful, as the sync operation can be very expensive.\n> \n> We could reconsider that now - but I think we should have a fix in 0.6 either way, and this here is the approach with the least churn.\n\nBut every use of MappedFrameBuffer / Image, I think, pretty much implies that data\nwill be accessed from the CPU, no?\n\nIt's also not clear to me why the same change is not done in the `cam` app as well?\n\nOn a related note, pipewire marks the libcamera buffers as SPA_DATA_FLAG_MAPPABLE, implying\nthat they can just be mmapped and used. I assume this has to change? There does not seem to\nbe anything in the application developers' guide explicitly mentioning when/how/if dma buf\nsync is needed; it would be good to make the requirements crystal clear.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n>>> case the accessed data in written by the GPU.\n>>>\n>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n>>> could avoid that in the future by implementing direct dmabuf import in\n>>> qcam.\n>>>\n>>> Signed-off-by: Robert Mader \n>>> ---\n>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n>>> index f31956ff0..0121c97ad 100644\n>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>>> @@ -9,14 +9,16 @@\n>>>     #include \n>>>   +#include \n>>> +\n>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>> +\n>>>   #include \n>>>   #include \n>>>   #include \n>>>   #include \n>>>   #include \n>>>   -#include \n>>> -\n>>>   #include \"../common/image.h\"\n>>>     static const QList supportedFormats{\n>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>>       /* Stride of the first plane, in pixels. */\n>>>       unsigned int stridePixels;\n>>>   +    std::vector dmaSyncers;\n>>> +    for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>> +        dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>> +\n>>>       switch (format_) {\n>>>       case libcamera::formats::NV12:\n>>>       case libcamera::formats::NV21:\n>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n>>> index 1a238922b..4d00f154d 100644\n>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>>> @@ -11,6 +11,7 @@\n>>>   #include \n>>>   #include \n>>>   +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>   #include \n>>>     #include \n>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n>>>                * Otherwise, convert the format and release the frame\n>>>                * buffer immediately.\n>>>                */\n>>> +            std::vector dmaSyncers;\n>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>> +\n>>>               converter_.convert(image, size, &image_);\n>>>           }\n>>>       }\n>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>>         /* If we have an image, draw it, with black letterbox rectangles. */\n>>>       if (!image_.isNull()) {\n>>> +        if (buffer_) {\n>>> +            std::vector dmaSyncers;\n>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>> +        }\n>>> +\n>>>           if (place_.width() < width()) {\n>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n>>>               painter.drawRect(rect);\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8FEBEC3330\n\tfor ;\n\tThu, 20 Nov 2025 12:35:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CE1460A8B;\n\tThu, 20 Nov 2025 13:35:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C55316069A\n\tfor ;\n\tThu, 20 Nov 2025 13:35:18 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 386A6C77;\n\tThu, 20 Nov 2025 13:33:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZET9XLiZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763641993;\n\tbh=3oAOm0UV3NmJKH1gYOSvoaW3tBP/UanIm4VTEJzbfkY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ZET9XLiZPFLufeNC1OjQUeBcyPXYawvngDeDrRwOmx2kP8U+QOIklJOyj8XtGlfZr\n\tjn7cpmU9TqUDJKyB7m4QtbzyEP6Fgv/wlFSuQU/06Yvfje8fM3ytTz3SMq+qFqyWV2\n\tfgAb5g5xkV+DA56WhavtME5tHyeBrFYsysxzTW/E=","Message-ID":"<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>","Date":"Thu, 20 Nov 2025 13:35:10 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36935,"web_url":"https://patchwork.libcamera.org/comment/36935/","msgid":"","date":"2025-11-20T13:26:31","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Hi,\n\nOn 20.11.25 13:35, Barnabás Pőcze wrote:\n> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n>>> Hi\n>>>\n>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n>>>> coherent. This is required by the spec - from\n>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>>>\n>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>>>>> possible to guarantee coherency between the CPU-visible map and\n>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be \n>>>>> used\n>>>>> to bracket any CPU access to give the kernel the chance to shuffle \n>>>>> memory\n>>>>> around if needed.\n>>>>\n>>>\n>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n>>\n>> We discussed that in the thread for \"[PATCH] libcamera: debayer_cpu: \n>> Sync output buffer\", which was eventually landed as \"libcamera: \n>> debayer_cpu: Sync DMABUFs\" a year ago. I wrote the following commit \n>> back then:\n>>\n>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 \n>>\n>>\n>> However we eventually decided that more fine-grained control would be \n>> useful, as the sync operation can be very expensive.\n>>\n>> We could reconsider that now - but I think we should have a fix in \n>> 0.6 either way, and this here is the approach with the least churn.\n>\n> But every use of MappedFrameBuffer / Image, I think, pretty much \n> implies that data\n> will be accessed from the CPU, no?\n\nGenerally yes, but it's not necessarily optimal in every situation, \nwhich is why the kernel doesn't hide away the sync in the map call in \nthe first place.\n\nIt can e.g. be a good choice to keep a buffer mapped and only sync it \nwhen required, reducing some overhead. I'm not aware of any such usage \nin libcamera yet, however it's one of the things that we *shoud* do in \nthe SW/GPU-ISP eventually, IMO, and I plan to work on that once the main \nseries has landed (Gstreamer even has helpers for that, see e.g. \nhttps://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n\nIn the end it's a preference of choice I'd say.\n\n>\n> It's also not clear to me why the same change is not done in the `cam` \n> app as well?\nShould certainly be done in a follow-up, assuming SDL uses an \nupload-from-CPU approach as well. Haven't looked into it - I just \nquickly typed this patch down as it fixed glitches Hans was seeing on \nIPU7 (IIRC - or was it IPU6).\n> On a related note, pipewire marks the libcamera buffers as \n> SPA_DATA_FLAG_MAPPABLE, implying\n> that they can just be mmapped and used. I assume this has to change? \n> There does not seem to\n> be anything in the application developers' guide explicitly mentioning \n> when/how/if dma buf\n> sync is needed; it would be good to make the requirements crystal clear.\nPipewire clients - or really any client doing anything with dmabufs -  \nindeed have to know that if they use buffers with the SPA_DATA_DmaBuf \ntype they have to sync them before accessing from the CPU. It's a \nfundamental part of the kernel API. When using Gstreamer that is hidden \nwithin the GstBuffer implementation, but other clients need to handle it \nindeed manually. In fact a quick look suggests that libwebrtc doesn't do \nit yet, which would also need to get fixed.\n>\n> Regards,\n> Barnabás Pőcze\nRegards\n>\n>>\n>>>\n>>>\n>>> Regards,\n>>> Barnabás Pőcze\n>>>\n>>>\n>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n>>>> case the accessed data in written by the GPU.\n>>>>\n>>>> Note that in the GPU-ISP case we are effectively seeing a \n>>>> round-trip of\n>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n>>>> could avoid that in the future by implementing direct dmabuf import in\n>>>> qcam.\n>>>>\n>>>> Signed-off-by: Robert Mader \n>>>> ---\n>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp \n>>>> b/src/apps/qcam/viewfinder_gl.cpp\n>>>> index f31956ff0..0121c97ad 100644\n>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>>>> @@ -9,14 +9,16 @@\n>>>>     #include \n>>>>   +#include \n>>>> +\n>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>> +\n>>>>   #include \n>>>>   #include \n>>>>   #include \n>>>>   #include \n>>>>   #include \n>>>>   -#include \n>>>> -\n>>>>   #include \"../common/image.h\"\n>>>>     static const QList supportedFormats{\n>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>>>       /* Stride of the first plane, in pixels. */\n>>>>       unsigned int stridePixels;\n>>>>   +    std::vector dmaSyncers;\n>>>> +    for (const libcamera::FrameBuffer::Plane &plane : \n>>>> buffer_->planes())\n>>>> +        dmaSyncers.emplace_back(plane.fd, \n>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>> +\n>>>>       switch (format_) {\n>>>>       case libcamera::formats::NV12:\n>>>>       case libcamera::formats::NV21:\n>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp \n>>>> b/src/apps/qcam/viewfinder_qt.cpp\n>>>> index 1a238922b..4d00f154d 100644\n>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>>>> @@ -11,6 +11,7 @@\n>>>>   #include \n>>>>   #include \n>>>>   +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>   #include \n>>>>     #include \n>>>> @@ -114,6 +115,10 @@ void \n>>>> ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n>>>>                * Otherwise, convert the format and release the frame\n>>>>                * buffer immediately.\n>>>>                */\n>>>> +            std::vector dmaSyncers;\n>>>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>>>> buffer->planes())\n>>>> +                dmaSyncers.emplace_back(plane.fd, \n>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>> +\n>>>>               converter_.convert(image, size, &image_);\n>>>>           }\n>>>>       }\n>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>>>         /* If we have an image, draw it, with black letterbox \n>>>> rectangles. */\n>>>>       if (!image_.isNull()) {\n>>>> +        if (buffer_) {\n>>>> +            std::vector dmaSyncers;\n>>>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>>>> buffer_->planes())\n>>>> +                dmaSyncers.emplace_back(plane.fd, \n>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>> +        }\n>>>> +\n>>>>           if (place_.width() < width()) {\n>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, \n>>>> height() };\n>>>>               painter.drawRect(rect);\n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 01A89C3335\n\tfor ;\n\tThu, 20 Nov 2025 13:26:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 580A760A80;\n\tThu, 20 Nov 2025 14:26:42 +0100 (CET)","from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com\n\t[136.143.188.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0421F6069A\n\tfor ;\n\tThu, 20 Nov 2025 14:26:40 +0100 (CET)","by mx.zohomail.com with SMTPS id 17636451964641005.764271777417;\n\tThu, 20 Nov 2025 05:26:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"VnMUgd+G\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1763645198; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=PHeoJhlWovwRjk7UKcOKy1ik89hwSbtSMUkjC4f7Uud0AOdrUB7TtK5qlBBKvW0ABssShcz4sJO571umfkvZo63K+DnwYURrYwwWtYj98Q63YdNKJjabc5X5iTTb1PQfD+lUFl+m7ulpEr5Zv0o+YqhtNy9u5UcONilT/cKOeTo=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1763645198;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=l+jwFeydQgIYENmrpA2NOHJCd1zJ1S6C4k86GMyTXTM=; \n\tb=hq1pBB/yw4Rmibn36BN5yZH10JIENbv5Aj7akU+Q/RUWqIJkp/F+X1Ux97P2ePpn4j80n/izJDAQNBGXE1/YahiVB1EiGG2wKMFrnVsRALOO4ydQtr6IxGH85kGYTltCNkAiQG0OTceRHvzF0sKaVUDRtvOwT4+6m4N2NzrYNsI=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1763645198;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=l+jwFeydQgIYENmrpA2NOHJCd1zJ1S6C4k86GMyTXTM=;\n\tb=VnMUgd+Gp1aMlKg2JmHH1NY+2CUMLEKFCSkQg0wc8PoqcpC1V6yDLvfpIDFkw6xJ\n\tWiWNJhIgSeshNz9YRTSnHH4QUNN0OdbkdO23xCwAfsFm2iZkXZ4Vv5hFG3qyQjmTMo7\n\tcG5WE6FNh0TdfPyYnosa4wlrRQn+TgtUWV+v9vQk=","Message-ID":"","Date":"Thu, 20 Nov 2025 14:26:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36936,"web_url":"https://patchwork.libcamera.org/comment/36936/","msgid":"","date":"2025-11-20T13:54:03","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 20. 14:26 keltezéssel, Robert Mader írta:\n> Hi,\n> \n> On 20.11.25 13:35, Barnabás Pőcze wrote:\n>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n>>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n>>>> Hi\n>>>>\n>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n>>>>> coherent. This is required by the spec - from\n>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>>>>\n>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>>>>>> possible to guarantee coherency between the CPU-visible map and\n>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory\n>>>>>> around if needed.\n>>>>>\n>>>>\n>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n>>>\n>>> We discussed that in the thread for \"[PATCH] libcamera: debayer_cpu: Sync output buffer\", which was eventually landed as \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the following commit back then:\n>>>\n>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7\n>>>\n>>> However we eventually decided that more fine-grained control would be useful, as the sync operation can be very expensive.\n>>>\n>>> We could reconsider that now - but I think we should have a fix in 0.6 either way, and this here is the approach with the least churn.\n>>\n>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data\n>> will be accessed from the CPU, no?\n> \n> Generally yes, but it's not necessarily optimal in every situation, which is why the kernel doesn't hide away the sync in the map call in the first place.\n> \n> It can e.g. be a good choice to keep a buffer mapped and only sync it when required, reducing some overhead. I'm not aware of any such usage in libcamera yet, however it's one of the things that we *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on that once the main series has landed (Gstreamer even has helpers for that, see e.g. https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n> \n> In the end it's a preference of choice I'd say.\n\nIt seems to me that extending `MappedFrameBuffer` / `Image` is then the way forward, not\nnecessarily adding it to the constructor/destructor but maybe adding extra member functions.\n\n\n> \n>>\n>> It's also not clear to me why the same change is not done in the `cam` app as well?\n> Should certainly be done in a follow-up, assuming SDL uses an upload-from-CPU approach as well. Haven't looked into it - I just quickly typed this patch down as it fixed glitches Hans was seeing on IPU7 (IIRC - or was it IPU6).\n\nIt is passed the mmap-ed pointer from an `Image` object, so I can't imagine it does anything different.\n\n\n>> On a related note, pipewire marks the libcamera buffers as SPA_DATA_FLAG_MAPPABLE, implying\n>> that they can just be mmapped and used. I assume this has to change? There does not seem to\n>> be anything in the application developers' guide explicitly mentioning when/how/if dma buf\n>> sync is needed; it would be good to make the requirements crystal clear.\n> Pipewire clients - or really any client doing anything with dmabufs - indeed have to know that if they use buffers with the SPA_DATA_DmaBuf type they have to sync them before accessing from the CPU. It's a fundamental part of the kernel API. When using Gstreamer that is hidden within the GstBuffer implementation, but other clients need to handle it indeed manually.\n\nSo that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not correct there.\n\n\n> In fact a quick look suggests that libwebrtc doesn't do it yet, which would also need to get fixed.\n\nAs far as I can tell it does:\n\n * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476\n * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99\n\nat least the libwebrtc copy in firefox. It actually mmap/munmap every frame, which is likely not ideal.\n\n\n>>\n>> Regards,\n>> Barnabás Pőcze\n> Regards\n>>\n>>>\n>>>>\n>>>>\n>>>> Regards,\n>>>> Barnabás Pőcze\n>>>>\n>>>>\n>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n>>>>> case the accessed data in written by the GPU.\n>>>>>\n>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n>>>>> could avoid that in the future by implementing direct dmabuf import in\n>>>>> qcam.\n>>>>>\n>>>>> Signed-off-by: Robert Mader \n>>>>> ---\n>>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n>>>>> index f31956ff0..0121c97ad 100644\n>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>>>>> @@ -9,14 +9,16 @@\n>>>>>     #include \n>>>>>   +#include \n>>>>> +\n>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>> +\n>>>>>   #include \n>>>>>   #include \n>>>>>   #include \n>>>>>   #include \n>>>>>   #include \n>>>>>   -#include \n>>>>> -\n>>>>>   #include \"../common/image.h\"\n>>>>>     static const QList supportedFormats{\n>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>>>>       /* Stride of the first plane, in pixels. */\n>>>>>       unsigned int stridePixels;\n>>>>>   +    std::vector dmaSyncers;\n>>>>> +    for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>>>> +        dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>> +\n>>>>>       switch (format_) {\n>>>>>       case libcamera::formats::NV12:\n>>>>>       case libcamera::formats::NV21:\n>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n>>>>> index 1a238922b..4d00f154d 100644\n>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>>>>> @@ -11,6 +11,7 @@\n>>>>>   #include \n>>>>>   #include \n>>>>>   +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>>   #include \n>>>>>     #include \n>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n>>>>>                * Otherwise, convert the format and release the frame\n>>>>>                * buffer immediately.\n>>>>>                */\n>>>>> +            std::vector dmaSyncers;\n>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>> +\n>>>>>               converter_.convert(image, size, &image_);\n>>>>>           }\n>>>>>       }\n>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>>>>         /* If we have an image, draw it, with black letterbox rectangles. */\n>>>>>       if (!image_.isNull()) {\n>>>>> +        if (buffer_) {\n>>>>> +            std::vector dmaSyncers;\n>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>> +        }\n>>>>> +\n>>>>>           if (place_.width() < width()) {\n>>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n>>>>>               painter.drawRect(rect);\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 700EFC3336\n\tfor ;\n\tThu, 20 Nov 2025 13:54:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C2EE60A80;\n\tThu, 20 Nov 2025 14:54:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E84586069A\n\tfor ;\n\tThu, 20 Nov 2025 14:54:07 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F4B3C77;\n\tThu, 20 Nov 2025 14:52:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FR0qDlVP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763646722;\n\tbh=ylK9uXEK7XQhDc9wAc695gLjuvHEJI1GtzQY0dS8T2M=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=FR0qDlVPHd7v6gbkC+PsBVhowVrGVgHsYSQKT+ijqY6eQm22qbPZHzh6YSoFemp+t\n\tt262lGVvwSgSndxD0S9pjmDX7vaE7OTNlVmixvhacuUDh4ZiZNLQqRYMTpbNXJeur7\n\t6niNfWh9L/p66SvLErXuub327K3SSQLl+jIQZ/I4=","Message-ID":"","Date":"Thu, 20 Nov 2025 14:54:03 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36945,"web_url":"https://patchwork.libcamera.org/comment/36945/","msgid":"<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>","date":"2025-11-20T14:50:42","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 20.11.25 14:54, Barnabás Pőcze wrote:\n> 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta:\n>> Hi,\n>>\n>> On 20.11.25 13:35, Barnabás Pőcze wrote:\n>>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n>>>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n>>>>> Hi\n>>>>>\n>>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>>>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n>>>>>> coherent. This is required by the spec - from\n>>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>>>>>\n>>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not \n>>>>>>> always\n>>>>>>> possible to guarantee coherency between the CPU-visible map and\n>>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must \n>>>>>>> be used\n>>>>>>> to bracket any CPU access to give the kernel the chance to \n>>>>>>> shuffle memory\n>>>>>>> around if needed.\n>>>>>>\n>>>>>\n>>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n>>>>\n>>>> We discussed that in the thread for \"[PATCH] libcamera: \n>>>> debayer_cpu: Sync output buffer\", which was eventually landed as \n>>>> \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the \n>>>> following commit back then:\n>>>>\n>>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 \n>>>>\n>>>>\n>>>> However we eventually decided that more fine-grained control would \n>>>> be useful, as the sync operation can be very expensive.\n>>>>\n>>>> We could reconsider that now - but I think we should have a fix in \n>>>> 0.6 either way, and this here is the approach with the least churn.\n>>>\n>>> But every use of MappedFrameBuffer / Image, I think, pretty much \n>>> implies that data\n>>> will be accessed from the CPU, no?\n>>\n>> Generally yes, but it's not necessarily optimal in every situation, \n>> which is why the kernel doesn't hide away the sync in the map call in \n>> the first place.\n>>\n>> It can e.g. be a good choice to keep a buffer mapped and only sync it \n>> when required, reducing some overhead. I'm not aware of any such \n>> usage in libcamera yet, however it's one of the things that we \n>> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on \n>> that once the main series has landed (Gstreamer even has helpers for \n>> that, see e.g. \n>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n>>\n>> In the end it's a preference of choice I'd say.\n>\n> It seems to me that extending `MappedFrameBuffer` / `Image` is then \n> the way forward, not\n> necessarily adding it to the constructor/destructor but maybe adding \n> extra member functions.\nUnfortunately I most likely won't have time to into that again the next \ntwo weeks, so if we want qcam fixed in 0.6, somebody else would need to \ntake that over.\n>\n>>\n>>>\n>>> It's also not clear to me why the same change is not done in the \n>>> `cam` app as well?\n>> Should certainly be done in a follow-up, assuming SDL uses an \n>> upload-from-CPU approach as well. Haven't looked into it - I just \n>> quickly typed this patch down as it fixed glitches Hans was seeing on \n>> IPU7 (IIRC - or was it IPU6).\n>\n> It is passed the mmap-ed pointer from an `Image` object, so I can't \n> imagine it does anything different.\n>\n>>> On a related note, pipewire marks the libcamera buffers as \n>>> SPA_DATA_FLAG_MAPPABLE, implying\n>>> that they can just be mmapped and used. I assume this has to change? \n>>> There does not seem to\n>>> be anything in the application developers' guide explicitly \n>>> mentioning when/how/if dma buf\n>>> sync is needed; it would be good to make the requirements crystal \n>>> clear.\n>> Pipewire clients - or really any client doing anything with dmabufs - \n>> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf \n>> type they have to sync them before accessing from the CPU. It's a \n>> fundamental part of the kernel API. When using Gstreamer that is \n>> hidden within the GstBuffer implementation, but other clients need to \n>> handle it indeed manually.\n>\n> So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not \n> correct there.\nI'd say it's still close enough. AFAIK not having the flag is usually \ninterpreted as \"you have to import via GL/VK\" - like in the case of \nmodifiers. But that should probably be discussed in a PW issue.\n\n>\n>> In fact a quick look suggests that libwebrtc doesn't do it yet, which \n>> would also need to get fixed.\n>\n> As far as I can tell it does:\n>\n>   * \n> https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476\n>   * \n> https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99\n>\n> at least the libwebrtc copy in firefox. It actually mmap/munmap every \n> frame, which is likely not ideal.\n\nAh nice, thanks, missed that.\n\nGood, so both libwebrtc and Gstreamer based apps are fine.\n\n>>>\n>>> Regards,\n>>> Barnabás Pőcze\n>> Regards\n>>>\n>>>>\n>>>>>\n>>>>>\n>>>>> Regards,\n>>>>> Barnabás Pőcze\n>>>>>\n>>>>>\n>>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in \n>>>>>> which\n>>>>>> case the accessed data in written by the GPU.\n>>>>>>\n>>>>>> Note that in the GPU-ISP case we are effectively seeing a \n>>>>>> round-trip of\n>>>>>> the buffer contents - from GPU synced to CPU, copied to GPU \n>>>>>> again. We\n>>>>>> could avoid that in the future by implementing direct dmabuf \n>>>>>> import in\n>>>>>> qcam.\n>>>>>>\n>>>>>> Signed-off-by: Robert Mader \n>>>>>> ---\n>>>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp \n>>>>>> b/src/apps/qcam/viewfinder_gl.cpp\n>>>>>> index f31956ff0..0121c97ad 100644\n>>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>>>>>> @@ -9,14 +9,16 @@\n>>>>>>     #include \n>>>>>>   +#include \n>>>>>> +\n>>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>>> +\n>>>>>>   #include \n>>>>>>   #include \n>>>>>>   #include \n>>>>>>   #include \n>>>>>>   #include \n>>>>>>   -#include \n>>>>>> -\n>>>>>>   #include \"../common/image.h\"\n>>>>>>     static const QList supportedFormats{\n>>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>>>>>       /* Stride of the first plane, in pixels. */\n>>>>>>       unsigned int stridePixels;\n>>>>>>   +    std::vector dmaSyncers;\n>>>>>> +    for (const libcamera::FrameBuffer::Plane &plane : \n>>>>>> buffer_->planes())\n>>>>>> +        dmaSyncers.emplace_back(plane.fd, \n>>>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>>>> +\n>>>>>>       switch (format_) {\n>>>>>>       case libcamera::formats::NV12:\n>>>>>>       case libcamera::formats::NV21:\n>>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp \n>>>>>> b/src/apps/qcam/viewfinder_qt.cpp\n>>>>>> index 1a238922b..4d00f154d 100644\n>>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>>>>>> @@ -11,6 +11,7 @@\n>>>>>>   #include \n>>>>>>   #include \n>>>>>>   +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>>>   #include \n>>>>>>     #include \n>>>>>> @@ -114,6 +115,10 @@ void \n>>>>>> ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n>>>>>>                * Otherwise, convert the format and release the frame\n>>>>>>                * buffer immediately.\n>>>>>>                */\n>>>>>> +            std::vector dmaSyncers;\n>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>>>>>> buffer->planes())\n>>>>>> +                dmaSyncers.emplace_back(plane.fd, \n>>>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>>>> +\n>>>>>>               converter_.convert(image, size, &image_);\n>>>>>>           }\n>>>>>>       }\n>>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>>>>>         /* If we have an image, draw it, with black letterbox \n>>>>>> rectangles. */\n>>>>>>       if (!image_.isNull()) {\n>>>>>> +        if (buffer_) {\n>>>>>> +            std::vector dmaSyncers;\n>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : \n>>>>>> buffer_->planes())\n>>>>>> +                dmaSyncers.emplace_back(plane.fd, \n>>>>>> libcamera::DmaSyncer::SyncType::Read);\n>>>>>> +        }\n>>>>>> +\n>>>>>>           if (place_.width() < width()) {\n>>>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, \n>>>>>> height() };\n>>>>>>               painter.drawRect(rect);\n>>>>>\n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5A724C3330\n\tfor ;\n\tThu, 20 Nov 2025 14:50:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7894460A8B;\n\tThu, 20 Nov 2025 15:50:55 +0100 (CET)","from sender3-pp-f112.zoho.com (sender3-pp-f112.zoho.com\n\t[136.143.184.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D86346069A\n\tfor ;\n\tThu, 20 Nov 2025 15:50:53 +0100 (CET)","by mx.zohomail.com with SMTPS id 1763650247857107.3436356888925;\n\tThu, 20 Nov 2025 06:50:47 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"Ulp0tnr+\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1763650250; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=jjnVCgwnSpyj84u1dCA/Rg2AmX+O0sxkaqqVod8EJ2dLJTPiLnb4iRPXOG4d4qN1hAbbEodbupLFXS+m5yPZLDYUttmDbRgh/jeeHFZBSM6SaHsDuSgiKADnl+plDR/hRpo8zIDeh26iT4bTDSWkM3lp8+FHACjDiQ+kKowcNXQ=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1763650250;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=/kMsYh1BdFgDnYZk5uhssPrPUatseqkduWZUPMAzJ+s=; \n\tb=lFKoZ71LKFZKFJlJfh+v5+Or2ZrExH31zIVNuwUz4PwFmkGhm56m6SHBB+adS0CroH+MYGi1aq1R1dtwDpw9zZ7LzCsKaFc9nl6VC7MkysWY8+CNftKg/RcDv1JJsT/OBmwHB2FZlcqOLIWxKajv/2NclMT/BSoixNwz18bL1xw=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1763650250;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=/kMsYh1BdFgDnYZk5uhssPrPUatseqkduWZUPMAzJ+s=;\n\tb=Ulp0tnr+/fg0L20Bj4ioMbAiVLk1wNrPmmafV73uykgQC2wT35e5yMqaGLTb5FWl\n\tUL+9jhCCKk4T3P2602stzZjQUc6vEocBKR6sPvM6iJZul679P/lz/NpHyewSDx4DTzc\n\tUZLYlCPmB9j+FJW2jSJiQYuPzVzq41sVHEDcz6eE=","Message-ID":"<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>","Date":"Thu, 20 Nov 2025 15:50:42 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>\n\t\n\t","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36964,"web_url":"https://patchwork.libcamera.org/comment/36964/","msgid":"<20251121033143.GB9186@pendragon.ideasonboard.com>","date":"2025-11-21T03:31:43","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 20, 2025 at 10:49:19AM +0100, Robert Mader wrote:\n> Use the DmaSyncer handler to ensure the data from the buffer is\n> coherent. This is required by the spec - from\n> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> \n> > When a DMA buffer is accessed from the CPU via mmap, it is not always\n> > possible to guarantee coherency between the CPU-visible map and\n> > underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> > to bracket any CPU access to give the kernel the chance to shuffle memory\n> > around if needed.\n> \n> This was reported to fix glitches with the upcoming GPU-ISP, in which\n> case the accessed data in written by the GPU.\n> \n> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> could avoid that in the future by implementing direct dmabuf import in\n> qcam.\n> \n> Signed-off-by: Robert Mader \n> ---\n> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> 2 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n> index f31956ff0..0121c97ad 100644\n> --- a/src/apps/qcam/viewfinder_gl.cpp\n> +++ b/src/apps/qcam/viewfinder_gl.cpp\n> @@ -9,14 +9,16 @@\n> \n> #include \n> \n> +#include \n> +\n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n\nAs the name implies, this is an internal header. It should not be used\nin qcam.\n\n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> -#include \n> -\n> #include \"../common/image.h\"\n> \n> static const QList supportedFormats{\n> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> \t/* Stride of the first plane, in pixels. */\n> \tunsigned int stridePixels;\n> \n> +\tstd::vector dmaSyncers;\n> +\tfor (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> +\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> \tswitch (format_) {\n> \tcase libcamera::formats::NV12:\n> \tcase libcamera::formats::NV21:\n> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n> index 1a238922b..4d00f154d 100644\n> --- a/src/apps/qcam/viewfinder_qt.cpp\n> +++ b/src/apps/qcam/viewfinder_qt.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \n> \n> #include \n> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> \t\t\t * Otherwise, convert the format and release the frame\n> \t\t\t * buffer immediately.\n> \t\t\t */\n> +\t\t\tstd::vector dmaSyncers;\n> +\t\t\tfor (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> +\t\t\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> \t\t\tconverter_.convert(image, size, &image_);\n> \t\t}\n> \t}\n> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> \n> \t/* If we have an image, draw it, with black letterbox rectangles. */\n> \tif (!image_.isNull()) {\n> +\t\tif (buffer_) {\n> +\t\t\tstd::vector dmaSyncers;\n> +\t\t\tfor (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> +\t\t\t\tdmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\t\t}\n> +\n> \t\tif (place_.width() < width()) {\n> \t\t\tQRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> \t\t\tpainter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 35529C3334\n\tfor ;\n\tFri, 21 Nov 2025 03:32:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 754A2606A0;\n\tFri, 21 Nov 2025 04:32:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 977CA606A0\n\tfor ;\n\tFri, 21 Nov 2025 04:32:09 +0100 (CET)","from pendragon.ideasonboard.com (fs276ed015.tkyc509.ap.nuro.jp\n\t[39.110.208.21])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 78499E7C;\n\tFri, 21 Nov 2025 04:30:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"M4dhQUSG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763695803;\n\tbh=DoYcPVUAzKswRtSYzLfTva/4LS9aNyL1kGZTHRpvnYQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M4dhQUSGSUDzfy7DCZbKz4MEEFPza6KZYmvmd2QwsUcfO2X0FosvkBWWOkSB/Jmss\n\tPmssgXkEn+eHKLfwegoyC1Dd6xacvx67g3ZmXMRMT6bshPBcHQgf4GLGBF//ev3+bK\n\thV6DKtJ5meggYuDzFZKSQeLlEVJWukYuh1AyN0/c=","Date":"Fri, 21 Nov 2025 12:31:43 +0900","From":"Laurent Pinchart ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Sync buffers before import","Message-ID":"<20251121033143.GB9186@pendragon.ideasonboard.com>","References":"<20251120094919.12138-1-robert.mader@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251120094919.12138-1-robert.mader@collabora.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36965,"web_url":"https://patchwork.libcamera.org/comment/36965/","msgid":"<20251121035205.GC9186@pendragon.ideasonboard.com>","date":"2025-11-21T03:52:05","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote:\n> On 20.11.25 14:54, Barnabás Pőcze wrote:\n> > 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta:\n> >> On 20.11.25 13:35, Barnabás Pőcze wrote:\n> >>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n> >>>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n> >>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n> >>>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n> >>>>>> coherent. This is required by the spec - from\n> >>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> >>>>>>\n> >>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n> >>>>>>> possible to guarantee coherency between the CPU-visible map and\n> >>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> >>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory\n> >>>>>>> around if needed.\n> >>>>>\n> >>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n> >>>>\n> >>>> We discussed that in the thread for \"[PATCH] libcamera: \n> >>>> debayer_cpu: Sync output buffer\", which was eventually landed as \n> >>>> \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the \n> >>>> following commit back then:\n> >>>>\n> >>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7 \n> >>>>\n> >>>> However we eventually decided that more fine-grained control would \n> >>>> be useful, as the sync operation can be very expensive.\n> >>>>\n> >>>> We could reconsider that now - but I think we should have a fix in \n> >>>> 0.6 either way, and this here is the approach with the least churn.\n> >>>\n> >>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data\n> >>> will be accessed from the CPU, no?\n> >>\n> >> Generally yes, but it's not necessarily optimal in every situation, \n> >> which is why the kernel doesn't hide away the sync in the map call in \n> >> the first place.\n> >>\n> >> It can e.g. be a good choice to keep a buffer mapped and only sync it \n> >> when required, reducing some overhead. I'm not aware of any such \n> >> usage in libcamera yet, however it's one of the things that we \n> >> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on \n\nIf you're talking about the images produced by the GPU ISP, then I think\nthe buffers should not be mapped by libcamera at all.\n\n> >> that once the main series has landed (Gstreamer even has helpers for \n> >> that, see e.g. \n> >> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n> >>\n> >> In the end it's a preference of choice I'd say.\n> >\n> > It seems to me that extending `MappedFrameBuffer` / `Image` is then \n> > the way forward, not\n> > necessarily adding it to the constructor/destructor but maybe adding \n> > extra member functions.\n\nThat sounds like a good idea. Especially given that qcam shouldn't use\nthe internal dma_buf_allocator.h header, implementing cache sync in the\nImage class would be cleaner.\n\n> Unfortunately I most likely won't have time to into that again the next \n> two weeks, so if we want qcam fixed in 0.6, somebody else would need to \n> take that over.\n> \n> >>> It's also not clear to me why the same change is not done in the \n> >>> `cam` app as well?\n> >> \n> >> Should certainly be done in a follow-up, assuming SDL uses an \n> >> upload-from-CPU approach as well. Haven't looked into it - I just \n> >> quickly typed this patch down as it fixed glitches Hans was seeing on \n> >> IPU7 (IIRC - or was it IPU6).\n> >\n> > It is passed the mmap-ed pointer from an `Image` object, so I can't \n> > imagine it does anything different.\n> >\n> >>> On a related note, pipewire marks the libcamera buffers as \n> >>> SPA_DATA_FLAG_MAPPABLE, implying\n> >>> that they can just be mmapped and used. I assume this has to change? \n> >>> There does not seem to\n> >>> be anything in the application developers' guide explicitly \n> >>> mentioning when/how/if dma buf\n> >>> sync is needed; it would be good to make the requirements crystal \n> >>> clear.\n> >> \n> >> Pipewire clients - or really any client doing anything with dmabufs - \n> >> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf \n> >> type they have to sync them before accessing from the CPU. It's a \n> >> fundamental part of the kernel API. When using Gstreamer that is \n> >> hidden within the GstBuffer implementation, but other clients need to \n> >> handle it indeed manually.\n> >\n> > So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not \n> > correct there.\n> \n> I'd say it's still close enough. AFAIK not having the flag is usually \n> interpreted as \"you have to import via GL/VK\" - like in the case of \n> modifiers. But that should probably be discussed in a PW issue.\n> \n> >> In fact a quick look suggests that libwebrtc doesn't do it yet, which \n> >> would also need to get fixed.\n> >\n> > As far as I can tell it does:\n> >\n> >   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476\n> >   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99\n> >\n> > at least the libwebrtc copy in firefox. It actually mmap/munmap every \n> > frame, which is likely not ideal.\n> \n> Ah nice, thanks, missed that.\n> \n> Good, so both libwebrtc and Gstreamer based apps are fine.\n> \n> >>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n> >>>>>> case the accessed data in written by the GPU.\n> >>>>>>\n> >>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> >>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> >>>>>> could avoid that in the future by implementing direct dmabuf import in\n> >>>>>> qcam.\n> >>>>>>\n> >>>>>> Signed-off-by: Robert Mader \n> >>>>>> ---\n> >>>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> >>>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> >>>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp \n> >>>>>> b/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>> index f31956ff0..0121c97ad 100644\n> >>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>> @@ -9,14 +9,16 @@\n> >>>>>> #include \n> >>>>>> +#include \n> >>>>>> +\n> >>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> >>>>>> +\n> >>>>>> #include \n> >>>>>> #include \n> >>>>>> #include \n> >>>>>> #include \n> >>>>>> #include \n> >>>>>> -#include \n> >>>>>> -\n> >>>>>> #include \"../common/image.h\"\n> >>>>>> static const QList supportedFormats{\n> >>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> >>>>>>       /* Stride of the first plane, in pixels. */\n> >>>>>>       unsigned int stridePixels;\n> >>>>>> +    std::vector dmaSyncers;\n> >>>>>> +    for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> >>>>>> +        dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>> +\n> >>>>>>       switch (format_) {\n> >>>>>>       case libcamera::formats::NV12:\n> >>>>>>       case libcamera::formats::NV21:\n> >>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp \n> >>>>>> b/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>> index 1a238922b..4d00f154d 100644\n> >>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>> @@ -11,6 +11,7 @@\n> >>>>>>  #include \n> >>>>>>  #include \n> >>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> >>>>>>  #include \n> >>>>>>  #include \n> >>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> >>>>>>                * Otherwise, convert the format and release the frame\n> >>>>>>                * buffer immediately.\n> >>>>>>                */\n> >>>>>> +            std::vector dmaSyncers;\n> >>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> >>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>> +\n> >>>>>>               converter_.convert(image, size, &image_);\n> >>>>>>           }\n> >>>>>>       }\n> >>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> >>>>>>         /* If we have an image, draw it, with black letterbox rectangles. */\n> >>>>>>       if (!image_.isNull()) {\n> >>>>>> +        if (buffer_) {\n> >>>>>> +            std::vector dmaSyncers;\n> >>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> >>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>> +        }\n\nThis is wrong, dmaSyncers will be destroyed as soon as you exit from the\nscope, not after finishing accessing the data.\n\n> >>>>>> +\n> >>>>>>           if (place_.width() < width()) {\n> >>>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> >>>>>>               painter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3C54FC3330\n\tfor ;\n\tFri, 21 Nov 2025 03:52:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FD6A609D8;\n\tFri, 21 Nov 2025 04:52:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0AC2606A0\n\tfor ;\n\tFri, 21 Nov 2025 04:52:30 +0100 (CET)","from pendragon.ideasonboard.com (fs276ed015.tkyc509.ap.nuro.jp\n\t[39.110.208.21])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 0F919E7C;\n\tFri, 21 Nov 2025 04:50:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Vi84ZvNs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763697025;\n\tbh=O6cyLpkCytIN0HJ7tTkC13Bmmu7GDoQF59ZQWjOTNuw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Vi84ZvNsKwtIaG4bSbuFGkQE/cbm2b5d0cecTswHHH0AZHISgsQUDjsXBuM6JddKD\n\tFvxgEs5QjvBY7j7XdlWLNImhiyOX2zPxR/VBRPG8d5NPb8+IyMq1jU4n8fKR8Z8Vob\n\tMcK6HLBxq+Agb0bO0ruNs9wOzbqqpBY0BiUfwlZw=","Date":"Fri, 21 Nov 2025 12:52:05 +0900","From":"Laurent Pinchart ","To":"Robert Mader ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Sync buffers before import","Message-ID":"<20251121035205.GC9186@pendragon.ideasonboard.com>","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>\n\t\n\t\n\t<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36968,"web_url":"https://patchwork.libcamera.org/comment/36968/","msgid":"<7061efb4-0450-48b8-bcb4-18b1558e98dd@collabora.com>","date":"2025-11-21T09:49:31","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Hi\n\nOn 21.11.25 04:52, Laurent Pinchart wrote:\n> On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote:\n>> On 20.11.25 14:54, Barnabás Pőcze wrote:\n>>> 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta:\n>>>> On 20.11.25 13:35, Barnabás Pőcze wrote:\n>>>>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n>>>>>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n>>>>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n>>>>>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n>>>>>>>> coherent. This is required by the spec - from\n>>>>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n>>>>>>>>\n>>>>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n>>>>>>>>> possible to guarantee coherency between the CPU-visible map and\n>>>>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n>>>>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory\n>>>>>>>>> around if needed.\n>>>>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n>>>>>> We discussed that in the thread for \"[PATCH] libcamera:\n>>>>>> debayer_cpu: Sync output buffer\", which was eventually landed as\n>>>>>> \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the\n>>>>>> following commit back then:\n>>>>>>\n>>>>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7\n>>>>>>\n>>>>>> However we eventually decided that more fine-grained control would\n>>>>>> be useful, as the sync operation can be very expensive.\n>>>>>>\n>>>>>> We could reconsider that now - but I think we should have a fix in\n>>>>>> 0.6 either way, and this here is the approach with the least churn.\n>>>>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data\n>>>>> will be accessed from the CPU, no?\n>>>> Generally yes, but it's not necessarily optimal in every situation,\n>>>> which is why the kernel doesn't hide away the sync in the map call in\n>>>> the first place.\n>>>>\n>>>> It can e.g. be a good choice to keep a buffer mapped and only sync it\n>>>> when required, reducing some overhead. I'm not aware of any such\n>>>> usage in libcamera yet, however it's one of the things that we\n>>>> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on\n> If you're talking about the images produced by the GPU ISP, then I think\n> the buffers should not be mapped by libcamera at all.\n\nNo, I meant the the input/v4l2 buffers. Even there we'll hopefully be \nable to avoid CPU access for most frames in the future (direct GPU \nimport + only collecting stats on every so many frames) - but there \nmight still be a small benefit in keeping the buffers mapped.\n\n>\n>>>> that once the main series has landed (Gstreamer even has helpers for\n>>>> that, see e.g.\n>>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n>>>>\n>>>> In the end it's a preference of choice I'd say.\n>>> It seems to me that extending `MappedFrameBuffer` / `Image` is then\n>>> the way forward, not\n>>> necessarily adding it to the constructor/destructor but maybe adding\n>>> extra member functions.\n> That sounds like a good idea. Especially given that qcam shouldn't use\n> the internal dma_buf_allocator.h header, implementing cache sync in the\n> Image class would be cleaner.\n>\n>> Unfortunately I most likely won't have time to into that again the next\n>> two weeks, so if we want qcam fixed in 0.6, somebody else would need to\n>> take that over.\n>>\n>>>>> It's also not clear to me why the same change is not done in the\n>>>>> `cam` app as well?\n>>>> Should certainly be done in a follow-up, assuming SDL uses an\n>>>> upload-from-CPU approach as well. Haven't looked into it - I just\n>>>> quickly typed this patch down as it fixed glitches Hans was seeing on\n>>>> IPU7 (IIRC - or was it IPU6).\n>>> It is passed the mmap-ed pointer from an `Image` object, so I can't\n>>> imagine it does anything different.\n>>>\n>>>>> On a related note, pipewire marks the libcamera buffers as\n>>>>> SPA_DATA_FLAG_MAPPABLE, implying\n>>>>> that they can just be mmapped and used. I assume this has to change?\n>>>>> There does not seem to\n>>>>> be anything in the application developers' guide explicitly\n>>>>> mentioning when/how/if dma buf\n>>>>> sync is needed; it would be good to make the requirements crystal\n>>>>> clear.\n>>>> Pipewire clients - or really any client doing anything with dmabufs -\n>>>> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf\n>>>> type they have to sync them before accessing from the CPU. It's a\n>>>> fundamental part of the kernel API. When using Gstreamer that is\n>>>> hidden within the GstBuffer implementation, but other clients need to\n>>>> handle it indeed manually.\n>>> So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not\n>>> correct there.\n>> I'd say it's still close enough. AFAIK not having the flag is usually\n>> interpreted as \"you have to import via GL/VK\" - like in the case of\n>> modifiers. But that should probably be discussed in a PW issue.\n>>\n>>>> In fact a quick look suggests that libwebrtc doesn't do it yet, which\n>>>> would also need to get fixed.\n>>> As far as I can tell it does:\n>>>\n>>>   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476\n>>>   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99\n>>>\n>>> at least the libwebrtc copy in firefox. It actually mmap/munmap every\n>>> frame, which is likely not ideal.\n>> Ah nice, thanks, missed that.\n>>\n>> Good, so both libwebrtc and Gstreamer based apps are fine.\n>>\n>>>>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n>>>>>>>> case the accessed data in written by the GPU.\n>>>>>>>>\n>>>>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n>>>>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n>>>>>>>> could avoid that in the future by implementing direct dmabuf import in\n>>>>>>>> qcam.\n>>>>>>>>\n>>>>>>>> Signed-off-by: Robert Mader \n>>>>>>>> ---\n>>>>>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n>>>>>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n>>>>>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n>>>>>>>>\n>>>>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp\n>>>>>>>> b/src/apps/qcam/viewfinder_gl.cpp\n>>>>>>>> index f31956ff0..0121c97ad 100644\n>>>>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n>>>>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n>>>>>>>> @@ -9,14 +9,16 @@\n>>>>>>>> #include \n>>>>>>>> +#include \n>>>>>>>> +\n>>>>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>>>>> +\n>>>>>>>> #include \n>>>>>>>> #include \n>>>>>>>> #include \n>>>>>>>> #include \n>>>>>>>> #include \n>>>>>>>> -#include \n>>>>>>>> -\n>>>>>>>> #include \"../common/image.h\"\n>>>>>>>> static const QList supportedFormats{\n>>>>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n>>>>>>>>       /* Stride of the first plane, in pixels. */\n>>>>>>>>       unsigned int stridePixels;\n>>>>>>>> +    std::vector dmaSyncers;\n>>>>>>>> +    for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>>>>>>> +        dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>>>>> +\n>>>>>>>>       switch (format_) {\n>>>>>>>>       case libcamera::formats::NV12:\n>>>>>>>>       case libcamera::formats::NV21:\n>>>>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp\n>>>>>>>> b/src/apps/qcam/viewfinder_qt.cpp\n>>>>>>>> index 1a238922b..4d00f154d 100644\n>>>>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n>>>>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n>>>>>>>> @@ -11,6 +11,7 @@\n>>>>>>>>  #include \n>>>>>>>>  #include \n>>>>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n>>>>>>>>  #include \n>>>>>>>>  #include \n>>>>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n>>>>>>>>                * Otherwise, convert the format and release the frame\n>>>>>>>>                * buffer immediately.\n>>>>>>>>                */\n>>>>>>>> +            std::vector dmaSyncers;\n>>>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n>>>>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>>>>> +\n>>>>>>>>               converter_.convert(image, size, &image_);\n>>>>>>>>           }\n>>>>>>>>       }\n>>>>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n>>>>>>>>         /* If we have an image, draw it, with black letterbox rectangles. */\n>>>>>>>>       if (!image_.isNull()) {\n>>>>>>>> +        if (buffer_) {\n>>>>>>>> +            std::vector dmaSyncers;\n>>>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n>>>>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n>>>>>>>> +        }\n> This is wrong, dmaSyncers will be destroyed as soon as you exit from the\n> scope, not after finishing accessing the data.\nWhoops, indeed! Shouldn't impact the result - once synced the data \nshould be concurrent and won't change until the buffer is recycled - but \nfrom an API perspective it's not fully correct.\n>>>>>>>> +\n>>>>>>>>           if (place_.width() < width()) {\n>>>>>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n>>>>>>>>               painter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5EC4EC3330\n\tfor ;\n\tFri, 21 Nov 2025 09:49:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7668C60A80;\n\tFri, 21 Nov 2025 10:49:46 +0100 (CET)","from sender3-pp-f112.zoho.com (sender3-pp-f112.zoho.com\n\t[136.143.184.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 365FC608CF\n\tfor ;\n\tFri, 21 Nov 2025 10:49:45 +0100 (CET)","by mx.zohomail.com with SMTPS id 1763718578302315.4318530186938;\n\tFri, 21 Nov 2025 01:49:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"X93i7vci\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1763718580; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=lkhlIz8HeRUTwm8crgyInleFPBH89bOlNqJKDhnetHWuWV39xNIqdNWYEZMPiL1VQlStV5nqy34tk61CCFNKsG33yEVX1Nck6MloMa7aLStXOYiw255OrNWK4ch7daMmQ4xSe3SO1OGM6h7FbhsP8Qnhi2EE6bsfzxxUPEzRrqM=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1763718580;\n\th=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To;\n\tbh=9/hPOSOWwgN3obe1TqMbfKcJif+bGdA1IMfM6isuzqo=; \n\tb=EWVMpP9iXYKv2bPgtDlFtl1qRfymO0HHzlJWSEfIX5+7rZb8LyA5ED4iO5vIvx0PX7mVclnX5t+1MmYRXQ9ePbLWpvUtib1elc0/RvKDl9theICXYtK5PZZHmwD0YIWPW+NNXPTg5pJVMFc8DhIW2N+aRl80HkLJSr7PzUJgrPI=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1763718580;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:Cc:Cc:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To;\n\tbh=9/hPOSOWwgN3obe1TqMbfKcJif+bGdA1IMfM6isuzqo=;\n\tb=X93i7vciBsHPj43vq5GKstzJj9MrXOkykB4/5kOkWFGTlJmWmeLkkXQSX3XGWKi3\n\tNoUCVxzUsh3nTHuBJg2O6moo8NrMo5oM7D4bM/xun7V9Ff+n3FPppokMd96xd1zj2d9\n\tLqEzFYvDNCl9t9YMdkx2hMVHJTF/6VTFcHdygSD8=","Message-ID":"<7061efb4-0450-48b8-bcb4-18b1558e98dd@collabora.com>","Date":"Fri, 21 Nov 2025 10:49:31 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] qcam: Sync buffers before import","To":"Laurent Pinchart ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>\n\t\n\t\n\t<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>\n\t<20251121035205.GC9186@pendragon.ideasonboard.com>","Content-Language":"en-US, de-DE, en-GB","From":"Robert Mader ","In-Reply-To":"<20251121035205.GC9186@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36969,"web_url":"https://patchwork.libcamera.org/comment/36969/","msgid":"<20251121101752.GB10298@pendragon.ideasonboard.com>","date":"2025-11-21T10:17:52","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 21, 2025 at 10:49:31AM +0100, Robert Mader wrote:\n> On 21.11.25 04:52, Laurent Pinchart wrote:\n> > On Thu, Nov 20, 2025 at 03:50:42PM +0100, Robert Mader wrote:\n> >> On 20.11.25 14:54, Barnabás Pőcze wrote:\n> >>> 2025. 11. 20. 14:26 keltezéssel, Robert Mader írta:\n> >>>> On 20.11.25 13:35, Barnabás Pőcze wrote:\n> >>>>> 2025. 11. 20. 11:14 keltezéssel, Robert Mader írta:\n> >>>>>> On 20.11.25 10:58, Barnabás Pőcze wrote:\n> >>>>>>> 2025. 11. 20. 10:49 keltezéssel, Robert Mader írta:\n> >>>>>>>> Use the DmaSyncer handler to ensure the data from the buffer is\n> >>>>>>>> coherent. This is required by the spec - from\n> >>>>>>>> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> >>>>>>>>\n> >>>>>>>>> When a DMA buffer is accessed from the CPU via mmap, it is not always\n> >>>>>>>>> possible to guarantee coherency between the CPU-visible map and\n> >>>>>>>>> underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> >>>>>>>>> to bracket any CPU access to give the kernel the chance to shuffle memory\n> >>>>>>>>> around if needed.\n> >>>>>>> Shouldn't this be in the `MappedFrameBuffer` / `Image` classes then?\n> >>>>>> We discussed that in the thread for \"[PATCH] libcamera:\n> >>>>>> debayer_cpu: Sync output buffer\", which was eventually landed as\n> >>>>>> \"libcamera: debayer_cpu: Sync DMABUFs\" a year ago. I wrote the\n> >>>>>> following commit back then:\n> >>>>>>\n> >>>>>> https://gitlab.freedesktop.org/rmader/libcamera/-/commit/6b7d280634accd9c214d052df34565eb208652e7\n> >>>>>>\n> >>>>>> However we eventually decided that more fine-grained control would\n> >>>>>> be useful, as the sync operation can be very expensive.\n> >>>>>>\n> >>>>>> We could reconsider that now - but I think we should have a fix in\n> >>>>>> 0.6 either way, and this here is the approach with the least churn.\n> >>>>> But every use of MappedFrameBuffer / Image, I think, pretty much implies that data\n> >>>>> will be accessed from the CPU, no?\n> >>>> Generally yes, but it's not necessarily optimal in every situation,\n> >>>> which is why the kernel doesn't hide away the sync in the map call in\n> >>>> the first place.\n> >>>>\n> >>>> It can e.g. be a good choice to keep a buffer mapped and only sync it\n> >>>> when required, reducing some overhead. I'm not aware of any such\n> >>>> usage in libcamera yet, however it's one of the things that we\n> >>>> *shoud* do in the SW/GPU-ISP eventually, IMO, and I plan to work on\n> > If you're talking about the images produced by the GPU ISP, then I think\n> > the buffers should not be mapped by libcamera at all.\n> \n> No, I meant the the input/v4l2 buffers. Even there we'll hopefully be \n> able to avoid CPU access for most frames in the future (direct GPU \n> import + only collecting stats on every so many frames) - but there \n> might still be a small benefit in keeping the buffers mapped.\n\nFor the CPU-based soft ISP we'll obviously need to map the buffers. For\nthe GPU-based soft ISP we will also need a CPU mapping until we find a\nway to compute statistics with the GPU. As far as I understand, that's\nnot the priority, so we'll need to deal with caches for a while. It\nwould be nice not to though.\n\n> >>>> that once the main series has landed (Gstreamer even has helpers for\n> >>>> that, see e.g.\n> >>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/8540/diffs?commit_id=7a4afe551847486fdffca5dabec435d04d696452#2d772fcda2b112157970ea8f37066d9a4742306b_0_135)\n> >>>>\n> >>>> In the end it's a preference of choice I'd say.\n> >>> It seems to me that extending `MappedFrameBuffer` / `Image` is then\n> >>> the way forward, not\n> >>> necessarily adding it to the constructor/destructor but maybe adding\n> >>> extra member functions.\n> > That sounds like a good idea. Especially given that qcam shouldn't use\n> > the internal dma_buf_allocator.h header, implementing cache sync in the\n> > Image class would be cleaner.\n> >\n> >> Unfortunately I most likely won't have time to into that again the next\n> >> two weeks, so if we want qcam fixed in 0.6, somebody else would need to\n> >> take that over.\n> >>\n> >>>>> It's also not clear to me why the same change is not done in the\n> >>>>> `cam` app as well?\n> >>>> Should certainly be done in a follow-up, assuming SDL uses an\n> >>>> upload-from-CPU approach as well. Haven't looked into it - I just\n> >>>> quickly typed this patch down as it fixed glitches Hans was seeing on\n> >>>> IPU7 (IIRC - or was it IPU6).\n> >>> It is passed the mmap-ed pointer from an `Image` object, so I can't\n> >>> imagine it does anything different.\n> >>>\n> >>>>> On a related note, pipewire marks the libcamera buffers as\n> >>>>> SPA_DATA_FLAG_MAPPABLE, implying\n> >>>>> that they can just be mmapped and used. I assume this has to change?\n> >>>>> There does not seem to\n> >>>>> be anything in the application developers' guide explicitly\n> >>>>> mentioning when/how/if dma buf\n> >>>>> sync is needed; it would be good to make the requirements crystal\n> >>>>> clear.\n> >>>> Pipewire clients - or really any client doing anything with dmabufs -\n> >>>> indeed have to know that if they use buffers with the SPA_DATA_DmaBuf\n> >>>> type they have to sync them before accessing from the CPU. It's a\n> >>>> fundamental part of the kernel API. When using Gstreamer that is\n> >>>> hidden within the GstBuffer implementation, but other clients need to\n> >>>> handle it indeed manually.\n> >>> So that implies to me that SPA_DATA_FLAG_MAPPABLE is in fact not\n> >>> correct there.\n> >> I'd say it's still close enough. AFAIK not having the flag is usually\n> >> interpreted as \"you have to import via GL/VK\" - like in the case of\n> >> modifiers. But that should probably be discussed in a PW issue.\n> >>\n> >>>> In fact a quick look suggests that libwebrtc doesn't do it yet, which\n> >>>> would also need to get fixed.\n> >>> As far as I can tell it does:\n> >>>\n> >>>   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/video_capture/linux/video_capture_pipewire.cc#476\n> >>>   * https://searchfox.org/firefox-main/rev/3728e0c87fc4fa87ddf0c7a8183f2dd2329be96a/third_party/libwebrtc/modules/portal/pipewire_utils.h#99\n> >>>\n> >>> at least the libwebrtc copy in firefox. It actually mmap/munmap every\n> >>> frame, which is likely not ideal.\n> >> Ah nice, thanks, missed that.\n> >>\n> >> Good, so both libwebrtc and Gstreamer based apps are fine.\n> >>\n> >>>>>>>> This was reported to fix glitches with the upcoming GPU-ISP, in which\n> >>>>>>>> case the accessed data in written by the GPU.\n> >>>>>>>>\n> >>>>>>>> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> >>>>>>>> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> >>>>>>>> could avoid that in the future by implementing direct dmabuf import in\n> >>>>>>>> qcam.\n> >>>>>>>>\n> >>>>>>>> Signed-off-by: Robert Mader \n> >>>>>>>> ---\n> >>>>>>>>   src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> >>>>>>>>   src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> >>>>>>>>   2 files changed, 19 insertions(+), 2 deletions(-)\n> >>>>>>>>\n> >>>>>>>> diff --git a/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>>>> b/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>>>> index f31956ff0..0121c97ad 100644\n> >>>>>>>> --- a/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>>>> +++ b/src/apps/qcam/viewfinder_gl.cpp\n> >>>>>>>> @@ -9,14 +9,16 @@\n> >>>>>>>> #include \n> >>>>>>>> +#include \n> >>>>>>>> +\n> >>>>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> >>>>>>>> +\n> >>>>>>>> #include \n> >>>>>>>> #include \n> >>>>>>>> #include \n> >>>>>>>> #include \n> >>>>>>>> #include \n> >>>>>>>> -#include \n> >>>>>>>> -\n> >>>>>>>> #include \"../common/image.h\"\n> >>>>>>>> static const QList supportedFormats{\n> >>>>>>>> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> >>>>>>>>       /* Stride of the first plane, in pixels. */\n> >>>>>>>>       unsigned int stridePixels;\n> >>>>>>>> +    std::vector dmaSyncers;\n> >>>>>>>> +    for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> >>>>>>>> +        dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>>>> +\n> >>>>>>>>       switch (format_) {\n> >>>>>>>>       case libcamera::formats::NV12:\n> >>>>>>>>       case libcamera::formats::NV21:\n> >>>>>>>> diff --git a/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>>>> b/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>>>> index 1a238922b..4d00f154d 100644\n> >>>>>>>> --- a/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>>>> +++ b/src/apps/qcam/viewfinder_qt.cpp\n> >>>>>>>> @@ -11,6 +11,7 @@\n> >>>>>>>>  #include \n> >>>>>>>>  #include \n> >>>>>>>> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> >>>>>>>>  #include \n> >>>>>>>>  #include \n> >>>>>>>> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> >>>>>>>>                * Otherwise, convert the format and release the frame\n> >>>>>>>>                * buffer immediately.\n> >>>>>>>>                */\n> >>>>>>>> +            std::vector dmaSyncers;\n> >>>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> >>>>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>>>> +\n> >>>>>>>>               converter_.convert(image, size, &image_);\n> >>>>>>>>           }\n> >>>>>>>>       }\n> >>>>>>>> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> >>>>>>>>         /* If we have an image, draw it, with black letterbox rectangles. */\n> >>>>>>>>       if (!image_.isNull()) {\n> >>>>>>>> +        if (buffer_) {\n> >>>>>>>> +            std::vector dmaSyncers;\n> >>>>>>>> +            for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> >>>>>>>> +                dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> >>>>>>>> +        }\n> > This is wrong, dmaSyncers will be destroyed as soon as you exit from the\n> > scope, not after finishing accessing the data.\n> Whoops, indeed! Shouldn't impact the result - once synced the data \n> should be concurrent and won't change until the buffer is recycled - but \n> from an API perspective it's not fully correct.\n> >>>>>>>> +\n> >>>>>>>>           if (place_.width() < width()) {\n> >>>>>>>>               QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> >>>>>>>>               painter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 36B75C3330\n\tfor ;\n\tFri, 21 Nov 2025 10:18:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1DBFE60A8B;\n\tFri, 21 Nov 2025 11:18:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75FFE608CF\n\tfor ;\n\tFri, 21 Nov 2025 11:18:17 +0100 (CET)","from pendragon.ideasonboard.com (fs276ed015.tkyc509.ap.nuro.jp\n\t[39.110.208.21])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 67B4A8E0;\n\tFri, 21 Nov 2025 11:16:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hVpgMOTs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763720171;\n\tbh=5675pmdXhyGaf+Jc4MfcDRVC1H5uqv+O5TFrEu8WZKo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hVpgMOTsVbVkQeFIazLfJ9vz5xAkp9yZB3SJ+GDm9Xk6DmiB19NEN/eCBdlJJH5w5\n\tpOZ2JgVMB8HJGYOgePamdBzfp2zBh57QnV6ZwZuBPuhvcvui9lCIcNfvF0etW1siq3\n\tb5rUw9lVd6IJaeWZDSHHc+DzXEqjubPJhwW45sE4=","Date":"Fri, 21 Nov 2025 19:17:52 +0900","From":"Laurent Pinchart ","To":"Robert Mader ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Sync buffers before import","Message-ID":"<20251121101752.GB10298@pendragon.ideasonboard.com>","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<11f2077e-262c-4a81-b4d3-4e0502226f77@ideasonboard.com>\n\t<74d7ce09-01fa-4014-8515-8814beb14969@collabora.com>\n\t<875d06d9-8e88-4561-abc9-569e6a464ab0@ideasonboard.com>\n\t\n\t\n\t<3eb11324-9075-48ae-b211-40f31c4aa61f@collabora.com>\n\t<20251121035205.GC9186@pendragon.ideasonboard.com>\n\t<7061efb4-0450-48b8-bcb4-18b1558e98dd@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<7061efb4-0450-48b8-bcb4-18b1558e98dd@collabora.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37995,"web_url":"https://patchwork.libcamera.org/comment/37995/","msgid":"<176960659455.3376561.14076009978001414718@ping.linuxembedded.co.uk>","date":"2026-01-28T13:23:14","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Robert Mader (2025-11-20 09:49:19)\n> Use the DmaSyncer handler to ensure the data from the buffer is\n> coherent. This is required by the spec - from\n> https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> \n> > When a DMA buffer is accessed from the CPU via mmap, it is not always\n> > possible to guarantee coherency between the CPU-visible map and\n> > underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> > to bracket any CPU access to give the kernel the chance to shuffle memory\n> > around if needed.\n> \n> This was reported to fix glitches with the upcoming GPU-ISP, in which\n> case the accessed data in written by the GPU.\n> \n> Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> could avoid that in the future by implementing direct dmabuf import in\n> qcam.\n> \n> Signed-off-by: Robert Mader \n\n\nTested-by: Kieran Bingham \n\nThis one almost makes me sad. It removes my\ninterdimensional-parallel-universe-coexistance which is only visible\nthrough an eye of misconfigured cache coherency.\n\nYEah it looks funky ;-)\n\n> ---\n> src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> 2 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n> index f31956ff0..0121c97ad 100644\n> --- a/src/apps/qcam/viewfinder_gl.cpp\n> +++ b/src/apps/qcam/viewfinder_gl.cpp\n> @@ -9,14 +9,16 @@\n> \n> #include \n> \n> +#include \n> +\n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> +\n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> -#include \n> -\n> #include \"../common/image.h\"\n> \n> static const QList supportedFormats{\n> @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> /* Stride of the first plane, in pixels. */\n> unsigned int stridePixels;\n> \n> + std::vector dmaSyncers;\n> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> switch (format_) {\n> case libcamera::formats::NV12:\n> case libcamera::formats::NV21:\n> diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n> index 1a238922b..4d00f154d 100644\n> --- a/src/apps/qcam/viewfinder_qt.cpp\n> +++ b/src/apps/qcam/viewfinder_qt.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/dma_buf_allocator.h\"\n> #include \n> \n> #include \n> @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> * Otherwise, convert the format and release the frame\n> * buffer immediately.\n> */\n> + std::vector dmaSyncers;\n> + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> +\n> converter_.convert(image, size, &image_);\n> }\n> }\n> @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> \n> /* If we have an image, draw it, with black letterbox rectangles. */\n> if (!image_.isNull()) {\n> + if (buffer_) {\n> + std::vector dmaSyncers;\n> + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> + }\n> +\n> if (place_.width() < width()) {\n> QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> painter.drawRect(rect);\n> -- \n> 2.51.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 516CCC3200\n\tfor ;\n\tWed, 28 Jan 2026 13:23:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73A9B61FC9;\n\tWed, 28 Jan 2026 14:23:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7170161FC4\n\tfor ;\n\tWed, 28 Jan 2026 14:23:17 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47EA33A2;\n\tWed, 28 Jan 2026 14:22:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gkMrvLdy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769606560;\n\tbh=edihcgJKxc2Gv66L3KRFyf9tN3W6DjjFmwcQ34Yrync=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=gkMrvLdyZ+JKDJHED2bomHq16ZVB45pLsMgFaGJaDSFgb9eMl2kJNrN8POLOmAWen\n\t5JKDdf072U4WMyk9qq2klGtTvXQmNBTOMWS2S7xLC9Ls4r/eDw2hsgQHmINdAJD8un\n\tBZxPIPLbOcOeVIKzeFkJXf8KieWAZ/Y8tPZFJVW4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251120094919.12138-1-robert.mader@collabora.com>","References":"<20251120094919.12138-1-robert.mader@collabora.com>","Subject":"Re: [PATCH] qcam: Sync buffers before import","From":"Kieran Bingham ","Cc":"Robert Mader ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 28 Jan 2026 13:23:14 +0000","Message-ID":"<176960659455.3376561.14076009978001414718@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37996,"web_url":"https://patchwork.libcamera.org/comment/37996/","msgid":"<176960724063.3376561.12606238649067346169@ping.linuxembedded.co.uk>","date":"2026-01-28T13:34:00","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-11-21 03:31:43)\n> On Thu, Nov 20, 2025 at 10:49:19AM +0100, Robert Mader wrote:\n> > Use the DmaSyncer handler to ensure the data from the buffer is\n> > coherent. This is required by the spec - from\n> > https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> > \n> > > When a DMA buffer is accessed from the CPU via mmap, it is not always\n> > > possible to guarantee coherency between the CPU-visible map and\n> > > underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> > > to bracket any CPU access to give the kernel the chance to shuffle memory\n> > > around if needed.\n> > \n> > This was reported to fix glitches with the upcoming GPU-ISP, in which\n> > case the accessed data in written by the GPU.\n> > \n> > Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> > the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> > could avoid that in the future by implementing direct dmabuf import in\n> > qcam.\n> > \n> > Signed-off-by: Robert Mader \n> > ---\n> > src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> > src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> > 2 files changed, 19 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n> > index f31956ff0..0121c97ad 100644\n> > --- a/src/apps/qcam/viewfinder_gl.cpp\n> > +++ b/src/apps/qcam/viewfinder_gl.cpp\n> > @@ -9,14 +9,16 @@\n> > \n> > #include \n> > \n> > +#include \n> > +\n> > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> \n> As the name implies, this is an internal header. It should not be used\n> in qcam.\n\nDmaSyncer seems like the right thing to be using though and this\nabsolutely fixes a highly visible bug - would you prefer to duplicate\nthe code and copy the DmaSyncer class into qcam ?\n\n--\nKieran\n\n> \n> > +\n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > \n> > -#include \n> > -\n> > #include \"../common/image.h\"\n> > \n> > static const QList supportedFormats{\n> > @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> > /* Stride of the first plane, in pixels. */\n> > unsigned int stridePixels;\n> > \n> > + std::vector dmaSyncers;\n> > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > +\n> > switch (format_) {\n> > case libcamera::formats::NV12:\n> > case libcamera::formats::NV21:\n> > diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n> > index 1a238922b..4d00f154d 100644\n> > --- a/src/apps/qcam/viewfinder_qt.cpp\n> > +++ b/src/apps/qcam/viewfinder_qt.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > #include \n> > \n> > #include \n> > @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> > * Otherwise, convert the format and release the frame\n> > * buffer immediately.\n> > */\n> > + std::vector dmaSyncers;\n> > + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > +\n> > converter_.convert(image, size, &image_);\n> > }\n> > }\n> > @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> > \n> > /* If we have an image, draw it, with black letterbox rectangles. */\n> > if (!image_.isNull()) {\n> > + if (buffer_) {\n> > + std::vector dmaSyncers;\n> > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > + }\n> > +\n> > if (place_.width() < width()) {\n> > QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> > painter.drawRect(rect);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD250C3226\n\tfor ;\n\tWed, 28 Jan 2026 13:34:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAE3E61FC9;\n\tWed, 28 Jan 2026 14:34:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0357961FC4\n\tfor ;\n\tWed, 28 Jan 2026 14:34:03 +0100 (CET)","from monstersaurus.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 028F33A2;\n\tWed, 28 Jan 2026 14:33:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WdTqQf0J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769607207;\n\tbh=C9BRwhh+cSD7ZQc3APwOS2XlqCNz3pGtzq5ekFh2C9s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WdTqQf0JUh4yDZMBQ+zWwXrOwRY2cx28LXJNyGVIYuawJGiijZYMrfRN5rAujkTq3\n\t3MiJBJli5Y/a0wJ3T0Myy6pCNqBR8WJrYZk3W73D+wq9ZBpaUgI7PH3zLhwIczWtQ4\n\tyWbKir1AYbXM9Svk8PAHEpoYVFLDP2U0yYhd7LWo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251121033143.GB9186@pendragon.ideasonboard.com>","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<20251121033143.GB9186@pendragon.ideasonboard.com>","Subject":"Re: [PATCH] qcam: Sync buffers before import","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tRobert Mader ","Date":"Wed, 28 Jan 2026 13:34:00 +0000","Message-ID":"<176960724063.3376561.12606238649067346169@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37997,"web_url":"https://patchwork.libcamera.org/comment/37997/","msgid":"<20260128134112.GE3210848@killaraus>","date":"2026-01-28T13:41:12","subject":"Re: [PATCH] qcam: Sync buffers before import","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 28, 2026 at 01:34:00PM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2025-11-21 03:31:43)\n> > On Thu, Nov 20, 2025 at 10:49:19AM +0100, Robert Mader wrote:\n> > > Use the DmaSyncer handler to ensure the data from the buffer is\n> > > coherent. This is required by the spec - from\n> > > https://docs.kernel.org/driver-api/dma-buf.html#c.dma_buf_sync:\n> > > \n> > > > When a DMA buffer is accessed from the CPU via mmap, it is not always\n> > > > possible to guarantee coherency between the CPU-visible map and\n> > > > underlying memory. To manage coherency, DMA_BUF_IOCTL_SYNC must be used\n> > > > to bracket any CPU access to give the kernel the chance to shuffle memory\n> > > > around if needed.\n> > > \n> > > This was reported to fix glitches with the upcoming GPU-ISP, in which\n> > > case the accessed data in written by the GPU.\n> > > \n> > > Note that in the GPU-ISP case we are effectively seeing a round-trip of\n> > > the buffer contents - from GPU synced to CPU, copied to GPU again. We\n> > > could avoid that in the future by implementing direct dmabuf import in\n> > > qcam.\n> > > \n> > > Signed-off-by: Robert Mader \n> > > ---\n> > > src/apps/qcam/viewfinder_gl.cpp | 10 ++++++++--\n> > > src/apps/qcam/viewfinder_qt.cpp | 11 +++++++++++\n> > > 2 files changed, 19 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/apps/qcam/viewfinder_gl.cpp b/src/apps/qcam/viewfinder_gl.cpp\n> > > index f31956ff0..0121c97ad 100644\n> > > --- a/src/apps/qcam/viewfinder_gl.cpp\n> > > +++ b/src/apps/qcam/viewfinder_gl.cpp\n> > > @@ -9,14 +9,16 @@\n> > > \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > \n> > As the name implies, this is an internal header. It should not be used\n> > in qcam.\n> \n> DmaSyncer seems like the right thing to be using though and this\n> absolutely fixes a highly visible bug - would you prefer to duplicate\n> the code and copy the DmaSyncer class into qcam ?\n\nI re-read the mail thread, and as far as I understand the consensus is\nthat this should be handled in the Image class.\n\n> > > +\n> > > #include \n> > > #include \n> > > #include \n> > > #include \n> > > #include \n> > > \n> > > -#include \n> > > -\n> > > #include \"../common/image.h\"\n> > > \n> > > static const QList supportedFormats{\n> > > @@ -542,6 +544,10 @@ void ViewFinderGL::doRender()\n> > > /* Stride of the first plane, in pixels. */\n> > > unsigned int stridePixels;\n> > > \n> > > + std::vector dmaSyncers;\n> > > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> > > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > > +\n> > > switch (format_) {\n> > > case libcamera::formats::NV12:\n> > > case libcamera::formats::NV21:\n> > > diff --git a/src/apps/qcam/viewfinder_qt.cpp b/src/apps/qcam/viewfinder_qt.cpp\n> > > index 1a238922b..4d00f154d 100644\n> > > --- a/src/apps/qcam/viewfinder_qt.cpp\n> > > +++ b/src/apps/qcam/viewfinder_qt.cpp\n> > > @@ -11,6 +11,7 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \"libcamera/internal/dma_buf_allocator.h\"\n> > > #include \n> > > \n> > > #include \n> > > @@ -114,6 +115,10 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, Image *image)\n> > > * Otherwise, convert the format and release the frame\n> > > * buffer immediately.\n> > > */\n> > > + std::vector dmaSyncers;\n> > > + for (const libcamera::FrameBuffer::Plane &plane : buffer->planes())\n> > > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > > +\n> > > converter_.convert(image, size, &image_);\n> > > }\n> > > }\n> > > @@ -161,6 +166,12 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> > > \n> > > /* If we have an image, draw it, with black letterbox rectangles. */\n> > > if (!image_.isNull()) {\n> > > + if (buffer_) {\n> > > + std::vector dmaSyncers;\n> > > + for (const libcamera::FrameBuffer::Plane &plane : buffer_->planes())\n> > > + dmaSyncers.emplace_back(plane.fd, libcamera::DmaSyncer::SyncType::Read);\n> > > + }\n> > > +\n> > > if (place_.width() < width()) {\n> > > QRect rect{ 0, 0, (width() - place_.width()) / 2, height() };\n> > > painter.drawRect(rect);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EB224C3200\n\tfor ;\n\tWed, 28 Jan 2026 13:41:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BB6C61FC9;\n\tWed, 28 Jan 2026 14:41:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B235161FC4\n\tfor ;\n\tWed, 28 Jan 2026 14:41:13 +0100 (CET)","from killaraus.ideasonboard.com\n\t(2001-14ba-703d-e500--2a1.rev.dnainternet.fi\n\t[IPv6:2001:14ba:703d:e500::2a1])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 7CE8B3A2;\n\tWed, 28 Jan 2026 14:40:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jwlWdJDt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1769607636;\n\tbh=3sLRwjWfqoFgWe/6TvqxmQmgaE+ZkJB7bUx5C7D4Qgw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jwlWdJDtFYUCeODddgVO53a+owUTklZ9Cuz/3JtXLns53w8lQZKwCGEsWcli+S+df\n\tR6Ly5qSqYzhzqiSZq8enHph8dj0kdvQia198oFZyMocR07ATu7Oh7trSUyXUP/KDZL\n\tRK5YFhuEIfY7I7rCTT9IdD2kMgV4Yn8eUtfoE8gY=","Date":"Wed, 28 Jan 2026 15:41:12 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] qcam: Sync buffers before import","Message-ID":"<20260128134112.GE3210848@killaraus>","References":"<20251120094919.12138-1-robert.mader@collabora.com>\n\t<20251121033143.GB9186@pendragon.ideasonboard.com>\n\t<176960724063.3376561.12606238649067346169@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176960724063.3376561.12606238649067346169@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]