[{"id":31246,"web_url":"https://patchwork.libcamera.org/comment/31246/","msgid":"<172649945183.3421941.2216890016391050949@ping.linuxembedded.co.uk>","date":"2024-09-16T15:10:51","subject":"Re: [PATCH v3] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Robert Mader (2024-09-16 15:11:25)\n> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n> correct output. Not doing so currently results in occasional tearing\n> and/or backlashes in GL/VK clients that use the buffers directly for\n> rendering.\n> \n> An alternative approach to have the sync code in `MappedFrameBuffer` was\n> considered but rejected for now, in order to allow clients more\n> flexibility.\n> \n> Signed-off-by: Robert Mader \n> \n> ---\n> \n> Changes in v3:\n> - add syncBufferForCPU() helper function\n> \n> Changes in v2:\n> - sync input buffer as well\n> - update commit title accordingly\n> - small changes to the commit message\n> - linting fixes, should pass now\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 077f7f4b..3748bd7c 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -12,8 +12,11 @@\n> #include \"debayer_cpu.h\"\n> \n> #include \n> +#include \n> #include \n> \n> +#include \n> +\n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> @@ -718,6 +721,17 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> }\n> }\n> \n> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n\nI think these functions (including timeDiff below) should be wrapped in\nan annoymous namespace for the libcamera code style.\n\nBut as the timeDiff is already in this form - they can both be converted\nafter. (or before if you want to do the fix up)\n\n> +{\n> + for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> + const int fd = plane.fd.get();\n> + struct dma_buf_sync sync = { syncFlags };\n> +\n> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n\nThis could be rewrapped easily\n\n\t\t\tLOG(Debayer, Error)\n\t\t\t\t<< \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n\nBut we can't reference errno directly from LOG() either, as mentioned in\nthe previous version:\n\nEasiest cleanup is probably:\n\n\t\tint ret;\n\n\t\tret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n\t\tif (ret < 0) {\n\t\t\tret = errno;\n\t\t\tLOG(Debayer, Error)\n\t\t\t\t<< \"Syncing buffer FD \" << fd << \"failed: \"\n\t\t\t\t<< strerror(-ret);\n\n\t\t}\n\nCould also print the sync flags too to know which one has failed?\n\nWith that handled:\n\nReviewed-by: Kieran Bingham \n\n> + }\n> +}\n> +\n> static inline int64_t timeDiff(timespec &after, timespec &before)\n> {\n> return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> @@ -733,6 +747,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> }\n> \n> + syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> + syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> +\n> green_ = params.green;\n> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> @@ -760,6 +777,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \n> metadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> + syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> + syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> +\n> /* Measure before emitting signals */\n> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> -- \n> 2.46.0\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 3FD82C3257\n\tfor ;\n\tMon, 16 Sep 2024 15:10:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9C98634FD;\n\tMon, 16 Sep 2024 17:10:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 25D00634F5\n\tfor ;\n\tMon, 16 Sep 2024 17:10:55 +0200 (CEST)","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 B2C77480;\n\tMon, 16 Sep 2024 17:09:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p9pQ/a0T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726499373;\n\tbh=NYuUQLHL2qgzjl8YaGwpGtlh7Z27eQBxOVSZO4d8DTg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=p9pQ/a0TB9cUhQmeznvXZ958m82ElxjQehoLvN1JmrXlVyl2pD2c6rorLF15yTXGH\n\t7r5C15pYDtZghwu3SALoUat3C0TQn3t5VOptw6dpIye7UkFbRb0rz/PzArw5a2veI9\n\t4vPJyMK6Ik/7mzQr2QZH3hIxD/PQRTznjGS/v+Fg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240916141125.212973-1-robert.mader@collabora.com>","References":"<20240916141125.212973-1-robert.mader@collabora.com>","Subject":"Re: [PATCH v3] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 16 Sep 2024 16:10:51 +0100","Message-ID":"<172649945183.3421941.2216890016391050949@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31252,"web_url":"https://patchwork.libcamera.org/comment/31252/","msgid":"<9bc1d984-edc9-4ee1-91fc-13a34620d5ce@collabora.com>","date":"2024-09-16T16:15:31","subject":"Re: [PATCH v3] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 16.09.24 17:10, Kieran Bingham wrote:\n> Quoting Robert Mader (2024-09-16 15:11:25)\n>> Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure\n>> correct output. Not doing so currently results in occasional tearing\n>> and/or backlashes in GL/VK clients that use the buffers directly for\n>> rendering.\n>>\n>> An alternative approach to have the sync code in `MappedFrameBuffer` was\n>> considered but rejected for now, in order to allow clients more\n>> flexibility.\n>>\n>> Signed-off-by: Robert Mader \n>>\n>> ---\n>>\n>> Changes in v3:\n>> - add syncBufferForCPU() helper function\n>>\n>> Changes in v2:\n>> - sync input buffer as well\n>> - update commit title accordingly\n>> - small changes to the commit message\n>> - linting fixes, should pass now\n>> ---\n>> src/libcamera/software_isp/debayer_cpu.cpp | 20 ++++++++++++++++++++\n>> 1 file changed, 20 insertions(+)\n>>\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 077f7f4b..3748bd7c 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -12,8 +12,11 @@\n>> #include \"debayer_cpu.h\"\n>> \n>> #include \n>> +#include \n>> #include \n>> \n>> +#include \n>> +\n>> #include \n>> \n>> #include \"libcamera/internal/bayer_format.h\"\n>> @@ -718,6 +721,17 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n>> }\n>> }\n>> \n>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> I think these functions (including timeDiff below) should be wrapped in\n> an annoymous namespace for the libcamera code style.\n>\n> But as the timeDiff is already in this form - they can both be converted\n> after. (or before if you want to do the fix up)\n>\n>> +{\n>> + for (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> + const int fd = plane.fd.get();\n>> + struct dma_buf_sync sync = { syncFlags };\n>> +\n>> + if (ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync) < 0)\n>> + LOG(Debayer, Error) << \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n> This could be rewrapped easily\n>\n> \t\t\tLOG(Debayer, Error)\n> \t\t\t\t<< \"Syncing buffer FD \" << fd << \"failed: \" << errno;\n>\n> But we can't reference errno directly from LOG() either, as mentioned in\n> the previous version:\n>\n> Easiest cleanup is probably:\n>\n> \t\tint ret;\n>\n> \t\tret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> \t\tif (ret < 0) {\n> \t\t\tret = errno;\n> \t\t\tLOG(Debayer, Error)\n> \t\t\t\t<< \"Syncing buffer FD \" << fd << \"failed: \"\n> \t\t\t\t<< strerror(-ret);\n>\n> \t\t}\n>\n> Could also print the sync flags too to know which one has failed?\n>\n> With that handled:\n>\n> Reviewed-by: Kieran Bingham \n\nThanks, addressed in v4, hopefully correctly (not quite sure about how \nto best print the flags, therefore didn't add the R-b).\n\n>\n>> + }\n>> +}\n>> +\n>> static inline int64_t timeDiff(timespec &after, timespec &before)\n>> {\n>> return (after.tv_sec - before.tv_sec) * 1000000000LL +\n>> @@ -733,6 +747,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> }\n>> \n>> + syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n>> + syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n>> +\n>> green_ = params.green;\n>> red_ = swapRedBlueGains_ ? params.blue : params.red;\n>> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> @@ -760,6 +777,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> \n>> metadata.planes()[0].bytesused = out.planes()[0].size();\n>> \n>> + syncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n>> + syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n>> +\n>> /* Measure before emitting signals */\n>> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> -- \n>> 2.46.0\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 8321FC3257\n\tfor ;\n\tMon, 16 Sep 2024 16:15:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 74E07634FF;\n\tMon, 16 Sep 2024 18:15:38 +0200 (CEST)","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 0C7F3634F8\n\tfor ;\n\tMon, 16 Sep 2024 18:15:36 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1726503333480502.45428377831024; \n\tMon, 16 Sep 2024 09:15:33 -0700 (PDT)"],"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=\"geg58ZZQ\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1726503334; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=QcH9b+jGnIdOGpoagu9Uwq3ocKL75ewh9ZEpIREOASSZiC03bMMwiS0o1DZETs0VB4GkmqbyMPWwLKa0c1dRO2UY1XBukXrJhmyGAO2dbdw55aMwH7I8G7G4sLNUIHvNUGS7X+5HKMLf4M4biZ+9P34DlQkadvmPL8YP3tl92XU=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1726503334;\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=bbmgk8+BesbcmiotK4OV/ygITtYO0gEs/tlGNqboqvM=; \n\tb=EMn3yyW/RKORBEvWT/OVnFzo/fyLgZCVcU/0t4BdlURAnMdA7/8sACNfuU/D0g1pN384mdyR7Qet+G7OAMJx38HyBSsqnFNfT94ypy2To50W6AriBs00C8iuqszMA+b2c3BTnPSTcML/DfBOvP0ofxeQboy05sLIs7RZkrAuclA=","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=1726503334;\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=bbmgk8+BesbcmiotK4OV/ygITtYO0gEs/tlGNqboqvM=;\n\tb=geg58ZZQB4PjGKLD1/waIhrxQ8losr7TM4UA4Qc4TP2UA5NSNuiFuhRPp99wqfPB\n\tvwjO88pu2rGLPYX3tCO+ijTUKCvF4FNR6qyAMHFAV+x9Lm3SjgwwPwJiwelxZFFxZOf\n\trVUQNpXK5p1qfpSP5tlZAuDGhykWYtOGHzDrOwbo=","Message-ID":"<9bc1d984-edc9-4ee1-91fc-13a34620d5ce@collabora.com>","Date":"Mon, 16 Sep 2024 18:15:31 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3] libcamera: debayer_cpu: Sync DMABUFs","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240916141125.212973-1-robert.mader@collabora.com>\n\t<172649945183.3421941.2216890016391050949@ping.linuxembedded.co.uk>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<172649945183.3421941.2216890016391050949@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]