[{"id":31254,"web_url":"https://patchwork.libcamera.org/comment/31254/","msgid":"<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>","date":"2024-09-17T12:01:24","subject":"Re: [PATCH v4] 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 17:12:31)\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 v4:\n> - Fixed errno handling\n> - Added sync flags to error message\n> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> }\n> }\n> \n> +namespace {\n> +\n> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n\nNow these are in an annoymous namespace, they don't need to be static -\nbut that can be fixed while applying perhaps or ignored for now if no\nfurther version is required otherwise.\n\nI'm happy to merge, but under software_isp/* I think we should have an\nAcked-by/Reviewed-by by one of the SoftISP developers.\n\nMilan, Hans ? Are you happy with this patch? Is there any impact with\nthe Intel devices for instance?\n\n--\nKieran\n\n\n\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> + int ret;\n> +\n> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(Debayer, Error)\n> + << \"Syncing buffer FD \" << fd << \" with flags \"\n> + << syncFlags << \" failed: \" << strerror(-ret);\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> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> }\n> \n> +} /* namespace */\n> +\n> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> {\n> timespec frameStartTime;\n> @@ -733,6 +757,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 +787,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 DE341C3257\n\tfor ;\n\tTue, 17 Sep 2024 12:01:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A80F8634F8;\n\tTue, 17 Sep 2024 14:01:29 +0200 (CEST)","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 C9617618F1\n\tfor ;\n\tTue, 17 Sep 2024 14:01:27 +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 C4E238BE;\n\tTue, 17 Sep 2024 14:00:05 +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=\"pPA+iPI9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726574405;\n\tbh=mJ2AINRN0xgE0tMJvpLPZ91jgYg81t505SrIW5C77jA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=pPA+iPI9HYmQpf1j0pE2Ux6e6fUsV6mQrdb9EAWTqiqjxncFjduIRz0v/n9TvxUfp\n\tDjP6lvyxNMX0Sb3Ogs4eXuqkZa5sZtR+rGQHYmWI1LP1j6F8w289lK5MIfeIQrcOmB\n\tMCELVwmxc/IH56953EblMMC7EPbt5cRMXBuOA6EU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240916161231.238116-1-robert.mader@collabora.com>","References":"<20240916161231.238116-1-robert.mader@collabora.com>","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , \n\tHans de Goede , ","Date":"Tue, 17 Sep 2024 13:01:24 +0100","Message-ID":"<172657448433.1366291.18424755118505899651@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":31262,"web_url":"https://patchwork.libcamera.org/comment/31262/","msgid":"<87jzf9uvz0.fsf@redhat.com>","date":"2024-09-18T09:55:47","subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Robert Mader (2024-09-16 17:12:31)\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>\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 v4:\n>> - Fixed errno handling\n>> - Added sync flags to error message\n>> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n>> 1 file changed, 30 insertions(+)\n>> \n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n>> }\n>> }\n>> \n>> +namespace {\n>> +\n>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n>\n> Now these are in an annoymous namespace, they don't need to be static -\n> but that can be fixed while applying perhaps or ignored for now if no\n> further version is required otherwise.\n>\n> I'm happy to merge, but under software_isp/* I think we should have an\n> Acked-by/Reviewed-by by one of the SoftISP developers.\n>\n> Milan, Hans ? Are you happy with this patch? Is there any impact with\n> the Intel devices for instance?\n\nI'm fine with the patch (with the exception below) and it works fine on\nDebix (with only ~0,5% performance penalty). It would be nice if Hans\ntested it on Intel.\n\n> --\n> Kieran\n>\n>\n>\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>> + int ret;\n>> +\n>> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n>> + if (ret < 0) {\n>> + ret = errno;\n>> + LOG(Debayer, Error)\n>> + << \"Syncing buffer FD \" << fd << \" with flags \"\n>> + << syncFlags << \" failed: \" << strerror(-ret);\n\nI think `ret' and not `-ret' should be used here. And I would drop\n`ret = errno' above and use errno here directly. I'm not sure why\nKieran suggested exactly this -- using strerror is right but changing\nthe sign seems wrong (tested in my environment).\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>> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n>> }\n>> \n>> +} /* namespace */\n>> +\n>> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>> {\n>> timespec frameStartTime;\n>> @@ -733,6 +757,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 +787,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 22049C324C\n\tfor ;\n\tWed, 18 Sep 2024 09:55:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03984634F9;\n\tWed, 18 Sep 2024 11:55:54 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02F7E618E0\n\tfor ;\n\tWed, 18 Sep 2024 11:55:52 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-441-IVUSKuJbODWU28zdKwy_AQ-1; Wed, 18 Sep 2024 05:55:50 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-377a26a5684so1991913f8f.0\n\tfor ;\n\tWed, 18 Sep 2024 02:55:50 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-378e73e81a5sm11934117f8f.34.2024.09.18.02.55.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 18 Sep 2024 02:55:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"LHVlF2D4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726653351;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Vrrb2bObjqxrErMHLnl0j0OGDrb01bXJInV+WRqzW18=;\n\tb=LHVlF2D4sbx2yP0A12t/2JhcJDlcCj3tzce9JPpcbk5jjb68mPgsY5pr7ZNam17/bq7vWu\n\tDNbufGXoH958auOenE85vMxgIY7b1UJotTQf84RbGhXyEco1lUVlNR3J80v/yD405KMlQ+\n\tUMTRe5QyWY4/11Fw589rFDPzKpQ0mVs=","X-MC-Unique":"IVUSKuJbODWU28zdKwy_AQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726653349; x=1727258149;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Vrrb2bObjqxrErMHLnl0j0OGDrb01bXJInV+WRqzW18=;\n\tb=W1mJanYri7OxObD+jvxSsUjke9zgn8BLmtpXX41tUne7/yOVUeGGFGDNdLfpgd+01t\n\tIiF1MjdxpzTWtV2OzPAR55K8Bp2j0xyyDzaCRMMQsz45D+izANb3m63K8XPGESHxIvZU\n\teAd+8qz7R2OVe8ez8nKIMfgc7yBD4om9omMu3ZgV0cb5fh/opm8ZV0zPyc8lYoN+dYpj\n\tELk4lKWNv3nO26HGW000Tb3sBxiERrhHRj7+Z1FjOW1PFKRa2MAy4BvtvbLIyc8L+Hqw\n\tfj4tolF+k+ka3G1eeZuVMilA53bu7iw5/MteFUFDZB871STcTWNnQDf5tW+HtMKLyxKy\n\tddbQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV/yVIgmCQae+qOV3KjI4URJYja9aFuXGtMNzOkLlejO2zT8+p8M0N9qbzwWqNbiwvXvpAGwAeycsieagrHvvs=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzZ+exvoOpXVZ9S9oQLd8JAnjo+d0MuyXlAAH/W12kuHtiPo2kS\n\tt1VANttJxRWP6RMLX1yZaT+4gkhqBBNzOHkdqcTew+1jl4P8IbEb3upVlo7K4k7obuEkftkQZfA\n\t5fBIPrz0azbUn6XaNgtVd1bJr5EUow13Jypk/wEJSAYur8MaS8VJy9n/kbA6/R0b9F2oW/A0=","X-Received":["by 2002:a5d:6d84:0:b0:371:c578:3130 with SMTP id\n\tffacd0b85a97d-378d61d4fdemr12507153f8f.8.1726653349284; \n\tWed, 18 Sep 2024 02:55:49 -0700 (PDT)","by 2002:a5d:6d84:0:b0:371:c578:3130 with SMTP id\n\tffacd0b85a97d-378d61d4fdemr12507131f8f.8.1726653348738; \n\tWed, 18 Sep 2024 02:55:48 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG6hbIYA5zsUXOcB3guu2NFNGmzw6Md2XonMlZD4FiF3yMjEXOY80wr+HpdbAQPCZ4it4wMJA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org, Hans de Goede ","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","In-Reply-To":"<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Tue, 17 Sep 2024 13:01:24 +0100\")","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>","Date":"Wed, 18 Sep 2024 11:55:47 +0200","Message-ID":"<87jzf9uvz0.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":31263,"web_url":"https://patchwork.libcamera.org/comment/31263/","msgid":"<21a01871-d16f-4912-9a5c-bbd22324d307@collabora.com>","date":"2024-09-18T10:26:25","subject":"Re: [PATCH v4] 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":"One thing I'd like to highlight again for the record: while the patch \nhere reduces glitches considerably for me, I still occasionally see some \nwhen using GL clients like Snapshot.\n\nAssuming that's not because of hardware/driver bugs (I see it on quite \ndifferent platforms), that could be caused by us still waiting for \nimplicit/explicit sync before reusing buffers.\n\n From https://docs.kernel.org/driver-api/dma-buf.html:\n\n> The synchronization provided via DMA_BUF_IOCTL_SYNC only provides \n> cache coherency. It does not prevent other processes or devices from \n> accessing the memory at the same time. If synchronization with a GPU \n> or other device driver is required, it is the client’s responsibility \n> to wait for buffer to be ready for reading or writing before calling \n> this ioctl with DMA_BUF_SYNC_START. Likewise, the client must ensure \n> that follow-up work is not submitted to GPU or other device driver \n> until after this ioctl has been called with DMA_BUF_SYNC_END?\n>\n> If the driver or API with which the client is interacting uses \n> implicit synchronization, waiting for prior work to complete can be \n> done via poll() on the DMA buffer file descriptor. If the driver or \n> API requires explicit synchronization, the client may have to wait on \n> a sync_file or other synchronization primitive outside the scope of \n> the DMA buffer API.\n>\nI wonder if we should add a swISP TODO for this - or is opening a \nbugzilla issue for tracking enough?\n\nP.S.: I'm not entirely sure if the udmabuf sync implementation really \ndoesn't wait for implicit fences - but if it does, there's no guarantee \nthat things stay that way.\n\nOn 16.09.24 18:12, Robert Mader wrote:\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 v4:\n> - Fixed errno handling\n> - Added sync flags to error message\n> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n>\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \t}\n> }\n> \n> +namespace {\n> +\n> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> +{\n> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> +\t\tconst int fd = plane.fd.get();\n> +\t\tstruct dma_buf_sync sync = { syncFlags };\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 << \" with flags \"\n> +\t\t\t\t<< syncFlags << \" failed: \" << strerror(-ret);\n> +\t\t}\n> +\t}\n> +}\n> +\n> static inline int64_t timeDiff(timespec &after, timespec &before)\n> {\n> \treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> \t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> }\n> \n> +} /* namespace */\n> +\n> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> {\n> \ttimespec frameStartTime;\n> @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> \t}\n> \n> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> +\n> \tgreen_ = params.green;\n> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n> @@ -760,6 +787,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \n> \tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> +\n> \t/* Measure before emitting signals */\n> \tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> \t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {","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 06DC9C3257\n\tfor ;\n\tWed, 18 Sep 2024 10:26:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C9FB634FC;\n\tWed, 18 Sep 2024 12:26:33 +0200 (CEST)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 849A3618E8\n\tfor ;\n\tWed, 18 Sep 2024 12:26:31 +0200 (CEST)","by mx.zohomail.com with SMTPS id 172665518725660.617200351030874; \n\tWed, 18 Sep 2024 03:26:27 -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=\"MY48OZoJ\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1726655189; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=P7x8QQ7YoZKZWUXLR3lazxNMNGmUXrcQIvvBSJE+DeiiN/Ti40EuNNXsLu6JDsmV1sWDqSSRxEFRqILtjPoZU6nsXBR6lRGHznvGgSAN9k4QSwD55qxSBbfE7Q2kbdYg/067IGojsm0c7MpaLuTw9qYu/KbgdgZgNNOPOy5ILIk=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1726655189;\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=jj+z32Opr8rX3H/B9Jm+ePKhzma1KAod6jdeN/yJHd0=; \n\tb=Ri3qnv4vo5Br6j+hgezcnJDg+5yDNWoiZXAwerXKxAAbrhKlqJyCIuraFwRu62mUgCWPGD9zwHzvLxTYSyuIt22qVTF6CyrXqv/sjrkmpPRH2vdprZ/6C+mcSs05h0FYugq10W2qNBYBb63u51XUiTel/BT7FevYsdPlmnoiFF8=","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=1726655189;\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=jj+z32Opr8rX3H/B9Jm+ePKhzma1KAod6jdeN/yJHd0=;\n\tb=MY48OZoJijw60D0XEIwziafvyPO4/C/J2e4jJheeIMJUftyRtF7dNizeQYXa+nL3\n\tgb7s4naYmgnrLM5pcnZZsXI8IjlSyxZXwH+hRyf0xgg+XkRLSkwxnicKKlOipcnvQe5\n\tWpJlh1aZb05vwxmcUAzTp64KH9eh4IIYnaRm85Gg=","Message-ID":"<21a01871-d16f-4912-9a5c-bbd22324d307@collabora.com>","Date":"Wed, 18 Sep 2024 12:26:25 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","To":"libcamera-devel@lists.libcamera.org","References":"<20240916161231.238116-1-robert.mader@collabora.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<20240916161231.238116-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":31264,"web_url":"https://patchwork.libcamera.org/comment/31264/","msgid":"<172665700605.3512001.16092546261528560159@ping.linuxembedded.co.uk>","date":"2024-09-18T10:56:46","subject":"Re: [PATCH v4] 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 Milan Zamazal (2024-09-18 10:55:47)\n> Kieran Bingham writes:\n> \n> > Quoting Robert Mader (2024-09-16 17:12:31)\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> >\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 v4:\n> >> - Fixed errno handling\n> >> - Added sync flags to error message\n> >> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> >> 1 file changed, 30 insertions(+)\n> >> \n> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> >> }\n> >> }\n> >> \n> >> +namespace {\n> >> +\n> >> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> >\n> > Now these are in an annoymous namespace, they don't need to be static -\n> > but that can be fixed while applying perhaps or ignored for now if no\n> > further version is required otherwise.\n> >\n> > I'm happy to merge, but under software_isp/* I think we should have an\n> > Acked-by/Reviewed-by by one of the SoftISP developers.\n> >\n> > Milan, Hans ? Are you happy with this patch? Is there any impact with\n> > the Intel devices for instance?\n> \n> I'm fine with the patch (with the exception below) and it works fine on\n> Debix (with only ~0,5% performance penalty). It would be nice if Hans\n> tested it on Intel.\n> \n> > --\n> > Kieran\n> >\n> >\n> >\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> >> + int ret;\n> >> +\n> >> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> >> + if (ret < 0) {\n> >> + ret = errno;\n> >> + LOG(Debayer, Error)\n> >> + << \"Syncing buffer FD \" << fd << \" with flags \"\n> >> + << syncFlags << \" failed: \" << strerror(-ret);\n> \n> I think `ret' and not `-ret' should be used here. And I would drop\n> `ret = errno' above and use errno here directly. I'm not sure why\n> Kieran suggested exactly this -- using strerror is right but changing\n> the sign seems wrong (tested in my environment).\n\nThis is a pattern we've used throughout libcamera. It's not advised to\nuse errno directly in a stream operation ( << ) because other parts of\nthe stream or calls, or even the construction of the LOG() class could\ndo something that also sets errno.\n\nSo we take a copy of errno as soon as the error is detected.\n\nYes - I'm wrong on the -ret in that instance. Sorry - I got confused\nbecause we would only return a negative error value - but in this\ninstance we are in a void function that doesn't return anything.\n(there's no point I think).\n\nBut yes I inverted the value - sorry. As long as 'errno' is not directly\nused in the LOG() << errno; statements it's fine.\n\n--\nKieran\n\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> >> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> >> }\n> >> \n> >> +} /* namespace */\n> >> +\n> >> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> >> {\n> >> timespec frameStartTime;\n> >> @@ -733,6 +757,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 +787,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> >>\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 30D61C324C\n\tfor ;\n\tWed, 18 Sep 2024 10:56:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0FF0634FC;\n\tWed, 18 Sep 2024 12:56:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B905618E8\n\tfor ;\n\tWed, 18 Sep 2024 12:56:49 +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 92FFF21E;\n\tWed, 18 Sep 2024 12:55:26 +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=\"uUKqTNsf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726656926;\n\tbh=5zPh6zZEOzrVC/x4HNkQ1Z7oM2ktGx0JLfH+IaY6Svs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uUKqTNsfahvCfofxDqlPXWeNDW4maj8Xedin0z7Q+BN7nqLngt+VEBd554wUAiEdU\n\tTFOibDooj3p/j+QKbxoAL0OWl0vMdOvUESyAAtPG54lYaUyorSMac+S/0pQ0NIJ7vf\n\tHCpq8XG9fty9YO0Snp1Wp6DMVl/DA7zxlMba1o2E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<87jzf9uvz0.fsf@redhat.com>","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>\n\t<87jzf9uvz0.fsf@redhat.com>","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org, Hans de Goede ","To":"Milan Zamazal ","Date":"Wed, 18 Sep 2024 11:56:46 +0100","Message-ID":"<172665700605.3512001.16092546261528560159@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":31265,"web_url":"https://patchwork.libcamera.org/comment/31265/","msgid":"<87bk0lus85.fsf@redhat.com>","date":"2024-09-18T11:16:42","subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-09-18 10:55:47)\n>> Kieran Bingham writes:\n>> \n>\n>> > Quoting Robert Mader (2024-09-16 17:12:31)\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>> >\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 v4:\n>> >> - Fixed errno handling\n>> >> - Added sync flags to error message\n>> >> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n>> >> 1 file changed, 30 insertions(+)\n>> >> \n>> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> >> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n>> >> }\n>> >> }\n>> >> \n>> >> +namespace {\n>> >> +\n>> >> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n>> >\n>> > Now these are in an annoymous namespace, they don't need to be static -\n>> > but that can be fixed while applying perhaps or ignored for now if no\n>> > further version is required otherwise.\n>> >\n>> > I'm happy to merge, but under software_isp/* I think we should have an\n>> > Acked-by/Reviewed-by by one of the SoftISP developers.\n>> >\n>> > Milan, Hans ? Are you happy with this patch? Is there any impact with\n>> > the Intel devices for instance?\n>> \n>> I'm fine with the patch (with the exception below) and it works fine on\n>> Debix (with only ~0,5% performance penalty). It would be nice if Hans\n>> tested it on Intel.\n>> \n>> > --\n>> > Kieran\n>> >\n>> >\n>> >\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>> >> + int ret;\n>> >> +\n>> >> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n>> >> + if (ret < 0) {\n>> >> + ret = errno;\n>> >> + LOG(Debayer, Error)\n>> >> + << \"Syncing buffer FD \" << fd << \" with flags \"\n>> >> + << syncFlags << \" failed: \" << strerror(-ret);\n>> \n>> I think `ret' and not `-ret' should be used here. And I would drop\n>> `ret = errno' above and use errno here directly. I'm not sure why\n>> Kieran suggested exactly this -- using strerror is right but changing\n>> the sign seems wrong (tested in my environment).\n>\n> This is a pattern we've used throughout libcamera. It's not advised to\n> use errno directly in a stream operation ( << ) because other parts of\n> the stream or calls, or even the construction of the LOG() class could\n> do something that also sets errno.\n>\n> So we take a copy of errno as soon as the error is detected.\n\nAh, right, thanks for explanation.\n\n> Yes - I'm wrong on the -ret in that instance. Sorry - I got confused\n> because we would only return a negative error value - but in this\n> instance we are in a void function that doesn't return anything.\n> (there's no point I think).\n>\n> But yes I inverted the value - sorry. As long as 'errno' is not directly\n> used in the LOG() << errno; statements it's fine.\n>\n> --\n> Kieran\n>\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>> >> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n>> >> }\n>> >> \n>> >> +} /* namespace */\n>> >> +\n>> >> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>> >> {\n>> >> timespec frameStartTime;\n>> >> @@ -733,6 +757,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 +787,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>> >>\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 4C5E8C3257\n\tfor ;\n\tWed, 18 Sep 2024 11:16:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5CEF634FC;\n\tWed, 18 Sep 2024 13:16:49 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14061618E8\n\tfor ;\n\tWed, 18 Sep 2024 13:16:47 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-663-mgafiSzNNDekP_4GdVaGKg-1; Wed, 18 Sep 2024 07:16:45 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a8a8d9a2a52so578902666b.1\n\tfor ;\n\tWed, 18 Sep 2024 04:16:45 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a90612e5d0csm571734366b.184.2024.09.18.04.16.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 18 Sep 2024 04:16:43 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"LTo3w+3p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726658206;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Q/cKcGKbnw+umuNgSDG3k/Rv/hkXUw7Bu9l3VjKcVxI=;\n\tb=LTo3w+3pJV4btiGkEi0tIcBCv9yGF2QyYKlyuo9765vG+FKZvajrUYDw+PNtqWVLFDjJ6f\n\t7kSmIw3qWdWw2eclGUGSdHlcL4AkAyk3Q/byYhg2yizjxxiQUGwV3YQqeoMoZhMk0q5MNY\n\tjUBmjGTwN/FmMXgSorDP7yk/vH34GA0=","X-MC-Unique":"mgafiSzNNDekP_4GdVaGKg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726658204; x=1727263004;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Q/cKcGKbnw+umuNgSDG3k/Rv/hkXUw7Bu9l3VjKcVxI=;\n\tb=MH2zLpEY33jV0aEvPPp0sw/mLDPakSY1x07GbdctjqAg0PcyeK1BisS0lDNR2b/NOh\n\tg0Qdub+6ZjiJqY5vr/2TND4D1eyWQNV/IZtU4DkMO6nj2uQVTfu8Bz7KE24qz8+WrIv1\n\toLlvKiw3QZqNy1/bHvmdQmabgD1bQKj/db7HA56VXA8qGK8ehxI8AykbLPgiycWj7cf3\n\tCejUiNEM1XJBY2c69X6+QJy55UMXnLhDJT9XhxzfsJD1bK3HqlwHOLyl5tQk6OGO6h0x\n\tj+zBJe9WnykqDoVZa4BaQU+x9Wp5QS4wo4Kl52L34I71xVpZsG2OQnfT9mZBhjIwPGds\n\tYZAg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUuQXjw+5qM4NyqTJeUwNyWUsbAMftkrOfvUkSsC4tbN/XEeXXMKSlxibJ8t/+dDqcDhLcQHzPrUPfZnX2osAc=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yye+W1QWn42ZzF+KNtjJcRD8k7t2WbfBn+eB4KTFSXgMfW5SfQr\n\tOMaajwyEdxIASUNQ/XUgpdv3ggbNJGneZOF1JYRxcvJK9VZI2UfczRcbaFN+MDZ7L45m9iSN/fc\n\txCDnzgX3EqxTavC/VvU73YxCV7KmewudEFYLNZNIEdzrdhkhUvMZpfNN5LXIGN7asGxnjnys=","X-Received":["by 2002:a17:907:f74f:b0:a8a:8127:4af with SMTP id\n\ta640c23a62f3a-a90294ad65dmr2295011266b.39.1726658204387; \n\tWed, 18 Sep 2024 04:16:44 -0700 (PDT)","by 2002:a17:907:f74f:b0:a8a:8127:4af with SMTP id\n\ta640c23a62f3a-a90294ad65dmr2295008966b.39.1726658203774; \n\tWed, 18 Sep 2024 04:16:43 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGEH445i3Ecr/iLchO7RcDblOktUm5V4oSdCnCXLvRww4uMWIs13YUMJ70fumPTsipeQ6ntLQ==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org, Hans de Goede ","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","In-Reply-To":"<172665700605.3512001.16092546261528560159@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Wed, 18 Sep 2024 11:56:46 +0100\")","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>\n\t<87jzf9uvz0.fsf@redhat.com>\n\t<172665700605.3512001.16092546261528560159@ping.linuxembedded.co.uk>","Date":"Wed, 18 Sep 2024 13:16:42 +0200","Message-ID":"<87bk0lus85.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":31266,"web_url":"https://patchwork.libcamera.org/comment/31266/","msgid":"<877cb9urri.fsf@redhat.com>","date":"2024-09-18T11:26:41","subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Robert Mader writes:\n\n> One thing I'd like to highlight again for the record: while the patch here reduces glitches considerably for\n> me, I still occasionally see some when using GL clients like Snapshot.\n>\n> Assuming that's not because of hardware/driver bugs (I see it on quite different platforms), that could be\n> caused by us still waiting for implicit/explicit sync before reusing buffers.\n>\n> From https://docs.kernel.org/driver-api/dma-buf.html:\n>\n>> The synchronization provided via DMA_BUF_IOCTL_SYNC only provides cache coherency. It does not prevent\n>> other processes or devices from accessing the memory at the same time. If synchronization with a GPU or\n>> other device driver is required, it is the client’s responsibility to wait for buffer to be ready for\n>> reading or writing before calling this ioctl with DMA_BUF_SYNC_START. Likewise, the client must ensure\n>> that follow-up work is not submitted to GPU or other device driver until after this ioctl has been called\n>> with DMA_BUF_SYNC_END?\n>>\n>> If the driver or API with which the client is interacting uses implicit synchronization, waiting for prior\n>> work to complete can be done via poll() on the DMA buffer file descriptor. If the driver or API requires\n>> explicit synchronization, the client may have to wait on a sync_file or other synchronization primitive\n>> outside the scope of the DMA buffer API.\n>>\n> I wonder if we should add a swISP TODO for this - or is opening a bugzilla issue for tracking enough?\n\nI would prefer tracking it in bugzilla. I consider the software ISP\nTODO being a snapshot of the implementation requirements at the given\nmoment, which should be cleared ASAP and not used anymore.\n\n> P.S.: I'm not entirely sure if the udmabuf sync implementation really doesn't wait for implicit fences - but\n> if it does, there's no guarantee that things stay that way.\n>\n> On 16.09.24 18:12, Robert Mader wrote:\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 v4:\n>> - Fixed errno handling\n>> - Added sync flags to error message\n>> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n>> 1 file changed, 30 insertions(+)\n>>\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 077f7f4b..96cf2c71 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>> #include \n>> +#include \n>> #include \n>> +#include \n>> +\n>> #include \n>> #include \"libcamera/internal/bayer_format.h\"\n>> @@ -718,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n>> \t}\n>> }\n>> +namespace {\n>> +\n>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n>> +{\n>> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n>> +\t\tconst int fd = plane.fd.get();\n>> +\t\tstruct dma_buf_sync sync = { syncFlags };\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 << \" with flags \"\n>> +\t\t\t\t<< syncFlags << \" failed: \" << strerror(-ret);\n>> +\t\t}\n>> +\t}\n>> +}\n>> +\n>> static inline int64_t timeDiff(timespec &after, timespec &before)\n>> {\n>> \treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n>> \t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n>> }\n>> +} /* namespace */\n>> +\n>> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>> {\n>> \ttimespec frameStartTime;\n>> @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> \t}\n>> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n>> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n>> +\n>> \tgreen_ = params.green;\n>> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n>> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> @@ -760,6 +787,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n>> \tmetadata.planes()[0].bytesused = out.planes()[0].size();\n>> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n>> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n>> +\n>> \t/* Measure before emitting signals */\n>> \tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> \t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {","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 5BA46C324C\n\tfor ;\n\tWed, 18 Sep 2024 11:26:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 448C8618F0;\n\tWed, 18 Sep 2024 13:26:50 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2FA8618E8\n\tfor ;\n\tWed, 18 Sep 2024 13:26:48 +0200 (CEST)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-117-i2JFBy6VO6aHrAouUVyz3A-1; Wed, 18 Sep 2024 07:26:46 -0400","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5c250f3b18aso5060442a12.2\n\tfor ;\n\tWed, 18 Sep 2024 04:26:45 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5c42bb56b8bsm4830872a12.38.2024.09.18.04.26.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 18 Sep 2024 04:26:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"CiVzoCop\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726658807;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=N2n5vsjMxzYtTIa9OWE47jEonjoiPF8NBEk4p1HqA58=;\n\tb=CiVzoCopN4Vz4Qyds2couB3e7Hyky1jr2TRr6B03X+Trd2rnU8J5PBa4vi+PacHsKv6lue\n\tAP+04uPOTfahi3wkJA04lm5Ly2YvgOohLu2uyWDp0Ur/H868wNk4tAXEe9w/PXx9s94qRt\n\t75nZ5UrTxMRn9JwxBaFEv6pc6hzXlXg=","X-MC-Unique":"i2JFBy6VO6aHrAouUVyz3A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726658804; x=1727263604;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=J8RKqciSUl7Hwv0/LYWfbO7YiK5J/UJb2BwRYbL+lDo=;\n\tb=JF89md++Jf3tSPnd7n4EhZRaQYr4RwT7KFfUx72rInKIdqjmpimrdPnrE/Agnj/yGq\n\tZHcrGcuABznSX7aFe3X46ir0skmDJupYmsxL4AKz6c7K/z7oMcTj1/p86tUp5FpkSMdW\n\tlk1cIY9PDT8u6ZaADXYw3zUg603XwY43mRHuN8ysLKFljAqUhRvhqpthIcruJ9uYgzml\n\tdk/+kSjXb7neXtayO6GalIo+OKpRPCBV2/hQbV5iUtR2GqXFdk6dHfRkxKZNw9csSB2b\n\tD6/98eVzfSFChiQhiGU/kFOGGpajKsBsqjNAvnuKInQ5ZsKCU9Svsn1famKAmbufQgwB\n\t0EQg==","X-Gm-Message-State":"AOJu0YxMfZMF3pSZs5dvBTa3ygxvrIp5+dPFeMgnNZ9ZIoUAPINz5+Z0\n\tpnMwd7I4ZOLnwG4pByWlwgirs03dCkFlihRnUn6A97qzKF0fMgnfrCQuN4uU3Yg/TqOKl8F85WV\n\tjDg1uplIhBlrZCQEh9HYnZnG946wRf5boyFb/6DGuqf+1q++ofOWj14h8pIFEGfwTqXWQ3TzIQg\n\tWe0ua76nOXx0PI9yrv6oEIGSORbyx+59KwVT/KYBtPJtCFi3MWBvYi2CBmVg==","X-Received":["by 2002:a05:6402:5186:b0:5c0:902c:e195 with SMTP id\n\t4fb4d7f45d1cf-5c413cbb42bmr22068612a12.0.1726658804472; \n\tWed, 18 Sep 2024 04:26:44 -0700 (PDT)","by 2002:a05:6402:5186:b0:5c0:902c:e195 with SMTP id\n\t4fb4d7f45d1cf-5c413cbb42bmr22068578a12.0.1726658803888; \n\tWed, 18 Sep 2024 04:26:43 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGtscXlMNaslR/4/5ONH9RivVtfebJXYH29SntQnbN8h6H41k/sxFG8/l7c+bc0YjEXERv0UQ==","From":"Milan Zamazal ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","In-Reply-To":"<21a01871-d16f-4912-9a5c-bbd22324d307@collabora.com> (Robert\n\tMader's message of \"Wed, 18 Sep 2024 12:26:25 +0200\")","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<21a01871-d16f-4912-9a5c-bbd22324d307@collabora.com>","Date":"Wed, 18 Sep 2024 13:26:41 +0200","Message-ID":"<877cb9urri.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31268,"web_url":"https://patchwork.libcamera.org/comment/31268/","msgid":"<20240918154027.GD28469@pendragon.ideasonboard.com>","date":"2024-09-18T15:40:27","subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Sep 18, 2024 at 01:26:41PM +0200, Milan Zamazal wrote:\n> Robert Mader writes:\n> \n> > One thing I'd like to highlight again for the record: while the patch here reduces glitches considerably for\n> > me, I still occasionally see some when using GL clients like Snapshot.\n> >\n> > Assuming that's not because of hardware/driver bugs (I see it on quite different platforms), that could be\n> > caused by us still waiting for implicit/explicit sync before reusing buffers.\n> >\n> > From https://docs.kernel.org/driver-api/dma-buf.html:\n> >\n> >> The synchronization provided via DMA_BUF_IOCTL_SYNC only provides cache coherency. It does not prevent\n> >> other processes or devices from accessing the memory at the same time. If synchronization with a GPU or\n> >> other device driver is required, it is the client’s responsibility to wait for buffer to be ready for\n> >> reading or writing before calling this ioctl with DMA_BUF_SYNC_START. Likewise, the client must ensure\n> >> that follow-up work is not submitted to GPU or other device driver until after this ioctl has been called\n> >> with DMA_BUF_SYNC_END?\n> >>\n> >> If the driver or API with which the client is interacting uses implicit synchronization, waiting for prior\n> >> work to complete can be done via poll() on the DMA buffer file descriptor. If the driver or API requires\n> >> explicit synchronization, the client may have to wait on a sync_file or other synchronization primitive\n> >> outside the scope of the DMA buffer API.\n> >>\n> > I wonder if we should add a swISP TODO for this - or is opening a bugzilla issue for tracking enough?\n> \n> I would prefer tracking it in bugzilla. I consider the software ISP\n> TODO being a snapshot of the implementation requirements at the given\n> moment, which should be cleared ASAP and not used anymore.\n\nAgreed.\n\n> > P.S.: I'm not entirely sure if the udmabuf sync implementation really doesn't wait for implicit fences - but\n> > if it does, there's no guarantee that things stay that way.\n> >\n> > On 16.09.24 18:12, Robert Mader wrote:\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 v4:\n> >> - Fixed errno handling\n> >> - Added sync flags to error message\n> >> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> >> 1 file changed, 30 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> index 077f7f4b..96cf2c71 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> >> #include \n> >> +#include \n> >> #include \n> >> +#include \n> >> +\n> >> #include \n> >> #include \"libcamera/internal/bayer_format.h\"\n> >> @@ -718,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> >> \t}\n> >> }\n> >> +namespace {\n> >> +\n> >> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> >> +{\n> >> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> >> +\t\tconst int fd = plane.fd.get();\n> >> +\t\tstruct dma_buf_sync sync = { syncFlags };\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 << \" with flags \"\n> >> +\t\t\t\t<< syncFlags << \" failed: \" << strerror(-ret);\n> >> +\t\t}\n> >> +\t}\n> >> +}\n> >> +\n> >> static inline int64_t timeDiff(timespec &after, timespec &before)\n> >> {\n> >> \treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> >> \t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> >> }\n> >> +} /* namespace */\n> >> +\n> >> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> >> {\n> >> \ttimespec frameStartTime;\n> >> @@ -733,6 +757,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> >> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> >> \t}\n> >> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> >> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> >> +\n> >> \tgreen_ = params.green;\n> >> \tred_ = swapRedBlueGains_ ? params.blue : params.red;\n> >> \tblue_ = swapRedBlueGains_ ? params.red : params.blue;\n> >> @@ -760,6 +787,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> >> \tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> >> +\tsyncBufferForCPU(output, DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE);\n> >> +\tsyncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> >> +\n> >> \t/* Measure before emitting signals */\n> >> \tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> >> \t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\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 AD203C324C\n\tfor ;\n\tWed, 18 Sep 2024 15:41:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DFD6634FC;\n\tWed, 18 Sep 2024 17:41:03 +0200 (CEST)","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 B19E7618E8\n\tfor ;\n\tWed, 18 Sep 2024 17:41:00 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [185.44.53.103])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E01B121E;\n\tWed, 18 Sep 2024 17:39:37 +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=\"LJcgBvbE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726673978;\n\tbh=s0w14JFV9c8MBn9qX0aPjAxh+ciApVxy9t1XDlW1Wdg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LJcgBvbE4wPZdRgPvOuBmj41zX5WFLdhYDqVULWs1pFAaBlmPbc4ZHbmcp9bWsnLC\n\tptI7Tk8gRWtCV1byXpdbYSRs6YxX0EXWg5BPff3RPooim3QguR/6jGid7+dMORrY6A\n\tQDZbC1/yNw1QcGhw1o3iQPqussFm2nGfzUWgu7x4=","Date":"Wed, 18 Sep 2024 18:40:27 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","Message-ID":"<20240918154027.GD28469@pendragon.ideasonboard.com>","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<21a01871-d16f-4912-9a5c-bbd22324d307@collabora.com>\n\t<877cb9urri.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<877cb9urri.fsf@redhat.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":31269,"web_url":"https://patchwork.libcamera.org/comment/31269/","msgid":"<7eb9efc5-410f-47da-bfdb-c1ea1dffbed6@redhat.com>","date":"2024-09-18T19:03:47","subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 18-Sep-24 11:55 AM, Milan Zamazal wrote:\n> Kieran Bingham writes:\n> \n>> Quoting Robert Mader (2024-09-16 17:12:31)\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>>\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 v4:\n>>> - Fixed errno handling\n>>> - Added sync flags to error message\n>>> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n>>> 1 file changed, 30 insertions(+)\n>>>\n>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n>>> }\n>>> }\n>>> \n>>> +namespace {\n>>> +\n>>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n>>\n>> Now these are in an annoymous namespace, they don't need to be static -\n>> but that can be fixed while applying perhaps or ignored for now if no\n>> further version is required otherwise.\n>>\n>> I'm happy to merge, but under software_isp/* I think we should have an\n>> Acked-by/Reviewed-by by one of the SoftISP developers.\n>>\n>> Milan, Hans ? Are you happy with this patch? Is there any impact with\n>> the Intel devices for instance?\n> \n> I'm fine with the patch (with the exception below) and it works fine on\n> Debix (with only ~0,5% performance penalty). It would be nice if Hans\n> tested it on Intel.\n\nI have tested this on a ThinkPad X1 Yoga gen 8 with IPU6 + ov2740\nsensor running at 1920x1080. Things still work and the builtin\nbench function shows an increase of the frame-processing time\n(including the syncing) from ~6.1 ms/frame to ~6.3 ms/frame which\nI consider an acceptable slowdown:\n\nTested-by: Hans de Goede # IPU6 + ov2740\n\nRegards,\n\nHans\n\n\n\n\n\n\n> \n>> --\n>> Kieran\n>>\n>>\n>>\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>>> + int ret;\n>>> +\n>>> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n>>> + if (ret < 0) {\n>>> + ret = errno;\n>>> + LOG(Debayer, Error)\n>>> + << \"Syncing buffer FD \" << fd << \" with flags \"\n>>> + << syncFlags << \" failed: \" << strerror(-ret);\n> \n> I think `ret' and not `-ret' should be used here. And I would drop\n> `ret = errno' above and use errno here directly. I'm not sure why\n> Kieran suggested exactly this -- using strerror is right but changing\n> the sign seems wrong (tested in my environment).\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>>> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n>>> }\n>>> \n>>> +} /* namespace */\n>>> +\n>>> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>>> {\n>>> timespec frameStartTime;\n>>> @@ -733,6 +757,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 +787,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>>>\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 D5313C3257\n\tfor ;\n\tWed, 18 Sep 2024 19:03:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B32C3618F0;\n\tWed, 18 Sep 2024 21:03:55 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACB59618E8\n\tfor ;\n\tWed, 18 Sep 2024 21:03:53 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-86-1kOFR008OoiK3-Qqzkrejw-1; Wed, 18 Sep 2024 15:03:50 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a8a6fee3ab1so286758466b.3\n\tfor ;\n\tWed, 18 Sep 2024 12:03:50 -0700 (PDT)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5c42bb4972asm5199580a12.18.2024.09.18.12.03.47\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 18 Sep 2024 12:03:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"dWPbQ8Dd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1726686232;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=awCIgp3vsaEZUmyVBCDUP51OylFUtZWzORvpwt8QvjQ=;\n\tb=dWPbQ8DdufsPbqa08kLmhxFAe62W8H3m+cfDyohq658fn96MS8AbUZC8o/wf48zwjZhi2z\n\t1YIAJ2Q1tk3Nd54yUys8krMs5aypR6k23R79+A0Qu2XCwF41Ob9SKbvcGaexa6GGNqxtHQ\n\tt7Wj3n20pTqq1IY/btwJa4wiayEOZH4=","X-MC-Unique":"1kOFR008OoiK3-Qqzkrejw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726686230; x=1727291030;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=awCIgp3vsaEZUmyVBCDUP51OylFUtZWzORvpwt8QvjQ=;\n\tb=q9d86IUDyrFiwZ5ic56RvySp3q6XFuN0vJb9nSAkyK2QYVRR+Rf7bQ0oc5SZc4BprF\n\t4TDwTrn6p6+fzyx3/+zadL8vRikX5UgOg4gMYroU0/mOnfS2964ZlLQ8Pzdpnyy9/OLz\n\tE9WnHPoo1xs93Rhe0Qmd3+/tZfX4ZC94QwEIiWIhHbwjqaK5NU0a+AGe4FaBRCbEDXXI\n\t1qWXdbOt0nH/z9SFR1odyucxvSKmU7w2MSeBnZzj/nro8DXEiS99R/rme4E7sg8eAaUr\n\tvCrcigkSHY7K+lJ6VOmQBvtVAubl8sooDrMVsdkzIaRNupKWJT9wayNJQLZ+11i1u1NG\n\t/p4A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUCWqZ2ECrOxsAoDAJ09FS7Kv7uYVvyCbmcsEx+XsApFMiRI0E20+G1OVOXjlpXzXrnqv1NP0vGM+7QMOEKOvA=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwMC/gmN7/5eWGhoyml/50Vh8QiVjb5XZr88YkwUlGWIlQoxRZp\n\twpHthsr4sxrHAJJRYvn5a6Qb+Ra3L9ECxu+WdykGjxGJggaIOaJaCbiWYdjjfdi/LN5HvFt1D8B\n\tggEqjWR08TNdZQJdablYr2oYpDjd2PGsYXaVfEsr7hArPjomlLzi9HqxeLwpWwBapm3LXZCE=","X-Received":["by 2002:a05:6402:42c4:b0:5c4:135d:c4d6 with SMTP id\n\t4fb4d7f45d1cf-5c41e1acbbdmr17187210a12.22.1726686229519; \n\tWed, 18 Sep 2024 12:03:49 -0700 (PDT)","by 2002:a05:6402:42c4:b0:5c4:135d:c4d6 with SMTP id\n\t4fb4d7f45d1cf-5c41e1acbbdmr17187191a12.22.1726686228971; \n\tWed, 18 Sep 2024 12:03:48 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IE/bfe/ZdMtTULH9UL0i3b0hoSq2NvX9QdzpTpJthy8dQZlUW1r2loqqrHlgpY3ztO4Ph0+6Q==","Message-ID":"<7eb9efc5-410f-47da-bfdb-c1ea1dffbed6@redhat.com>","Date":"Wed, 18 Sep 2024 21:03:47 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","To":"Milan Zamazal ,\n\tKieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>\n\t<87jzf9uvz0.fsf@redhat.com>","From":"Hans de Goede ","In-Reply-To":"<87jzf9uvz0.fsf@redhat.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","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\" "}},{"id":31271,"web_url":"https://patchwork.libcamera.org/comment/31271/","msgid":"<172674095096.1721105.13706771924496654381@ping.linuxembedded.co.uk>","date":"2024-09-19T10:15:50","subject":"Re: [PATCH v4] 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 Hans de Goede (2024-09-18 20:03:47)\n> Hi,\n> \n> On 18-Sep-24 11:55 AM, Milan Zamazal wrote:\n> > Kieran Bingham writes:\n> > \n> >> Quoting Robert Mader (2024-09-16 17:12:31)\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> >>\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 v4:\n> >>> - Fixed errno handling\n> >>> - Added sync flags to error message\n> >>> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> >>> 1 file changed, 30 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> >>> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> >>> }\n> >>> }\n> >>> \n> >>> +namespace {\n> >>> +\n> >>> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> >>\n> >> Now these are in an annoymous namespace, they don't need to be static -\n> >> but that can be fixed while applying perhaps or ignored for now if no\n> >> further version is required otherwise.\n> >>\n> >> I'm happy to merge, but under software_isp/* I think we should have an\n> >> Acked-by/Reviewed-by by one of the SoftISP developers.\n> >>\n> >> Milan, Hans ? Are you happy with this patch? Is there any impact with\n> >> the Intel devices for instance?\n> > \n> > I'm fine with the patch (with the exception below) and it works fine on\n> > Debix (with only ~0,5% performance penalty). It would be nice if Hans\n> > tested it on Intel.\n> \n> I have tested this on a ThinkPad X1 Yoga gen 8 with IPU6 + ov2740\n> sensor running at 1920x1080. Things still work and the builtin\n> bench function shows an increase of the frame-processing time\n> (including the syncing) from ~6.1 ms/frame to ~6.3 ms/frame which\n> I consider an acceptable slowdown:\n> \n> Tested-by: Hans de Goede # IPU6 + ov2740\n\nChecking the stats is a great idea and I've just realised I can also do\nthis ;-)\n\nOn a Lenovo X13s with OV5675 2584x1944-ABGR8888 output pipeline:\n\nWithout the patch: 67446 (average of below)\n\n[0:53:27.924908852] [12249] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2026886us, 67562 us/frame\n[0:54:04.343961479] [12316] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2023919us, 67463 us/frame\n[0:56:43.206420524] [13810] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2021544us, 67384 us/frame\n[1:03:39.259725236] [15813] INFO Debayer debayer_cpu.cpp:772 Processed 30 frames in 2021270us, 67375 us/frame\n\n\n\nWith the patch: 67620 (average of below)\n\n[0:59:29.918112330] [14793] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2026329us, 67544 us/frame\n[1:00:04.847115778] [14834] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2033635us, 67787 us/frame\n[1:00:33.564276945] [15161] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2028140us, 67604 us/frame\n[1:01:26.971334924] [15200] INFO Debayer debayer_cpu.cpp:802 Processed 30 frames in 2026393us, 67546 us/frame\n\n\nI think that's a ... 0.25% slowdown? I think I can live with that if\nit's providing cache handling guarantees ;-)\n\nTested-by: Kieran Bingham # Lenovo X13s + OV5675\n\n--\nKieran\n\n\n> \n> Regards,\n> \n> Hans\n> \n> \n> \n> \n> \n> \n> > \n> >> --\n> >> Kieran\n> >>\n> >>\n> >>\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> >>> + int ret;\n> >>> +\n> >>> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> >>> + if (ret < 0) {\n> >>> + ret = errno;\n> >>> + LOG(Debayer, Error)\n> >>> + << \"Syncing buffer FD \" << fd << \" with flags \"\n> >>> + << syncFlags << \" failed: \" << strerror(-ret);\n> > \n> > I think `ret' and not `-ret' should be used here. And I would drop\n> > `ret = errno' above and use errno here directly. I'm not sure why\n> > Kieran suggested exactly this -- using strerror is right but changing\n> > the sign seems wrong (tested in my environment).\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> >>> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> >>> }\n> >>> \n> >>> +} /* namespace */\n> >>> +\n> >>> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> >>> {\n> >>> timespec frameStartTime;\n> >>> @@ -733,6 +757,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 +787,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> >>>\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 269D5C3257\n\tfor ;\n\tThu, 19 Sep 2024 10:15:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF9CD634FC;\n\tThu, 19 Sep 2024 12:15:55 +0200 (CEST)","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 2A54E618E7\n\tfor ;\n\tThu, 19 Sep 2024 12:15:54 +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 B82F2236;\n\tThu, 19 Sep 2024 12:14:30 +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=\"ZyAL9Ea3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726740870;\n\tbh=j5a1P+FHk97gu4ItUXBPUPu3YMdkq1mVv11Oq1GEuC4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZyAL9Ea3cS8vAPOMwbMBtz2MTAWo6l+jm/2LlSPn2djoxwz/UfcZbjZnvVs5qxDfX\n\t3bRSOqDD/jMs1G3rMPMiON6XsY0Jzmaj7pkblQS+OFNhEvZihgfFnJcjiXlaWmiq0H\n\tKO1nanhFiqoTQ0BezL5x3GXYAUzDEK8z+3ENjmX8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<7eb9efc5-410f-47da-bfdb-c1ea1dffbed6@redhat.com>","References":"<20240916161231.238116-1-robert.mader@collabora.com>\n\t<172657448433.1366291.18424755118505899651@ping.linuxembedded.co.uk>\n\t<87jzf9uvz0.fsf@redhat.com>\n\t<7eb9efc5-410f-47da-bfdb-c1ea1dffbed6@redhat.com>","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","To":"Hans de Goede , Milan Zamazal ","Date":"Thu, 19 Sep 2024 11:15:50 +0100","Message-ID":"<172674095096.1721105.13706771924496654381@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":31284,"web_url":"https://patchwork.libcamera.org/comment/31284/","msgid":"<172683282007.129190.17059992952570216477@ping.linuxembedded.co.uk>","date":"2024-09-20T11:47:00","subject":"Re: [PATCH v4] 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 17:12:31)\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 v4:\n> - Fixed errno handling\n> - Added sync flags to error message\n> - Wrapped syncBufferForCPU() and timeDiff() into an anonymous namespace\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 | 30 ++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 077f7f4b..96cf2c71 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,12 +721,33 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> }\n> }\n> \n> +namespace {\n> +\n> +static void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n\nDoesn't need to be static.\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> + int ret;\n> +\n> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> + if (ret < 0) {\n> + ret = errno;\n> + LOG(Debayer, Error)\n> + << \"Syncing buffer FD \" << fd << \" with flags \"\n> + << syncFlags << \" failed: \" << strerror(-ret);\n\nAs noted by Milan, my proposal was wrong here. This should be\nstrerror(ret).\n\n> + }\n> + }\n> +}\n> +\n> static inline int64_t timeDiff(timespec &after, timespec &before)\n\nAnd we can remove this static at the same time.\n\nWith those ...\n\nReviewed-by: Kieran Bingham \n\nBut I've already fixed those up locally so I'll send v5 which I think\ncan then be merged so no further action required.\n\n--\nKieran\n\n> {\n> return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> }\n> \n> +} /* namespace */\n> +\n> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> {\n> timespec frameStartTime;\n> @@ -733,6 +757,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 +787,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 751C8C3257\n\tfor ;\n\tFri, 20 Sep 2024 11:47:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 252C9634FC;\n\tFri, 20 Sep 2024 13:47:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F319618E0\n\tfor ;\n\tFri, 20 Sep 2024 13:47:03 +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 EE01C4CE;\n\tFri, 20 Sep 2024 13:45:38 +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=\"vN4vA2zs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726832739;\n\tbh=+p64m4LALHMP0xMU4RoPUOvyyuMm18hjn+byN8VSNJA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vN4vA2zsF4IBXxOsbGrdziLImonsDgQ6dsbjrY/GgVqFUA0Bcyt/vYzixmUK42nq5\n\taNAjQfrkQSSL6XUFPSgrddKPSmISWdtLdwUFSv2ATMseq4hVI+lxu/ZZEVYumIh6tD\n\txOvNG5qNUSOFVtfV3Haldy4cUMto36VTZAq3uDvw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240916161231.238116-1-robert.mader@collabora.com>","References":"<20240916161231.238116-1-robert.mader@collabora.com>","Subject":"Re: [PATCH v4] libcamera: debayer_cpu: Sync DMABUFs","From":"Kieran Bingham ","Cc":"Robert Mader ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 20 Sep 2024 12:47:00 +0100","Message-ID":"<172683282007.129190.17059992952570216477@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\" "}}]