[{"id":33048,"web_url":"https://patchwork.libcamera.org/comment/33048/","msgid":"<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-13T14:44:04","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nHarvey Yang writes:\n\n> As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n> instance would trigger sync end earlier than expected. This patch makes\n> it non-copyable to avoid the issue.\n\nAfter this patch, software ISP still works but reports the following\nerror on each frame:\n\n ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor, flags: 5\n\nDmaSyncer is used only in debayer_cpu.cpp:\n\n std::vector dmaSyncers;\n for (const FrameBuffer::Plane &plane : input->planes())\n \tdmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n \n for (const FrameBuffer::Plane &plane : output->planes())\n \tdmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n \n green_ = params.green;\n red_ = swapRedBlueGains_ ? params.blue : params.red;\n blue_ = swapRedBlueGains_ ? params.red : params.blue;\n \n /* Copy metadata from the input buffer */\n FrameMetadata &metadata = output->_d()->metadata();\n metadata.status = input->metadata().status;\n metadata.sequence = input->metadata().sequence;\n metadata.timestamp = input->metadata().timestamp;\n \n MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n if (!in.isValid() || !out.isValid()) {\n \tLOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n \tmetadata.status = FrameMetadata::FrameError;\n \treturn;\n }\n \n stats_->startFrame();\n \n if (inputConfig_.patternSize.height == 2)\n \tprocess2(in.planes()[0].data(), out.planes()[0].data());\n else\n \tprocess4(in.planes()[0].data(), out.planes()[0].data());\n \n metadata.planes()[0].bytesused = out.planes()[0].size();\n \n dmaSyncers.clear();\n\nAny idea what could be wrong?\n\n> Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n> Signed-off-by: Harvey Yang \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n> src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n> 2 files changed, 17 insertions(+)\n>\n> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> index fc5de2c13edd..d26f8a74f4c6 100644\n> --- a/include/libcamera/internal/dma_buf_allocator.h\n> +++ b/include/libcamera/internal/dma_buf_allocator.h\n> @@ -60,9 +60,14 @@ public:\n> \n> \texplicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n> \n> +\tDmaSyncer(DmaSyncer &&other) = default;\n> +\tDmaSyncer &operator=(DmaSyncer &&other) = default;\n> +\n> \t~DmaSyncer();\n> \n> private:\n> +\tLIBCAMERA_DISABLE_COPY(DmaSyncer)\n> +\n> \tvoid sync(uint64_t step);\n> \n> \tSharedFD fd_;\n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index 3cc52f9686b0..a014c3b4263c 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n> \tsync(DMA_BUF_SYNC_START);\n> }\n> \n> +/**\n> + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n> + * \\param[in] other The other instance\n> + * \\brief Enable move on class DmaSyncer\n> + */\n> +\n> +/**\n> + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n> + * \\param[in] other The other instance\n> + * \\brief Enable move on class DmaSyncer\n> + */\n> +\n> DmaSyncer::~DmaSyncer()\n> {\n> \tsync(DMA_BUF_SYNC_END);","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 BA133C326C\n\tfor ;\n\tMon, 13 Jan 2025 14:44:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91FDF68503;\n\tMon, 13 Jan 2025 15:44:12 +0100 (CET)","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 CB86A684E7\n\tfor ;\n\tMon, 13 Jan 2025 15:44:09 +0100 (CET)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-52-zPj7hgVmMFWmWouddv3n8w-1; Mon, 13 Jan 2025 09:44:07 -0500","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-385f0829430so2774758f8f.1\n\tfor ;\n\tMon, 13 Jan 2025 06:44:06 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-436f04df606sm112961035e9.12.2025.01.13.06.44.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Jan 2025 06:44:04 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"hMbzx1P1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1736779448;\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=UxU8pGqaIuj71HCO4dSHfGRYJwynG0Ig1ijNj4QNcc8=;\n\tb=hMbzx1P1YRuanboDVhXh/Aj96OVHAiTgrV+AWoXnsT8Fjy/HYGB81KBiWJCaJR1Knfn5Aj\n\t8aADIDl7+53xmUnaYByqvHDfrKsmER+KV10pBbVuo/06hgRVMFrpb1IG86hlqanJ6FbZop\n\tChi99Ia1r0sz2tOA3bo8Yvj9BqvHuMs=","X-MC-Unique":"zPj7hgVmMFWmWouddv3n8w-1","X-Mimecast-MFC-AGG-ID":"zPj7hgVmMFWmWouddv3n8w","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736779446; x=1737384246;\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=UxU8pGqaIuj71HCO4dSHfGRYJwynG0Ig1ijNj4QNcc8=;\n\tb=Lm6QuGnSRpeMxUvuaf1P8bTTKxSHWuHesbgWLRlyxZntFQ+LIzAFCHK6Bm12BBgmTz\n\tO4HtmSmJzGrQNaf3mvAkYmPHwFd+lvZnncdLL+1uSbvdcxdLeB9eAYELl/S+vDKrumv4\n\ttVG6UfqlIFiXu4Pu7TsnLawAJrGtIKczHEEXSckHCx9DLj4SldLLPc8SoP/xpTwrrbJm\n\tm2yJAn5DbR+4QHqTW3gMxDxDC+moXfy8fAlka6LiRgRv4RoeT9jCyOp9cgbiP93kyh38\n\tURYFq58xamRb64VTdmHhkpFZ4wVzj52D9+tR1W43vssebOwt5Ti554eXeLEIe/8w6whZ\n\tXZAA==","X-Gm-Message-State":"AOJu0Yx9I8UrCgOX0a8CmxjCa39EXVoe+Tx38PumYY4D9jOwZr06hRB9\n\tz2lzZW5bft8v9zyQPFnP9U4c5aW+H+NozkeTpA+bzIYsOdR9mxJwnbJ0VtZ3fb7CFEeCaEcBKkX\n\tjaTwqBquigRWZnGxKbA8RtocVV1U1Vvx3brCBPr5+IcZy/IDzXh3tjVr/33yl82WkdmiV66U=","X-Gm-Gg":"ASbGncs72LQe7R+RllQoihjRd5nn1GPEY7gbwuDopb9bUdeyYHFPQBAtbI8yrXi6ltm\n\tURqT4kVBG9djaw99qklpyAyjlWWGzKuUp+BOGpXysbVjONuRBMiG7dqmnKepx9LoDZHQRJfoBU6\n\t6swjFagfppWoyISudipScFDSvC9OyXAhh9AAl0hvGJOjNMy7ZSwhZKBPl/B+OF4TLClBOWXES5a\n\tpNJq3oAwqPaE006QW3wqmnuBN5jre+CcoWFJerEpzvx7c7DzvhlI3RGj5/EHnehnswBH64wftIQ\n\tYYQJsp+J/e0kciJE/3L2Dg1/4nkUvzdhVQ==","X-Received":["by 2002:a05:600c:3506:b0:434:f4fa:83c4 with SMTP id\n\t5b1f17b1804b1-436e26f137dmr199358545e9.29.1736779445608; \n\tMon, 13 Jan 2025 06:44:05 -0800 (PST)","by 2002:a05:600c:3506:b0:434:f4fa:83c4 with SMTP id\n\t5b1f17b1804b1-436e26f137dmr199358345e9.29.1736779445245; \n\tMon, 13 Jan 2025 06:44:05 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IErucKVHV+sC2fxJBkInmoaMIESKaPQHDlXM/4NU1VER3ZQeBRiVvUCCUbb0vPyW8F96B854w==","From":"Milan Zamazal ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","In-Reply-To":"<20241211084433.3039442-1-chenghaoyang@chromium.org> (Harvey\n\tYang's message of \"Wed, 11 Dec 2024 08:44:27 +0000\")","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>","Date":"Mon, 13 Jan 2025 15:44:04 +0100","Message-ID":"<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"sF5UZp7j3MPMLChdCN24yxNuHJBEoWH4uo6wi6JztJw_1736779446","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":33049,"web_url":"https://patchwork.libcamera.org/comment/33049/","msgid":"","date":"2025-01-13T14:56:07","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Milan,\n\nOn Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal wrote:\n>\n> Hi,\n>\n> Harvey Yang writes:\n>\n> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n> > instance would trigger sync end earlier than expected. This patch makes\n> > it non-copyable to avoid the issue.\n>\n> After this patch, software ISP still works but reports the following\n> error on each frame:\n\nMay I know what the environment is? (Unit tests or specific tests\nyou're running?\n\n>\n> ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor, flags: 5\n\nBased on the log, `fd_.get()` is -1. Could you confirm that\n`plane.fd.get()` below is never `-1`?\n\nBR,\nHarvey\n\n>\n> DmaSyncer is used only in debayer_cpu.cpp:\n>\n> std::vector dmaSyncers;\n> for (const FrameBuffer::Plane &plane : input->planes())\n> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n>\n> for (const FrameBuffer::Plane &plane : output->planes())\n> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n>\n> green_ = params.green;\n> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>\n> /* Copy metadata from the input buffer */\n> FrameMetadata &metadata = output->_d()->metadata();\n> metadata.status = input->metadata().status;\n> metadata.sequence = input->metadata().sequence;\n> metadata.timestamp = input->metadata().timestamp;\n>\n> MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n> if (!in.isValid() || !out.isValid()) {\n> LOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n> metadata.status = FrameMetadata::FrameError;\n> return;\n> }\n>\n> stats_->startFrame();\n>\n> if (inputConfig_.patternSize.height == 2)\n> process2(in.planes()[0].data(), out.planes()[0].data());\n> else\n> process4(in.planes()[0].data(), out.planes()[0].data());\n>\n> metadata.planes()[0].bytesused = out.planes()[0].size();\n>\n> dmaSyncers.clear();\n>\n> Any idea what could be wrong?\n>\n> > Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n> > Signed-off-by: Harvey Yang \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n> > src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n> > 2 files changed, 17 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> > index fc5de2c13edd..d26f8a74f4c6 100644\n> > --- a/include/libcamera/internal/dma_buf_allocator.h\n> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n> > @@ -60,9 +60,14 @@ public:\n> >\n> > explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n> >\n> > + DmaSyncer(DmaSyncer &&other) = default;\n> > + DmaSyncer &operator=(DmaSyncer &&other) = default;\n> > +\n> > ~DmaSyncer();\n> >\n> > private:\n> > + LIBCAMERA_DISABLE_COPY(DmaSyncer)\n> > +\n> > void sync(uint64_t step);\n> >\n> > SharedFD fd_;\n> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> > index 3cc52f9686b0..a014c3b4263c 100644\n> > --- a/src/libcamera/dma_buf_allocator.cpp\n> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n> > sync(DMA_BUF_SYNC_START);\n> > }\n> >\n> > +/**\n> > + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n> > + * \\param[in] other The other instance\n> > + * \\brief Enable move on class DmaSyncer\n> > + */\n> > +\n> > +/**\n> > + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n> > + * \\param[in] other The other instance\n> > + * \\brief Enable move on class DmaSyncer\n> > + */\n> > +\n> > DmaSyncer::~DmaSyncer()\n> > {\n> > sync(DMA_BUF_SYNC_END);\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 92533C32F6\n\tfor ;\n\tMon, 13 Jan 2025 14:56:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EE496851C;\n\tMon, 13 Jan 2025 15:56:21 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8D2C684E7\n\tfor ;\n\tMon, 13 Jan 2025 15:56:18 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id\n\t38308e7fff4ca-30613802a04so18670811fa.2\n\tfor ;\n\tMon, 13 Jan 2025 06:56:18 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"jM3cb6YB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1736780178; x=1737384978;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=FTeUCbSf48Tk288Obyy6KltTuCdeaVtaFLcZUfAtXzw=;\n\tb=jM3cb6YBxH7YpJOyo76rGtuxjij6Em837q0E387S7jH2Vcx7n5GC6tzDTrU0F90Wxy\n\tp6aKwlc/vqghZPDRJO1cXYjnxchyd4r3cXRfufwe7hKVuevgGYSR+2cf3uH77l8KN/gy\n\tSGUYsSiV1HJ9n1WwiUKO2ND9QQTOOlNXRyjH4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736780178; x=1737384978;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=FTeUCbSf48Tk288Obyy6KltTuCdeaVtaFLcZUfAtXzw=;\n\tb=LMMpDwiThSztOMR+Os+XtCWiVSvfgwsOEP/UJoe61dphQHN980JFQtlb2KK+stc5Ka\n\tyQsc1JNcWAc/7049CrH++oGpzd8pV/cYawt1uY1zNzWm6ltkLINNdE6KehyMVu5a7e3o\n\twJB5CoP6xtAkcZihtimSUZjfi/mTbtbHzg+0I41oqkg7ISjMcJFH47WqGDB5bGj6yKoS\n\tVRG1FyzQEavQ/gfAb35ZSDl2SWCvOXrIbSiIm3w9ZIp4ywA+kvplHp5b3BiGKVoOTL6E\n\tPf8zwItU4s/SB6h1wJPsgUnjBJTEkj0dG7orsX1AbGdq8POX9G5APQ010lmJyzZXU7no\n\tPykw==","X-Gm-Message-State":"AOJu0YxNYUHvSCEmPRUhxYEc2HWcds6LXh1SAgXghOnT3c6eIPKuUKvO\n\t0VUCi+w+Yh1WB3jaq/omoD3S15JXN/+mW1YgxL6ODq7z21ng5mlnJuV9r69gRIiswNXfQeR65f8\n\tIjs/78tMddT78ND9SqGiG82DDmwNyyPOCvbzy","X-Gm-Gg":"ASbGncsDMhR1VLTmqpuVAH7/MsiMWdr/vbZdfE+ZznvaEunqtYEe0Wv/Q/ufC4JIMSv\n\t7dz+oxE+y6vpEZpKansfS8t+zf3fm8SIoO4ow","X-Google-Smtp-Source":"AGHT+IEDvsrLKALEEGahq/0fhViokCtI5JLrZReiif04oYq5nJ5Fji/0J+cS5kDyWyoLFlJweI2pMXYxhpi70KXpwfg=","X-Received":"by 2002:a05:651c:4cb:b0:304:4e03:f9d9 with SMTP id\n\t38308e7fff4ca-305f45e84b9mr68988221fa.28.1736780177988;\n\tMon, 13 Jan 2025 06:56:17 -0800 (PST)","MIME-Version":"1.0","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>\n\t<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","In-Reply-To":"<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","From":"Cheng-Hao Yang ","Date":"Mon, 13 Jan 2025 15:56:07 +0100","X-Gm-Features":"AbW1kvaH-xtBJkGbCvsG-XyHpgk6rLs81kQyjyRZJutPK8I6fGVjtXmTptXlkbQ","Message-ID":"","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, \n\tKieran Bingham , \n\tLaurent Pinchart ","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":33050,"web_url":"https://patchwork.libcamera.org/comment/33050/","msgid":"<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-13T16:16:46","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Harvey,\n\nCheng-Hao Yang writes:\n\n> Hi Milan,\n>\n> On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal wrote:\n>>\n>> Hi,\n>>\n>> Harvey Yang writes:\n>>\n>> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n>> > instance would trigger sync end earlier than expected. This patch makes\n>> > it non-copyable to avoid the issue.\n>>\n>> After this patch, software ISP still works but reports the following\n>> error on each frame:\n>\n> May I know what the environment is? (Unit tests or specific tests\n> you're running?\n\nNo tests, running `cam' with software ISP.\n\n>> ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor,\n>> flags: 5\n>\n> Based on the log, `fd_.get()` is -1. Could you confirm that\n> `plane.fd.get()` below is never `-1`?\n\nYes, they are never -1.\n\nThe problem occurs in the following line from the snippet below:\n\n dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n\nIt processes the input plane fine and calls DmaSyncer::sync with the\nright fd, then it process the output plane and it first calls\nDmaSyncer::sync with the right fd, followed by a call with fd -1.\nHere is the backtrace of the corresponding call (note the\nline numbers are a bit off due to debugging prints I inserted):\n\n #0 0x0000fffff7f0810c in libcamera::DmaSyncer::sync (this=this@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341\n #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@entry=0xffffe4000bf0, __in_chrg=) at ../src/libcamera/dma_buf_allocator.cpp:329\n #2 0x0000fffff7f68fe4 in std::_Destroy (__pointer=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:151\n #3 std::_Destroy_aux::__destroy (__last=, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:163\n #4 std::_Destroy (__last=, __first=) at /usr/include/c++/11/bits/stl_construct.h:196\n #5 std::_Destroy (__last=0xffffe4000c08, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/alloc_traits.h:848\n #6 std::vector >::_M_realloc_insert (this=0xffffefe0d738, __position=..., \n __position@entry=...) at /usr/include/c++/11/bits/vector.tcc:498\n #7 0x0000fffff7f691b4 in std::vector >::emplace_back (this=this@entry=0xffffefe0d738)\n at /usr/include/c++/11/bits/vector.tcc:121\n #8 0x0000fffff7f66d38 in libcamera::DebayerCpu::process (this=0xfffff001e8d0, frame=0, input=0xfffff0020270, output=0xfffff0018f70, params=...) at ../src/libcamera/software_isp/debayer_cpu.cpp:752\n\nThis means DmaSyncer::sync is called twice within emplace_back, the\nsecond time from the destructor with the invalid fd. This is weird,\nwhat's the destroyed instance and why does it happen only with the\noutput plane and not the input plane? dmaSyncers is a local variable\nnot used anywhere else, I suspect the vector gets resized and some\nunwanted actions on the instances happen during the operation.\n\n> BR,\n> Harvey\n>\n>>\n>> DmaSyncer is used only in debayer_cpu.cpp:\n>>\n>> std::vector dmaSyncers;\n>> for (const FrameBuffer::Plane &plane : input->planes())\n>> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n>>\n>> for (const FrameBuffer::Plane &plane : output->planes())\n>> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n>>\n>> green_ = params.green;\n>> red_ = swapRedBlueGains_ ? params.blue : params.red;\n>> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>>\n>> /* Copy metadata from the input buffer */\n>> FrameMetadata &metadata = output->_d()->metadata();\n>> metadata.status = input->metadata().status;\n>> metadata.sequence = input->metadata().sequence;\n>> metadata.timestamp = input->metadata().timestamp;\n>>\n>> MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n>> MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n>> if (!in.isValid() || !out.isValid()) {\n>> LOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n>> metadata.status = FrameMetadata::FrameError;\n>> return;\n>> }\n>>\n>> stats_->startFrame();\n>>\n>> if (inputConfig_.patternSize.height == 2)\n>> process2(in.planes()[0].data(), out.planes()[0].data());\n>> else\n>> process4(in.planes()[0].data(), out.planes()[0].data());\n>>\n>> metadata.planes()[0].bytesused = out.planes()[0].size();\n>>\n>> dmaSyncers.clear();\n>>\n>> Any idea what could be wrong?\n>>\n>> > Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n>> > Signed-off-by: Harvey Yang \n>> > Reviewed-by: Kieran Bingham \n>> > Reviewed-by: Laurent Pinchart \n>> > ---\n>> > include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n>> > src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n>> > 2 files changed, 17 insertions(+)\n>> >\n>> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n>> > index fc5de2c13edd..d26f8a74f4c6 100644\n>> > --- a/include/libcamera/internal/dma_buf_allocator.h\n>> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n>> > @@ -60,9 +60,14 @@ public:\n>> >\n>> > explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n>> >\n>> > + DmaSyncer(DmaSyncer &&other) = default;\n>> > + DmaSyncer &operator=(DmaSyncer &&other) = default;\n>> > +\n>> > ~DmaSyncer();\n>> >\n>> > private:\n>> > + LIBCAMERA_DISABLE_COPY(DmaSyncer)\n>> > +\n>> > void sync(uint64_t step);\n>> >\n>> > SharedFD fd_;\n>> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n>> > index 3cc52f9686b0..a014c3b4263c 100644\n>> > --- a/src/libcamera/dma_buf_allocator.cpp\n>> > +++ b/src/libcamera/dma_buf_allocator.cpp\n>> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n>> > sync(DMA_BUF_SYNC_START);\n>> > }\n>> >\n>> > +/**\n>> > + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n>> > + * \\param[in] other The other instance\n>> > + * \\brief Enable move on class DmaSyncer\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n>> > + * \\param[in] other The other instance\n>> > + * \\brief Enable move on class DmaSyncer\n>> > + */\n>> > +\n>> > DmaSyncer::~DmaSyncer()\n>> > {\n>> > sync(DMA_BUF_SYNC_END);\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 DF1FFC326C\n\tfor ;\n\tMon, 13 Jan 2025 16:16:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A3E768503;\n\tMon, 13 Jan 2025 17:16:54 +0100 (CET)","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 5D94E684E7\n\tfor ;\n\tMon, 13 Jan 2025 17:16:52 +0100 (CET)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-636-nWTBKTArMJW9tATvWKWQaw-1; Mon, 13 Jan 2025 11:16:49 -0500","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-4361ac8b25fso25154505e9.2\n\tfor ;\n\tMon, 13 Jan 2025 08:16:49 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-436e2ddca2dsm182895285e9.21.2025.01.13.08.16.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Jan 2025 08:16:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"HMqb7SUi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1736785011;\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=OvZ6iufHDFoIWKrDHU5qauYSEIAOYpwQ7Zs/i+QZUWM=;\n\tb=HMqb7SUi2SWKggw0mm9/P7vzPLG2C+VVK2U0pE8xfXfUfhesgNpftjyPALPGtzXEvkN3SL\n\toiZJuPoQExu+624iPTSD4Z1VnBdBohLBnhkAKstQswOsJGnG9WzjLPXCqkxDDzFmJSepXR\n\tEoFukbRADrGJZK2jPiSVK/m9b8O8y5k=","X-MC-Unique":"nWTBKTArMJW9tATvWKWQaw-1","X-Mimecast-MFC-AGG-ID":"nWTBKTArMJW9tATvWKWQaw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736785008; x=1737389808;\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=+9k9q8BCFiKH8JRViug1V5+PGucDnDRAC2aGlE7yBV8=;\n\tb=UF8PISN1np42QVfbSsnltvMo4AQF5f4otdhGYOH1aAAaNnjK+rPacPD9IPMGzdbiI2\n\t6cBRyT2NK5gGxAzf5d9ZfTzI0rLNETKE+cvKqkHWAEQwCiAIvTrWsbliLFQctFzfpQu7\n\tmD6usTX7U40GU70GW+r+3DIZv9cu4a0Lq8nRizcd/BGJLp0nz+UT7wF7Nb4M3/eLhSMU\n\t9gQKi2EMDynVVQPzt75czrcMPoZ4xRDwk43dzqhcPr+86wqx7t4tED0AGUbpLDDZeyhY\n\tf9owO3QXTVi05ZbqG1B58tfR97JyOdldl/SxXpbp3LdaFxwmkDPfTRbNdOAiDSI4M1ib\n\tgXaA==","X-Gm-Message-State":"AOJu0Yy0/NkKHVbDxnfg//uYJQTnW6nViAloihqb7ITA6YLjHlnrk7XI\n\tullqYrYkeI/drI7ZpyPJbLz45Nu7IwGKVt6HZI8Hm4i2PKArf9pP/uf3gM0KazMV0dXumrRgVyk\n\thkXhGQFtlUGDyIApT1a+pcUNyZyq1MT1xIXcqA33uKxhx0YTbyF4fsbAemJjUhchZ9tP5bUo=","X-Gm-Gg":"ASbGncstAlt5/OBkWxE1diCvbGwMIDuN7WvP81JWuIgeOAhF5tpiLT30FKFKmy942pA\n\tYaO7sdSb1W0SV6T8cTsv29yAmyRGtqIZ7+IG3zNjx2IGre8l1HXiiSZYk3Yiw80NOSRdJ7jL2wK\n\tAgkCEELVP0VsKvgphYrwr7gvwezDfHMvhXPodKUkcfkgEZWnDecTPaRUmx5sCvQzXpmtof4KBLh\n\txR+1nRyIS6McOD/WBevmn9K+sSdPxg/zglgvRbFgSjYQis9a+ScFSYnBZCfSy05xXRleNfPFvsT\n\tI1HZO43/BZ/YEqJSpRQtyUEF/JeylT48kg==","X-Received":["by 2002:a05:600c:4fc2:b0:434:f3d8:62d0 with SMTP id\n\t5b1f17b1804b1-436e26803f4mr185989565e9.3.1736785007871; \n\tMon, 13 Jan 2025 08:16:47 -0800 (PST)","by 2002:a05:600c:4fc2:b0:434:f3d8:62d0 with SMTP id\n\t5b1f17b1804b1-436e26803f4mr185989315e9.3.1736785007503; \n\tMon, 13 Jan 2025 08:16:47 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IFPSwuZaWgKMioI7YdQYSLFDJgf2OVKh10BDSel8sYjQxnP607QFL8saF9Uisumc1dK24dr9A==","From":"Milan Zamazal ","To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","In-Reply-To":"\n\t(Cheng-Hao Yang's message of \"Mon, 13 Jan 2025 15:56:07 +0100\")","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>\n\t<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t","Date":"Mon, 13 Jan 2025 17:16:46 +0100","Message-ID":"<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"yjiJHADN_PYBSSAvD_DNmAlFT82M_dBRSbdhmoexYhk_1736785008","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":33051,"web_url":"https://patchwork.libcamera.org/comment/33051/","msgid":"","date":"2025-01-13T16:26:31","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal írta:\n\n> Hi Harvey,\n> \n> Cheng-Hao Yang writes:\n> \n> > Hi Milan,\n> >\n> > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal wrote:\n> >>\n> >> Hi,\n> >>\n> >> Harvey Yang writes:\n> >>\n> >> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n> >> > instance would trigger sync end earlier than expected. This patch makes\n> >> > it non-copyable to avoid the issue.\n> >>\n> >> After this patch, software ISP still works but reports the following\n> >> error on each frame:\n> >\n> > May I know what the environment is? (Unit tests or specific tests\n> > you're running?\n> \n> No tests, running `cam' with software ISP.\n> \n> >> ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor,\n> >> flags: 5\n> >\n> > Based on the log, `fd_.get()` is -1. Could you confirm that\n> > `plane.fd.get()` below is never `-1`?\n> \n> Yes, they are never -1.\n> \n> The problem occurs in the following line from the snippet below:\n> \n> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n> \n> It processes the input plane fine and calls DmaSyncer::sync with the\n> right fd, then it process the output plane and it first calls\n> DmaSyncer::sync with the right fd, followed by a call with fd -1.\n> Here is the backtrace of the corresponding call (note the\n> line numbers are a bit off due to debugging prints I inserted):\n> \n> #0 0x0000fffff7f0810c in libcamera::DmaSyncer::sync (this=this@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341\n> #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@entry=0xffffe4000bf0, __in_chrg=) at ../src/libcamera/dma_buf_allocator.cpp:329\n> #2 0x0000fffff7f68fe4 in std::_Destroy (__pointer=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:151\n> #3 std::_Destroy_aux::__destroy (__last=, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:163\n> #4 std::_Destroy (__last=, __first=) at /usr/include/c++/11/bits/stl_construct.h:196\n> #5 std::_Destroy (__last=0xffffe4000c08, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/alloc_traits.h:848\n> #6 std::vector >::_M_realloc_insert (this=0xffffefe0d738, __position=...,\n> __position@entry=...) at /usr/include/c++/11/bits/vector.tcc:498\n> #7 0x0000fffff7f691b4 in std::vector >::emplace_back (this=this@entry=0xffffefe0d738)\n> at /usr/include/c++/11/bits/vector.tcc:121\n> #8 0x0000fffff7f66d38 in libcamera::DebayerCpu::process (this=0xfffff001e8d0, frame=0, input=0xfffff0020270, output=0xfffff0018f70, params=...) at ../src/libcamera/software_isp/debayer_cpu.cpp:752\n> \n> This means DmaSyncer::sync is called twice within emplace_back, the\n> second time from the destructor with the invalid fd. This is weird,\n> what's the destroyed instance and why does it happen only with the\n> output plane and not the input plane? dmaSyncers is a local variable\n> not used anywhere else, I suspect the vector gets resized and some\n> unwanted actions on the instances happen during the operation.\n\nWhen the vector is resized, it must destroy the objects in the old storage.\nSee the call to `_M_realloc_insert()` in the stack trace. Hence the call to the\ndestructor during `emplace_back()`.\n\nAnd the reason why this did not happen previously is that previously the vector\nselected the copy constructor for moving the objects between allocation because\nthe move constructor is not `noexcept` (because `SharedFD`'s corresponding methods\nare not `noexcept` - this should probably be fixed in any case).\n\nBut since this change it must choose the move constructor because the copy\nconstructor is deleted. And the move constructor leaves `SharedFd::fd_ == nullptr`,\nso `SharedFD::get()` returns -1.\n\nThis also means that previously the syncs were probably off due to the destructor calls.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > BR,\n> > Harvey\n> >\n> >>\n> >> DmaSyncer is used only in debayer_cpu.cpp:\n> >>\n> >> std::vector dmaSyncers;\n> >> for (const FrameBuffer::Plane &plane : input->planes())\n> >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n> >>\n> >> for (const FrameBuffer::Plane &plane : output->planes())\n> >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n> >>\n> >> green_ = params.green;\n> >> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> >> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> >>\n> >> /* Copy metadata from the input buffer */\n> >> FrameMetadata &metadata = output->_d()->metadata();\n> >> metadata.status = input->metadata().status;\n> >> metadata.sequence = input->metadata().sequence;\n> >> metadata.timestamp = input->metadata().timestamp;\n> >>\n> >> MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> >> MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n> >> if (!in.isValid() || !out.isValid()) {\n> >> LOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n> >> metadata.status = FrameMetadata::FrameError;\n> >> return;\n> >> }\n> >>\n> >> stats_->startFrame();\n> >>\n> >> if (inputConfig_.patternSize.height == 2)\n> >> process2(in.planes()[0].data(), out.planes()[0].data());\n> >> else\n> >> process4(in.planes()[0].data(), out.planes()[0].data());\n> >>\n> >> metadata.planes()[0].bytesused = out.planes()[0].size();\n> >>\n> >> dmaSyncers.clear();\n> >>\n> >> Any idea what could be wrong?\n> >>\n> >> > Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n> >> > Signed-off-by: Harvey Yang \n> >> > Reviewed-by: Kieran Bingham \n> >> > Reviewed-by: Laurent Pinchart \n> >> > ---\n> >> > include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n> >> > src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n> >> > 2 files changed, 17 insertions(+)\n> >> >\n> >> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> >> > index fc5de2c13edd..d26f8a74f4c6 100644\n> >> > --- a/include/libcamera/internal/dma_buf_allocator.h\n> >> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n> >> > @@ -60,9 +60,14 @@ public:\n> >> >\n> >> > explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n> >> >\n> >> > + DmaSyncer(DmaSyncer &&other) = default;\n> >> > + DmaSyncer &operator=(DmaSyncer &&other) = default;\n> >> > +\n> >> > ~DmaSyncer();\n> >> >\n> >> > private:\n> >> > + LIBCAMERA_DISABLE_COPY(DmaSyncer)\n> >> > +\n> >> > void sync(uint64_t step);\n> >> >\n> >> > SharedFD fd_;\n> >> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> >> > index 3cc52f9686b0..a014c3b4263c 100644\n> >> > --- a/src/libcamera/dma_buf_allocator.cpp\n> >> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> >> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n> >> > sync(DMA_BUF_SYNC_START);\n> >> > }\n> >> >\n> >> > +/**\n> >> > + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n> >> > + * \\param[in] other The other instance\n> >> > + * \\brief Enable move on class DmaSyncer\n> >> > + */\n> >> > +\n> >> > +/**\n> >> > + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n> >> > + * \\param[in] other The other instance\n> >> > + * \\brief Enable move on class DmaSyncer\n> >> > + */\n> >> > +\n> >> > DmaSyncer::~DmaSyncer()\n> >> > {\n> >> > sync(DMA_BUF_SYNC_END);\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 17721C32F6\n\tfor ;\n\tMon, 13 Jan 2025 16:26:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AF55684E7;\n\tMon, 13 Jan 2025 17:26:41 +0100 (CET)","from mail-10629.protonmail.ch (mail-10629.protonmail.ch\n\t[79.135.106.29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B72D5684E7\n\tfor ;\n\tMon, 13 Jan 2025 17:26:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"E+pScHg5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736785597; x=1737044797;\n\tbh=vySOWzbbd0VfzySArdx1Yf1yB58uIGlRM64EuQ8T7uE=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=E+pScHg5vb54kN99Pf8FttyFj4xPPKIGYVvQkfqbv6vgahzKZ+1SkDkNDVgGKZJwd\n\taAV0nhPHVv9z4qt0D1PSaVm2e1Sv/HGTOQKA28v9D1FKxC1WD1/o6rP1MpAbRv8Hpf\n\tcnlWwa2gDFZdtpalAJFPIpA11AAWL6bmgx4iw5PGXBqOHQPJozBkQEWUv6NxG6UQRN\n\tcNmJr4EWyjz1LoN1Xcz1TKq6cTJLDat8+PdfqFdJ1Tlj9WKAvb3mZLo0AyFbL7tnSU\n\tG11yp3196mip2nyBYOsnxmzkrXSVf/6KGFovImnBckvTDVBvsA5nRzpy6X2DFImSpz\n\tkgdj/Y6Z9+Paw==","Date":"Mon, 13 Jan 2025 16:26:31 +0000","To":"Milan Zamazal ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Cheng-Hao Yang ,\n\tlibcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tLaurent Pinchart ","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","Message-ID":"","In-Reply-To":"<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>\n\t<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t\n\t<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"e2507f9f5dd578338520843aa44a83b44b822ac9","MIME-Version":"1.0","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":33052,"web_url":"https://patchwork.libcamera.org/comment/33052/","msgid":"","date":"2025-01-13T16:43:27","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Barnabás,\n\nOn Mon, Jan 13, 2025 at 5:26 PM Barnabás Pőcze wrote:\n>\n> Hi\n>\n>\n> 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal írta:\n>\n> > Hi Harvey,\n> >\n> > Cheng-Hao Yang writes:\n> >\n> > > Hi Milan,\n> > >\n> > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal wrote:\n> > >>\n> > >> Hi,\n> > >>\n> > >> Harvey Yang writes:\n> > >>\n> > >> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n> > >> > instance would trigger sync end earlier than expected. This patch makes\n> > >> > it non-copyable to avoid the issue.\n> > >>\n> > >> After this patch, software ISP still works but reports the following\n> > >> error on each frame:\n> > >\n> > > May I know what the environment is? (Unit tests or specific tests\n> > > you're running?\n> >\n> > No tests, running `cam' with software ISP.\n> >\n> > >> ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor,\n> > >> flags: 5\n> > >\n> > > Based on the log, `fd_.get()` is -1. Could you confirm that\n> > > `plane.fd.get()` below is never `-1`?\n> >\n> > Yes, they are never -1.\n> >\n> > The problem occurs in the following line from the snippet below:\n> >\n> > dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n> >\n> > It processes the input plane fine and calls DmaSyncer::sync with the\n> > right fd, then it process the output plane and it first calls\n> > DmaSyncer::sync with the right fd, followed by a call with fd -1.\n> > Here is the backtrace of the corresponding call (note the\n> > line numbers are a bit off due to debugging prints I inserted):\n> >\n> > #0 0x0000fffff7f0810c in libcamera::DmaSyncer::sync (this=this@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341\n> > #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@entry=0xffffe4000bf0, __in_chrg=) at ../src/libcamera/dma_buf_allocator.cpp:329\n> > #2 0x0000fffff7f68fe4 in std::_Destroy (__pointer=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:151\n> > #3 std::_Destroy_aux::__destroy (__last=, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:163\n> > #4 std::_Destroy (__last=, __first=) at /usr/include/c++/11/bits/stl_construct.h:196\n> > #5 std::_Destroy (__last=0xffffe4000c08, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/alloc_traits.h:848\n> > #6 std::vector >::_M_realloc_insert (this=0xffffefe0d738, __position=...,\n> > __position@entry=...) at /usr/include/c++/11/bits/vector.tcc:498\n> > #7 0x0000fffff7f691b4 in std::vector >::emplace_back (this=this@entry=0xffffefe0d738)\n> > at /usr/include/c++/11/bits/vector.tcc:121\n> > #8 0x0000fffff7f66d38 in libcamera::DebayerCpu::process (this=0xfffff001e8d0, frame=0, input=0xfffff0020270, output=0xfffff0018f70, params=...) at ../src/libcamera/software_isp/debayer_cpu.cpp:752\n> >\n> > This means DmaSyncer::sync is called twice within emplace_back, the\n> > second time from the destructor with the invalid fd. This is weird,\n> > what's the destroyed instance and why does it happen only with the\n> > output plane and not the input plane? dmaSyncers is a local variable\n> > not used anywhere else, I suspect the vector gets resized and some\n> > unwanted actions on the instances happen during the operation.\n>\n> When the vector is resized, it must destroy the objects in the old storage.\n> See the call to `_M_realloc_insert()` in the stack trace. Hence the call to the\n> destructor during `emplace_back()`.\n>\n> And the reason why this did not happen previously is that previously the vector\n> selected the copy constructor for moving the objects between allocation because\n> the move constructor is not `noexcept` (because `SharedFD`'s corresponding methods\n> are not `noexcept` - this should probably be fixed in any case).\n>\n> But since this change it must choose the move constructor because the copy\n> constructor is deleted. And the move constructor leaves `SharedFd::fd_ == nullptr`,\n> so `SharedFD::get()` returns -1.\n>\n> This also means that previously the syncs were probably off due to the destructor calls.\n\nThanks! That perfectly explains the error log.\nIn `DmaSyncer::sync`, it should confirm `fd_.isValid() == true` before\nexecuting the sync.\n\nMilan, could you try\nhttps://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/7a376b4b2f1fa0dd06a9cea268df5dbb12d357e9?\nI'll upload another patch after the pipeline tests are passed.\n\nThanks,\nHarvey\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > > BR,\n> > > Harvey\n> > >\n> > >>\n> > >> DmaSyncer is used only in debayer_cpu.cpp:\n> > >>\n> > >> std::vector dmaSyncers;\n> > >> for (const FrameBuffer::Plane &plane : input->planes())\n> > >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n> > >>\n> > >> for (const FrameBuffer::Plane &plane : output->planes())\n> > >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n> > >>\n> > >> green_ = params.green;\n> > >> red_ = swapRedBlueGains_ ? params.blue : params.red;\n> > >> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n> > >>\n> > >> /* Copy metadata from the input buffer */\n> > >> FrameMetadata &metadata = output->_d()->metadata();\n> > >> metadata.status = input->metadata().status;\n> > >> metadata.sequence = input->metadata().sequence;\n> > >> metadata.timestamp = input->metadata().timestamp;\n> > >>\n> > >> MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n> > >> MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n> > >> if (!in.isValid() || !out.isValid()) {\n> > >> LOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n> > >> metadata.status = FrameMetadata::FrameError;\n> > >> return;\n> > >> }\n> > >>\n> > >> stats_->startFrame();\n> > >>\n> > >> if (inputConfig_.patternSize.height == 2)\n> > >> process2(in.planes()[0].data(), out.planes()[0].data());\n> > >> else\n> > >> process4(in.planes()[0].data(), out.planes()[0].data());\n> > >>\n> > >> metadata.planes()[0].bytesused = out.planes()[0].size();\n> > >>\n> > >> dmaSyncers.clear();\n> > >>\n> > >> Any idea what could be wrong?\n> > >>\n> > >> > Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n> > >> > Signed-off-by: Harvey Yang \n> > >> > Reviewed-by: Kieran Bingham \n> > >> > Reviewed-by: Laurent Pinchart \n> > >> > ---\n> > >> > include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n> > >> > src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n> > >> > 2 files changed, 17 insertions(+)\n> > >> >\n> > >> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> > >> > index fc5de2c13edd..d26f8a74f4c6 100644\n> > >> > --- a/include/libcamera/internal/dma_buf_allocator.h\n> > >> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n> > >> > @@ -60,9 +60,14 @@ public:\n> > >> >\n> > >> > explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n> > >> >\n> > >> > + DmaSyncer(DmaSyncer &&other) = default;\n> > >> > + DmaSyncer &operator=(DmaSyncer &&other) = default;\n> > >> > +\n> > >> > ~DmaSyncer();\n> > >> >\n> > >> > private:\n> > >> > + LIBCAMERA_DISABLE_COPY(DmaSyncer)\n> > >> > +\n> > >> > void sync(uint64_t step);\n> > >> >\n> > >> > SharedFD fd_;\n> > >> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> > >> > index 3cc52f9686b0..a014c3b4263c 100644\n> > >> > --- a/src/libcamera/dma_buf_allocator.cpp\n> > >> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> > >> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n> > >> > sync(DMA_BUF_SYNC_START);\n> > >> > }\n> > >> >\n> > >> > +/**\n> > >> > + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n> > >> > + * \\param[in] other The other instance\n> > >> > + * \\brief Enable move on class DmaSyncer\n> > >> > + */\n> > >> > +\n> > >> > +/**\n> > >> > + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n> > >> > + * \\param[in] other The other instance\n> > >> > + * \\brief Enable move on class DmaSyncer\n> > >> > + */\n> > >> > +\n> > >> > DmaSyncer::~DmaSyncer()\n> > >> > {\n> > >> > sync(DMA_BUF_SYNC_END);\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 00D14C326C\n\tfor ;\n\tMon, 13 Jan 2025 16:43:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F33AF68503;\n\tMon, 13 Jan 2025 17:43:41 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 948C1684E7\n\tfor ;\n\tMon, 13 Jan 2025 17:43:39 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id\n\t38308e7fff4ca-30225b2586cso50684451fa.0\n\tfor ;\n\tMon, 13 Jan 2025 08:43:39 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Q/ix1mif\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1736786619; x=1737391419;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=haG9PhmZ3zEijZ9ok0u2V+c1FBdfHTx0TqlOtWDpyaE=;\n\tb=Q/ix1mifijlSgfMfcYBDsXZOJ6rUW7SbBH/GtEnU1wztnSsGbW/Nf5mLQQDD/N+l6I\n\tfDAmydq1Mn4Q1tvzUPyn6zfA6wXsB9bzNN7PivX6J/fWS5oKC1fws3gL+lGqDvB3C5i+\n\t48FqPecI5i9iXbQ2iIynRIW0Dolf5ZpYIdAu8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736786619; x=1737391419;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=haG9PhmZ3zEijZ9ok0u2V+c1FBdfHTx0TqlOtWDpyaE=;\n\tb=fJkEdE0Uff7O+x5Fwk1F1TAwhFE4mLqtFLe5TE/dhrWSEhOVe91eWvN2XrY/MbplYE\n\tg2RPxsOmgdEFGXJNab6TJTxiWMEWoaO4QnNHpcMecS4SZ/inojPtdOD2Sou3sBAijXfe\n\t+FHNr7ccKSsUZrT7AdxWxNoAqqL9Zl60AHKt+GX0rGDDdJkmhkkeLStOrA+nj2n8FY7x\n\thCDPiNk3RPyCJeZSGnIDcx5wTnW0gmnL/MAoS4sLTB/rl9qms3Ng4RmmVOAg6nu6mem0\n\tLwiL07o++yFF62SFa1UcsVrAq83UKfRyQPzVgZ4boOErYyf8rDQoFjvVwxyiLRjwOx8V\n\tpVZw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW+1eX+/nMhnvGSHrzQ3kzlOP+fqVX9cH9kxHxlk7Y//2KofYV13b78aWkb9IJHbNB89/xPm7ykSZ54cur0uig=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzlR1xGhhXkJsLULUVn5Gc2VcVoRaKf6d/rz2KqtnZWwBH2kWVr\n\t884lnoN0RLKu1UIyA0FX927oTTMP+gpDQzgpl88JyefYOp4jrrL1OvQfnPr8SloHgfaAqynbmRv\n\tBkDaNt45peWo2VUKKrPcujMTQIeVgk+Dw7oxA","X-Gm-Gg":"ASbGncubH4iW3RlIdwt8+GBl9GvS0cP7YFHgDgXJEPYN1kyuF980+M2skIVp9t6tUYr\n\tPIxGE4OK6RYAP4d+dcIWq82WaS+dIIF8oVSDg","X-Google-Smtp-Source":"AGHT+IEhv4HuZXa8/i/cVWxsUjhfeL5DxtwH/0D9NgkXLSXZ75wJicKVQwsdnXCG1zQr0PnPqiFSTaso59CcdpSZ4gA=","X-Received":"by 2002:a05:651c:2227:b0:2ff:c167:a965 with SMTP id\n\t38308e7fff4ca-305fcfbc0e7mr54191731fa.8.1736786618663;\n\tMon, 13 Jan 2025 08:43:38 -0800 (PST)","MIME-Version":"1.0","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>\n\t<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t\n\t<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Mon, 13 Jan 2025 17:43:27 +0100","X-Gm-Features":"AbW1kvaAHFZNZ0Engj93ZGu7eaJ367dTXMFdUypkgDE9-_ddNK8VnLpP6ev3lU8","Message-ID":"","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org,\n\tKieran Bingham , \n\tLaurent Pinchart ","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":33053,"web_url":"https://patchwork.libcamera.org/comment/33053/","msgid":"<857c6ywt27.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-13T16:57:04","subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Cheng-Hao Yang writes:\n\n> Hi Barnabás,\n>\n> On Mon, Jan 13, 2025 at 5:26 PM Barnabás Pőcze wrote:\n>>\n>> Hi\n>>\n>>\n>> 2025. január 13., hétfő 17:16 keltezéssel, Milan Zamazal írta:\n>>\n>> > Hi Harvey,\n>> >\n>> > Cheng-Hao Yang writes:\n>> >\n>> > > Hi Milan,\n>> > >\n>> > > On Mon, Jan 13, 2025 at 3:44 PM Milan Zamazal wrote:\n>> > >>\n>> > >> Hi,\n>> > >>\n>> > >> Harvey Yang writes:\n>> > >>\n>> > >> > As DmaSyncer does sync start/end in the c'tor/d'tor, copying a DmaSyncer\n>> > >> > instance would trigger sync end earlier than expected. This patch makes\n>> > >> > it non-copyable to avoid the issue.\n>> > >>\n>> > >> After this patch, software ISP still works but reports the following\n>> > >> error on each frame:\n>> > >\n>> > > May I know what the environment is? (Unit tests or specific tests\n>> > > you're running?\n>> >\n>> > No tests, running `cam' with software ISP.\n>> >\n>> > >> ERROR DmaBufAllocator dma_buf_allocator.cpp:344 Unable to sync dma fd: -1, err: Bad file descriptor,\n>> > >> flags: 5\n>> > >\n>> > > Based on the log, `fd_.get()` is -1. Could you confirm that\n>> > > `plane.fd.get()` below is never `-1`?\n>> >\n>> > Yes, they are never -1.\n>> >\n>> > The problem occurs in the following line from the snippet below:\n>> >\n>> > dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n>> >\n>> > It processes the input plane fine and calls DmaSyncer::sync with the\n>> > right fd, then it process the output plane and it first calls\n>> > DmaSyncer::sync with the right fd, followed by a call with fd -1.\n>> > Here is the backtrace of the corresponding call (note the\n>> > line numbers are a bit off due to debugging prints I inserted):\n>> >\n>> > #0 0x0000fffff7f0810c in libcamera::DmaSyncer::sync (this=this@entry=0xffffe4000bf0, step=step@entry=4) at ../src/libcamera/dma_buf_allocator.cpp:341\n>> > #1 0x0000fffff7f08228 in libcamera::DmaSyncer::~DmaSyncer (this=this@entry=0xffffe4000bf0, __in_chrg=) at ../src/libcamera/dma_buf_allocator.cpp:329\n>> > #2 0x0000fffff7f68fe4 in std::_Destroy (__pointer=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:151\n>> > #3 std::_Destroy_aux::__destroy (__last=, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/stl_construct.h:163\n>> > #4 std::_Destroy (__last=, __first=) at /usr/include/c++/11/bits/stl_construct.h:196\n>> > #5 std::_Destroy (__last=0xffffe4000c08, __first=0xffffe4000bf0) at /usr/include/c++/11/bits/alloc_traits.h:848\n>> > #6 std::vector >::_M_realloc_insert (this=0xffffefe0d738, __position=...,\n>> > __position@entry=...) at /usr/include/c++/11/bits/vector.tcc:498\n>> > #7 0x0000fffff7f691b4 in std::vector >::emplace_back (this=this@entry=0xffffefe0d738)\n>> > at /usr/include/c++/11/bits/vector.tcc:121\n>> > #8 0x0000fffff7f66d38 in libcamera::DebayerCpu::process (this=0xfffff001e8d0, frame=0, input=0xfffff0020270, output=0xfffff0018f70, params=...) at ../src/libcamera/software_isp/debayer_cpu.cpp:752\n>> >\n>> > This means DmaSyncer::sync is called twice within emplace_back, the\n>> > second time from the destructor with the invalid fd. This is weird,\n>> > what's the destroyed instance and why does it happen only with the\n>> > output plane and not the input plane? dmaSyncers is a local variable\n>> > not used anywhere else, I suspect the vector gets resized and some\n>> > unwanted actions on the instances happen during the operation.\n>>\n>> When the vector is resized, it must destroy the objects in the old storage.\n>> See the call to `_M_realloc_insert()` in the stack trace. Hence the call to the\n>> destructor during `emplace_back()`.\n>>\n>> And the reason why this did not happen previously is that previously the vector\n>> selected the copy constructor for moving the objects between allocation because\n>> the move constructor is not `noexcept` (because `SharedFD`'s corresponding methods\n>> are not `noexcept` - this should probably be fixed in any case).\n>>\n>> But since this change it must choose the move constructor because the copy\n>> constructor is deleted. And the move constructor leaves `SharedFd::fd_ == nullptr`,\n>> so `SharedFD::get()` returns -1.\n>>\n>> This also means that previously the syncs were probably off due to the destructor calls.\n>\n> Thanks! That perfectly explains the error log.\n> In `DmaSyncer::sync`, it should confirm `fd_.isValid() == true` before\n> executing the sync.\n>\n> Milan, could you try\n> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/7a376b4b2f1fa0dd06a9cea268df5dbb12d357e9?\n> I'll upload another patch after the pipeline tests are passed.\n\nYes, this works fine. Thank you both for help.\n\n> Thanks,\n> Harvey\n>\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>> >\n>> > > BR,\n>> > > Harvey\n>> > >\n>> > >>\n>> > >> DmaSyncer is used only in debayer_cpu.cpp:\n>> > >>\n>> > >> std::vector dmaSyncers;\n>> > >> for (const FrameBuffer::Plane &plane : input->planes())\n>> > >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Read);\n>> > >>\n>> > >> for (const FrameBuffer::Plane &plane : output->planes())\n>> > >> dmaSyncers.emplace_back(plane.fd, DmaSyncer::SyncType::Write);\n>> > >>\n>> > >> green_ = params.green;\n>> > >> red_ = swapRedBlueGains_ ? params.blue : params.red;\n>> > >> blue_ = swapRedBlueGains_ ? params.red : params.blue;\n>> > >>\n>> > >> /* Copy metadata from the input buffer */\n>> > >> FrameMetadata &metadata = output->_d()->metadata();\n>> > >> metadata.status = input->metadata().status;\n>> > >> metadata.sequence = input->metadata().sequence;\n>> > >> metadata.timestamp = input->metadata().timestamp;\n>> > >>\n>> > >> MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);\n>> > >> MappedFrameBuffer out(output, MappedFrameBuffer::MapFlag::Write);\n>> > >> if (!in.isValid() || !out.isValid()) {\n>> > >> LOG(Debayer, Error) << \"mmap-ing buffer(s) failed\";\n>> > >> metadata.status = FrameMetadata::FrameError;\n>> > >> return;\n>> > >> }\n>> > >>\n>> > >> stats_->startFrame();\n>> > >>\n>> > >> if (inputConfig_.patternSize.height == 2)\n>> > >> process2(in.planes()[0].data(), out.planes()[0].data());\n>> > >> else\n>> > >> process4(in.planes()[0].data(), out.planes()[0].data());\n>> > >>\n>> > >> metadata.planes()[0].bytesused = out.planes()[0].size();\n>> > >>\n>> > >> dmaSyncers.clear();\n>> > >>\n>> > >> Any idea what could be wrong?\n>> > >>\n>> > >> > Fixes: 39482d59fe71 (\"DmaBufAllocator: Add Dma Buffer synchronization function & helper class\")\n>> > >> > Signed-off-by: Harvey Yang \n>> > >> > Reviewed-by: Kieran Bingham \n>> > >> > Reviewed-by: Laurent Pinchart \n>> > >> > ---\n>> > >> > include/libcamera/internal/dma_buf_allocator.h | 5 +++++\n>> > >> > src/libcamera/dma_buf_allocator.cpp | 12 ++++++++++++\n>> > >> > 2 files changed, 17 insertions(+)\n>> > >> >\n>> > >> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n>> > >> > index fc5de2c13edd..d26f8a74f4c6 100644\n>> > >> > --- a/include/libcamera/internal/dma_buf_allocator.h\n>> > >> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n>> > >> > @@ -60,9 +60,14 @@ public:\n>> > >> >\n>> > >> > explicit DmaSyncer(SharedFD fd, SyncType type = SyncType::ReadWrite);\n>> > >> >\n>> > >> > + DmaSyncer(DmaSyncer &&other) = default;\n>> > >> > + DmaSyncer &operator=(DmaSyncer &&other) = default;\n>> > >> > +\n>> > >> > ~DmaSyncer();\n>> > >> >\n>> > >> > private:\n>> > >> > + LIBCAMERA_DISABLE_COPY(DmaSyncer)\n>> > >> > +\n>> > >> > void sync(uint64_t step);\n>> > >> >\n>> > >> > SharedFD fd_;\n>> > >> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n>> > >> > index 3cc52f9686b0..a014c3b4263c 100644\n>> > >> > --- a/src/libcamera/dma_buf_allocator.cpp\n>> > >> > +++ b/src/libcamera/dma_buf_allocator.cpp\n>> > >> > @@ -311,6 +311,18 @@ DmaSyncer::DmaSyncer(SharedFD fd, SyncType type)\n>> > >> > sync(DMA_BUF_SYNC_START);\n>> > >> > }\n>> > >> >\n>> > >> > +/**\n>> > >> > + * \\fn DmaSyncer::DmaSyncer(DmaSyncer &&other);\n>> > >> > + * \\param[in] other The other instance\n>> > >> > + * \\brief Enable move on class DmaSyncer\n>> > >> > + */\n>> > >> > +\n>> > >> > +/**\n>> > >> > + * \\fn DmaSyncer::operator=(DmaSyncer &&other);\n>> > >> > + * \\param[in] other The other instance\n>> > >> > + * \\brief Enable move on class DmaSyncer\n>> > >> > + */\n>> > >> > +\n>> > >> > DmaSyncer::~DmaSyncer()\n>> > >> > {\n>> > >> > sync(DMA_BUF_SYNC_END);\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 88D30C32F6\n\tfor ;\n\tMon, 13 Jan 2025 16:57:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D8076851C;\n\tMon, 13 Jan 2025 17:57:12 +0100 (CET)","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 56B04684E7\n\tfor ;\n\tMon, 13 Jan 2025 17:57:10 +0100 (CET)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-554-kGhFIkEsMoa89tgZbG-aWA-1; Mon, 13 Jan 2025 11:57:08 -0500","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-385ded5e92aso1636990f8f.3\n\tfor ;\n\tMon, 13 Jan 2025 08:57:07 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-436e9e6249csm147382845e9.38.2025.01.13.08.57.04\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Jan 2025 08:57:04 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"EI9FYZJD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1736787429;\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=22KuAdOn5RoVkcTedvudwg5TVmbuAYbf3l9S7phLmc8=;\n\tb=EI9FYZJDrjvXtCxGWlmJOfVLq2QfEs904ehIIQ3NnC8qlP5qjzfzgFLm9uwFLX6PY2MmXV\n\t/bXP1FFUZV0y9iD4HN6Bvd0woW7NZF6qgyl8JB02vkVzqjgN2nQDexf9nEesAR0E4MxWL4\n\t6C7dRkTAPX6DIbezOfIm/18WCsySUnY=","X-MC-Unique":"kGhFIkEsMoa89tgZbG-aWA-1","X-Mimecast-MFC-AGG-ID":"kGhFIkEsMoa89tgZbG-aWA","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736787427; x=1737392227;\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=Py5BsuLpJhtl7X9fMReONS/1iAPwQ7E2rEuBGVzscaM=;\n\tb=CfFuG9ZNwJeQUk1MQ/J8PlXrnE7ZfvOYVoKNvx15tOOW4x83ErC/6LxXf7UgS/lABS\n\tTckeibksmmQaZIXF9H7+RpADf2K+FH6qaqvbOj5tv87KN4yQ+nqMeaJs+BvuIgXbFq4j\n\thsyLo80oYc0QyIEZI0rxE9MvyiMwqzBunUxpu7eA/yxLazHk5y7Jey+nTiGSiXHMZDDO\n\t370kXC/tM4pEl41Br9JJXAK7aEyS3Jo6PH6e3m7rJwBeymp8e9W0zmX/F1Uo76GFdR2i\n\tYyLYkv6cH+lIne2MyTniNW3qB3q75y5J1ohaSRSOQZ9pg6T8CX0U1iZagmjjed5qh+ou\n\tcz6w==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWxaUhwXpnYDfl9oAgO2/24mY/jBz/Mzq9qmn6X6jw+5JHY9Q5M9cclMJiNaUTYoDX1IGqM4S/5yj3HwZesAmI=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yysfc8HtMY+M9lbJSulHAVJLpWV7o5Tb8Jc29V1+ggDWgIb3jeQ\n\t+45JJDd/DxNv/Ilx9Goz4czs3YGUt4qvs4M2Y0Zwx6Z8Pu8WznUj7XZr5BjfuKKLXHSTtmtcqs5\n\tKdGaR7CM4IxBiKLQU/l8cS+l4fROhkxuovkijZdEA/9EC4iOsjvs61O+bsuJethFgxw/fwYQ=","X-Gm-Gg":"ASbGncs0kRnONATOc4Dlyb1grXsUImTwvAf7YMmY7BhifmSjGGxZKTF4eNZjpmvuTAd\n\tCpRHIrnNR3Suwki7pb7PWwQs3dR+GbTEcmBl8xtaxO4L+uOdhVkNw0IS15cA8F3BTyXsFcZhijX\n\thtonOg18BbAilKCfsCn+jzFB0TRFu/byGPhdOvzZLR64kv7tgUx5TlT8MB1KRVbgGoOSM5+2Nzu\n\tFyMqWZuum+9hsrXkiudJB+bRe6Ea8WqGfrLq2uCUGyuVM93O4n/s6Jj38ePwqX5Tlhm87JzmjAc\n\tEGOHrYa3KF9SgDGy+dn8a2OHpKPHYAxjBA==","X-Received":["by 2002:adf:b509:0:b0:38a:6264:d13e with SMTP id\n\tffacd0b85a97d-38a872d2b99mr13740415f8f.8.1736787426554; \n\tMon, 13 Jan 2025 08:57:06 -0800 (PST)","by 2002:adf:b509:0:b0:38a:6264:d13e with SMTP id\n\tffacd0b85a97d-38a872d2b99mr13740399f8f.8.1736787426082; \n\tMon, 13 Jan 2025 08:57:06 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHRo7qSVG4wAeY5DqzblydeYaY1dXe1pZy9fPhPd0YfXcSydb43DkDqo2YejBuSQHpgLUyk9w==","From":"Milan Zamazal ","To":"Cheng-Hao Yang ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v3] DmaBufAllocator: Make DmaSyncer non-copyable","In-Reply-To":"\n\t(Cheng-Hao Yang's message of \"Mon, 13 Jan 2025 17:43:27 +0100\")","References":"<20241211084433.3039442-1-chenghaoyang@chromium.org>\n\t<85h662wz7v.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t\n\t<85bjwawuxd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t\n\t","Date":"Mon, 13 Jan 2025 17:57:04 +0100","Message-ID":"<857c6ywt27.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"XOMqWCHarVwTTxfy4rFidhJAioO3Z9NgMT3uwY5Rc8M_1736787427","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\" "}}]