[{"id":32146,"web_url":"https://patchwork.libcamera.org/comment/32146/","msgid":"<87zfm3mg6v.fsf@redhat.com>","date":"2024-11-13T13:11:36","subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Harvey,\n\nthank you for the update, it's fine from my side.\n\nHarvey Yang writes:\n\n> To synchronize CPU access with mmap and hardware access on DMA buffers,\n> using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and\n> a helper class to allow users to sync buffers more easily.\n>\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n\nReviewed-by: Milan Zamazal \n\n> ---\n> .../libcamera/internal/dma_buf_allocator.h | 33 +++++++\n> src/libcamera/dma_buf_allocator.cpp | 90 +++++++++++++++++++\n> 2 files changed, 123 insertions(+)\n>\n> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> index d2a0a0d19..2b9b99678 100644\n> --- a/include/libcamera/internal/dma_buf_allocator.h\n> +++ b/include/libcamera/internal/dma_buf_allocator.h\n> @@ -23,6 +23,19 @@ public:\n> \n> \tusing DmaBufAllocatorFlags = Flags;\n> \n> +\tenum class SyncStep {\n> +\t\tStart = 0,\n> +\t\tEnd\n> +\t};\n> +\n> +\tenum class SyncType {\n> +\t\tRead = 0,\n> +\t\tWrite,\n> +\t\tReadWrite,\n> +\t};\n> +\n> +\tstatic void sync(int fd, SyncStep step, SyncType type);\n> +\n> \tDmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap);\n> \t~DmaBufAllocator();\n> \tbool isValid() const { return providerHandle_.isValid(); }\n> @@ -35,6 +48,26 @@ private:\n> \tDmaBufAllocatorFlag type_;\n> };\n> \n> +class DmaSyncer final\n> +{\n> +public:\n> +\texplicit DmaSyncer(int fd,\n> +\t\t\t DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> +\t\t: fd_(fd), type_(type)\n> +\t{\n> +\t\tDmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_);\n> +\t}\n> +\n> +\t~DmaSyncer()\n> +\t{\n> +\t\tDmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_);\n> +\t}\n> +\n> +private:\n> +\tint fd_;\n> +\tDmaBufAllocator::SyncType type_;\n> +};\n> +\n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag)\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index be6efb89f..f0f068cc1 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -78,6 +78,81 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator)\n> * \\brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values\n> */\n> \n> +/**\n> + * \\enum DmaBufAllocator::SyncStep\n> + * \\brief Either start or end of the synchronization\n> + * \\var DmaBufAllocator::Start\n> + * \\brief Indicates the start of a map access session\n> + * \\var DmaBufAllocator::End\n> + * \\brief Indicates the end of a map access session\n> + */\n> +\n> +/**\n> + * \\enum DmaBufAllocator::SyncType\n> + * \\brief Read and/or write access via the CPU map\n> + * \\var DmaBufAllocator::Read\n> + * \\brief Indicates that the mapped dma-buf will be read by the client via the\n> + * CPU map\n> + * \\var DmaBufAllocator::Write\n> + * \\brief Indicates that the mapped dm-buf will be written by the client via the\n> + * CPU map\n> + * \\var DmaBufAllocator::ReadWrite\n> + * \\brief Indicates that the mapped dma-buf will be read and written by the\n> + * client via the CPU map\n> + */\n> +\n> +/**\n> + * \\brief Synchronize CPU access and hardware access\n> + * \\param[in] fd The dma-buf's file descriptor to be synchronized\n> + * \\param[in] step Either start or end of the synchronization\n> + * \\param[in] type Read and/or write access during CPU mmap\n> + *\n> + * The client is expected to call this function with\n> + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the\n> + * start and end of buffer access respectively.\n> + */\n> +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type)\n> +{\n> +\tuint64_t flags = 0;\n> +\tswitch (step) {\n> +\tcase DmaBufAllocator::SyncStep::Start:\n> +\t\tflags = DMA_BUF_SYNC_START;\n> +\t\tbreak;\n> +\tcase DmaBufAllocator::SyncStep::End:\n> +\t\tflags = DMA_BUF_SYNC_END;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tswitch (type) {\n> +\tcase DmaBufAllocator::SyncType::Read:\n> +\t\tflags = flags | DMA_BUF_SYNC_READ;\n> +\t\tbreak;\n> +\tcase DmaBufAllocator::SyncType::Write:\n> +\t\tflags = flags | DMA_BUF_SYNC_WRITE;\n> +\t\tbreak;\n> +\tcase DmaBufAllocator::SyncType::ReadWrite:\n> +\t\tflags = flags | DMA_BUF_SYNC_RW;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tstruct dma_buf_sync sync = {\n> +\t\t.flags = flags\n> +\t};\n> +\n> +\tint ret;\n> +\tdo {\n> +\t\tret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> +\t} while (ret && (errno == EINTR || errno == EAGAIN));\n> +\n> +\tif (ret) {\n> +\t\tret = errno;\n> +\t\tLOG(DmaBufAllocator, Error) << \"Unable to sync dma fd: \" << fd\n> +\t\t\t\t\t << \", err: \" << strerror(ret)\n> +\t\t\t\t\t << \", step: \" << static_cast(step)\n> +\t\t\t\t\t << \", type: \" << static_cast(type);\n> +\t}\n> +}\n> +\n> /**\n> * \\brief Construct a DmaBufAllocator of a given type\n> * \\param[in] type The type(s) of the dma-buf providers to allocate from\n> @@ -205,4 +280,19 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)\n> \t\treturn allocFromHeap(name, size);\n> }\n> \n> +/**\n> + * \\class DmaSyncer\n> + * \\brief Helper class for dma-buf's synchronization\n> + *\n> + * This class wraps a userspace dma-buf's synchronization process with an\n> + * object's lifetime.\n> + */\n> +\n> +/**\n> + * \\fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> + * \\brief Construct a DmaSyncer with a dma-buf's fd and the access type\n> + * \\param[in] fd The dma-buf's file descriptor to synchronize\n> + * \\param[in] type Read and/or write access via the CPU map\n> + */\n> +\n> } /* namespace libcamera */","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 41223BE173\n\tfor ;\n\tWed, 13 Nov 2024 13:11:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 291E465816;\n\tWed, 13 Nov 2024 14:11:44 +0100 (CET)","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 7BC9C657CF\n\tfor ;\n\tWed, 13 Nov 2024 14:11:42 +0100 (CET)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-527-GuajSjr4N7ScCY-2mjuAjg-1; Wed, 13 Nov 2024 08:11:40 -0500","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-4314f1e0f2bso49017175e9.1\n\tfor ;\n\tWed, 13 Nov 2024 05:11:40 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-381ed99a0efsm18386769f8f.58.2024.11.13.05.11.37\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 13 Nov 2024 05:11:37 -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=\"i+ekm3OL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1731503501;\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=xb/OMUv2A89x7wZbpXI+IvCsVAh058LMOee3XuBHEQE=;\n\tb=i+ekm3OLngR2kDfbOan+FM/jaNgI5NdFSsURyl+q9e+xfB+saqGwQFpbHdhZ43LHzcuF4K\n\t893jHrgMrZQE0kZxTZ0b6D5kDTczr3O4b9dYxbqUJJikpDMWFrsxcD9++gc4ZcBlHu+/Pa\n\tPkW4XbfOaPEy14SlTzDDCtiS8wC9vGs=","X-MC-Unique":"GuajSjr4N7ScCY-2mjuAjg-1","X-Mimecast-MFC-AGG-ID":"GuajSjr4N7ScCY-2mjuAjg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731503499; x=1732108299;\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=xb/OMUv2A89x7wZbpXI+IvCsVAh058LMOee3XuBHEQE=;\n\tb=rQTEo1MJC5dQQkOceG0nfFBtmL4yJQc9uuguDUTVvqHJB+wxw2OG0Ira5TzG4K6phq\n\t9AE3+fIpjE0MlBQbWIBTg6R0vi9aAR2ZDjR5nf7nva/OVA8nLcGR5vdaxckcA6rUGpPd\n\tUXU9Ezy8z0tt1rF4WYXIj9stohLXbOAVxFPoJl+3UvgXy32HYU3Lxa4Qzw6PmE+HC09C\n\tH014ov8zjbWpkYv+kb4RlOVgmsLKfvB9t74IOGcpVnesgAp7xxdMAYprhXU5XEpUkgeL\n\tGX6G33w0tsaA0FE+hQwf1m5ANTjk9X12Y9YlpnYqDM+piWBU/BmApUk7tuxSg0h8O4ho\n\tUquQ==","X-Gm-Message-State":"AOJu0Yy1tzZxDaLgTW93fRADImVUuI1F5EUC2hOrJUC74sVZ0HukFq2M\n\t6OBTKMI/FmGfy5ZbqMZrzLe0L16WRbtqj9ju+v9eM7ZJhmvBeBLsMcMjbNGHvhQzKfAAfrHAg0D\n\thwfuw8w92BXxiD/Nc9RePnwHPMYkX/rUfZtn1529vxhRgOnq+GqJ+VyLTziPdmmvdxuND10A=","X-Received":["by 2002:a05:6000:1acf:b0:381:cffb:f35e with SMTP id\n\tffacd0b85a97d-381f18851cbmr17236090f8f.54.1731503499017; \n\tWed, 13 Nov 2024 05:11:39 -0800 (PST)","by 2002:a05:6000:1acf:b0:381:cffb:f35e with SMTP id\n\tffacd0b85a97d-381f18851cbmr17236073f8f.54.1731503498596; \n\tWed, 13 Nov 2024 05:11:38 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGsn8Yohx3kaLdc/U/katLExLd33iKd6MuKGSjFlPAaorVZKBZt2ipumvpLfLPNcDbnsB+cFQ==","From":"Milan Zamazal ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org, Han-Lin Chen\n\t","Subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","In-Reply-To":"<20241113055524.3099340-2-chenghaoyang@chromium.org> (Harvey\n\tYang's message of \"Wed, 13 Nov 2024 05:54:32 +0000\")","References":"<20241113055524.3099340-1-chenghaoyang@chromium.org>\n\t<20241113055524.3099340-2-chenghaoyang@chromium.org>","Date":"Wed, 13 Nov 2024 14:11:36 +0100","Message-ID":"<87zfm3mg6v.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"1VLVUEkXr9kMN9-RKvZtEFTBhbXpqIwoPFMcnAg1hFg_1731503499","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":32294,"web_url":"https://patchwork.libcamera.org/comment/32294/","msgid":"<173205840985.2275610.4149001672920463078@ping.linuxembedded.co.uk>","date":"2024-11-19T23:20:09","subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-11-13 05:54:32)\n> To synchronize CPU access with mmap and hardware access on DMA buffers,\n> using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and\n> a helper class to allow users to sync buffers more easily.\n> \n\nI think the idea of scoped dma buffer control handling sounds useful,\nbut some comments below on how I would simplify this\nclass/implementation. But beyond simplfying ... I am not sure if\nDmaBufAllocator is the right place for this.\n\nThe documentation you've added talks about a 'map session' - and the\nintent is to ensure a mapped access is correctly managed for any DMA buf\ninteraction when accessed by the CPU.\n\nThe usage in 2/2 also seems to show it's more directly associated with a\nFrameBuffer (or a mapped frame buffer?) rather than the allocator ...\n\nIn MappedFrameBuffer - the user has also already specified with the\nMapFlag if this is a Read,Write, or ReadWrite operation ...\n\nIs the current user only using this associated with a MappedFrameBuffer?\nDo you foresee other users that would need to use DmaSyncer that\nwouldn't be tieing it to a MappedFrameBuffer?\n\n\nI've added the 'simple DmaSyncer' after I'd added other review comments,\nso it might look out of place if you read straight down.\n\n\n\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> .../libcamera/internal/dma_buf_allocator.h | 33 +++++++\n> src/libcamera/dma_buf_allocator.cpp | 90 +++++++++++++++++++\n> 2 files changed, 123 insertions(+)\n> \n> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> index d2a0a0d19..2b9b99678 100644\n> --- a/include/libcamera/internal/dma_buf_allocator.h\n> +++ b/include/libcamera/internal/dma_buf_allocator.h\n> @@ -23,6 +23,19 @@ public:\n> \n> using DmaBufAllocatorFlags = Flags;\n> \n> + enum class SyncStep {\n> + Start = 0,\n> + End\n> + };\n> +\n> + enum class SyncType {\n> + Read = 0,\n> + Write,\n> + ReadWrite,\n> + };\n> +\n> + static void sync(int fd, SyncStep step, SyncType type);\n\nWhy is sync() here as a static? Maybe it should be a function inside the\nDmaSyncer? Then it would have direct access to the fd_ that's stored in\nthat class?\n\n> +\n> DmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap);\n> ~DmaBufAllocator();\n> bool isValid() const { return providerHandle_.isValid(); }\n> @@ -35,6 +48,26 @@ private:\n> DmaBufAllocatorFlag type_;\n> };\n> \n> +class DmaSyncer final\n> +{\n> +public:\n> + explicit DmaSyncer(int fd,\n> + DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> + : fd_(fd), type_(type)\n> + {\n> + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_);\n> + }\n> +\n> + ~DmaSyncer()\n> + {\n> + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_);\n> + }\n> +\n> +private:\n> + int fd_;\n\nShould this be a SharedFD ? \n\nThe usage in patch 2/2 is coming from a FrameBuffer::Plane fd, so I\nthink we should just store that here if we need to store an fd_ ?\n\n> + DmaBufAllocator::SyncType type_;\n\nI also wonder if SyncType should be defined in the DmaSyncer class\ninstead of in the DmaBufAllocator class too!\n\n> +};\n> +\n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag)\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> index be6efb89f..f0f068cc1 100644\n> --- a/src/libcamera/dma_buf_allocator.cpp\n> +++ b/src/libcamera/dma_buf_allocator.cpp\n> @@ -78,6 +78,81 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator)\n> * \\brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values\n> */\n> \n> +/**\n> + * \\enum DmaBufAllocator::SyncStep\n> + * \\brief Either start or end of the synchronization\n> + * \\var DmaBufAllocator::Start\n> + * \\brief Indicates the start of a map access session\n> + * \\var DmaBufAllocator::End\n> + * \\brief Indicates the end of a map access session\n\nI think if the implementation moves into the DmaSyncer class, the 'step'\nbecomes a private implementation detail, as sync() is only called by the\nconstructor and destructor.\n\nI think at that point you could remove the 'Step' and just pass\nDMA_BUF_SYNC_START, or DMA_BUF_SYNC_END to sync() as the starting\ninitialisation of flags.\n\ni.e.\n\nDmaSyncer::sync(SyncType type, uint64_t flags)\n{\n\tswitch(type) {\n\tcase DmaBufAllocator::SyncType::Read:\n\t\tflags |= DMA_BUF_SYNC_READ;\n\t\tbreak;\n\t}\n\n\t...\n\t...\n}\n\nIn fact, now I see that - the type is then processed twice - so actually\nit should do that in the constructor - store the flags and then use that\nand this all gets shorter...\n\n\nThe following is not compiled... please expect bugs...\n\nclass DmaSyncer final\n{\npublic:\n\tenum class SyncType {\n\t\tRead = 0,\n\t\tWrite,\n\t\tReadWrite,\n\t};\n\n\texplicit DmaSyncer(SharedFD fd, SyncType type = ReadWrite)\n\t\t: fd_(fd)\n\t{\n\t\tswitch(type) {\n\t\tcase SyncType::Read:\n\t\t\tflags_ = DMA_BUF_SYNC_READ;\n\t\t\tbreak;\n\t\tcase SyncType::Write:\n\t\t\tflags_ = DMA_BUF_SYNC_WRITE;\n\t\t\tbreak;\n\t\tcase SyncType::ReadWrite:\n\t\t\tflags_ = DMA_BUF_SYNC_RW;\n\t\t\tbreak;\n\t\t}\n\n\t\tsync(DMA_BUF_SYNC_START);\n\t}\n\n\t~DmaSyncer()\n\t{\n\t\tsync(DMA_BUF_SYNC_END);\n\t}\n\nprivate:\n\tuint64_t flags = 0;\n\n\tvoid sync(uint64_t step)\n\t{\n\t\tstruct dma_buf_sync sync = {\n\t\t\t.flags = flags_ | step\n\t\t};\n\n\t\tint ret;\n\t\tdo {\n\t\t\tret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n\t\t} while (ret && (errno == EINTR || errno == EAGAIN));\n\n\t\tif (ret) {\n\t\t\tret = errno;\n\t\t\tLOG(DmaBufAllocator, Error)\n\t\t\t\t<< \"Unable to sync dma fd: \" << fd\n\t\t\t\t<< \", err: \" << strerror(ret)\n\t\t\t\t<< \", flags: \" << sync.flags;\n\t\t}\n\t}\n}\n\n\nDepending on who would actually call this - and if they are already\nexpected to have full knowledge of the DMA_BUF_SYNC_{READ,WRITE,RW}\ntypes - even that switch case could be removed from the constructor..\nbut it depends whether that's helpful/preferred to keep as a class enum\nor not.\n\n\n> + */\n> +\n> +/**\n> + * \\enum DmaBufAllocator::SyncType\n> + * \\brief Read and/or write access via the CPU map\n> + * \\var DmaBufAllocator::Read\n> + * \\brief Indicates that the mapped dma-buf will be read by the client via the\n> + * CPU map\n> + * \\var DmaBufAllocator::Write\n> + * \\brief Indicates that the mapped dm-buf will be written by the client via the\n> + * CPU map\n> + * \\var DmaBufAllocator::ReadWrite\n> + * \\brief Indicates that the mapped dma-buf will be read and written by the\n> + * client via the CPU map\n> + */\n> +\n> +/**\n> + * \\brief Synchronize CPU access and hardware access\n> + * \\param[in] fd The dma-buf's file descriptor to be synchronized\n> + * \\param[in] step Either start or end of the synchronization\n> + * \\param[in] type Read and/or write access during CPU mmap\n> + *\n> + * The client is expected to call this function with\n> + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the\n> + * start and end of buffer access respectively.\n\nI think if this goes into the DmaSyncer class this method would become\nprivate, and you won't need to document this. But some of the detail\nmight be suited to detail in the DmaSyncer class level documentation.\n\n> + */\n> +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type)\n> +{\n> + uint64_t flags = 0;\n> + switch (step) {\n> + case DmaBufAllocator::SyncStep::Start:\n> + flags = DMA_BUF_SYNC_START;\n> + break;\n> + case DmaBufAllocator::SyncStep::End:\n> + flags = DMA_BUF_SYNC_END;\n> + break;\n> + }\n> +\n> + switch (type) {\n> + case DmaBufAllocator::SyncType::Read:\n> + flags = flags | DMA_BUF_SYNC_READ;\n\n\tflags |= DMA_BUF_SYNC_READ;\n\nsame below.\n\n\n> + break;\n> + case DmaBufAllocator::SyncType::Write:\n> + flags = flags | DMA_BUF_SYNC_WRITE;\n> + break;\n> + case DmaBufAllocator::SyncType::ReadWrite:\n> + flags = flags | DMA_BUF_SYNC_RW;\n> + break;\n> + }\n> +\n> + struct dma_buf_sync sync = {\n> + .flags = flags\n> + };\n> +\n> + int ret;\n> + do {\n> + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> + } while (ret && (errno == EINTR || errno == EAGAIN));\n> +\n> + if (ret) {\n> + ret = errno;\n> + LOG(DmaBufAllocator, Error) << \"Unable to sync dma fd: \" << fd\n> + << \", err: \" << strerror(ret)\n> + << \", step: \" << static_cast(step)\n> + << \", type: \" << static_cast(type);\n> + }\n> +}\n> +\n> /**\n> * \\brief Construct a DmaBufAllocator of a given type\n> * \\param[in] type The type(s) of the dma-buf providers to allocate from\n> @@ -205,4 +280,19 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)\n> return allocFromHeap(name, size);\n> }\n> \n> +/**\n> + * \\class DmaSyncer\n> + * \\brief Helper class for dma-buf's synchronization\n> + *\n> + * This class wraps a userspace dma-buf's synchronization process with an\n> + * object's lifetime.\n\nEarlier you've called it a 'map session'. I think this is where the\ndocumentation needs detail about why someone should use this and what it\nsolves.\n\nTalking about a map session makes me think this should be associated in\nsome form with a MappedFrameBuffer more than the DmaBufAllocator ?\n\n\n\n> + */\n> +\n> +/**\n> + * \\fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> + * \\brief Construct a DmaSyncer with a dma-buf's fd and the access type\n> + * \\param[in] fd The dma-buf's file descriptor to synchronize\n> + * \\param[in] type Read and/or write access via the CPU map\n> + */\n> +\n> } /* namespace libcamera */\n> -- \n> 2.47.0.277.g8800431eea-goog\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 9BDAFC32F5\n\tfor ;\n\tTue, 19 Nov 2024 23:20:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58B2465F2B;\n\tWed, 20 Nov 2024 00:20:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86BD7658F0\n\tfor ;\n\tWed, 20 Nov 2024 00:20:13 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC0FF465;\n\tWed, 20 Nov 2024 00:19:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cTfrNuQK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732058395;\n\tbh=cpJzF56W/rcwb/g8Bir50EkGO8gKiqfG0ilxjw6ZMsM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cTfrNuQK+7Dq6DVu+og2TJDLbqVUIehrULKid7U4wBLXL/iGJdzqydUnboegqXiFr\n\tXUqk0PnhT5snpMFVOTjCc/UWlVo0SkxMuyLiymAWX8nEux3JNxLQ4oO1MuK/lqZrTL\n\tJJdSO/1Ql5S+Wj7U8//NjSr75EAIIOu4LWo8MC0Q=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241113055524.3099340-2-chenghaoyang@chromium.org>","References":"<20241113055524.3099340-1-chenghaoyang@chromium.org>\n\t<20241113055524.3099340-2-chenghaoyang@chromium.org>","Subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tHan-Lin Chen ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 19 Nov 2024 23:20:09 +0000","Message-ID":"<173205840985.2275610.4149001672920463078@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":32323,"web_url":"https://patchwork.libcamera.org/comment/32323/","msgid":"","date":"2024-11-21T05:51:09","subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Wed, Nov 20, 2024 at 7:20 AM Kieran Bingham\n wrote:\n>\n> Quoting Harvey Yang (2024-11-13 05:54:32)\n> > To synchronize CPU access with mmap and hardware access on DMA buffers,\n> > using `DMA_BUF_IOCTL_SYNC` is required. This patch adds a function and\n> > a helper class to allow users to sync buffers more easily.\n> >\n>\n> I think the idea of scoped dma buffer control handling sounds useful,\n> but some comments below on how I would simplify this\n> class/implementation. But beyond simplfying ... I am not sure if\n> DmaBufAllocator is the right place for this.\n>\n> The documentation you've added talks about a 'map session' - and the\n> intent is to ensure a mapped access is correctly managed for any DMA buf\n> interaction when accessed by the CPU.\n>\n> The usage in 2/2 also seems to show it's more directly associated with a\n> FrameBuffer (or a mapped frame buffer?) rather than the allocator ...\n>\n> In MappedFrameBuffer - the user has also already specified with the\n> MapFlag if this is a Read,Write, or ReadWrite operation ...\n>\n> Is the current user only using this associated with a MappedFrameBuffer?\n> Do you foresee other users that would need to use DmaSyncer that\n> wouldn't be tieing it to a MappedFrameBuffer?\n\nThere's only one case in the upcoming mtkisp7 [1].\n\nSeems that the file descriptors and memory address is preset. Memory\naddress is set by InfoFramePool's custom mmap [2].\n\n[1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/imgsys/single_device.cpp;l=141\n[2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/info_frame.cpp;l=109\n\n>\n>\n> I've added the 'simple DmaSyncer' after I'd added other review comments,\n> so it might look out of place if you read straight down.\n>\n>\n>\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > .../libcamera/internal/dma_buf_allocator.h | 33 +++++++\n> > src/libcamera/dma_buf_allocator.cpp | 90 +++++++++++++++++++\n> > 2 files changed, 123 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h\n> > index d2a0a0d19..2b9b99678 100644\n> > --- a/include/libcamera/internal/dma_buf_allocator.h\n> > +++ b/include/libcamera/internal/dma_buf_allocator.h\n> > @@ -23,6 +23,19 @@ public:\n> >\n> > using DmaBufAllocatorFlags = Flags;\n> >\n> > + enum class SyncStep {\n> > + Start = 0,\n> > + End\n> > + };\n> > +\n> > + enum class SyncType {\n> > + Read = 0,\n> > + Write,\n> > + ReadWrite,\n> > + };\n> > +\n> > + static void sync(int fd, SyncStep step, SyncType type);\n>\n> Why is sync() here as a static? Maybe it should be a function inside the\n> DmaSyncer? Then it would have direct access to the fd_ that's stored in\n> that class?\n\nYeah that makes more sense. [1] uses the statis sync function directly,\nwhile there's no reason not to use DmaSyncer there.\n\n>\n> > +\n> > DmaBufAllocator(DmaBufAllocatorFlags flags = DmaBufAllocatorFlag::CmaHeap);\n> > ~DmaBufAllocator();\n> > bool isValid() const { return providerHandle_.isValid(); }\n> > @@ -35,6 +48,26 @@ private:\n> > DmaBufAllocatorFlag type_;\n> > };\n> >\n> > +class DmaSyncer final\n> > +{\n> > +public:\n> > + explicit DmaSyncer(int fd,\n> > + DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> > + : fd_(fd), type_(type)\n> > + {\n> > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::Start, type_);\n> > + }\n> > +\n> > + ~DmaSyncer()\n> > + {\n> > + DmaBufAllocator::sync(fd_, DmaBufAllocator::SyncStep::End, type_);\n> > + }\n> > +\n> > +private:\n> > + int fd_;\n>\n> Should this be a SharedFD ?\n>\n> The usage in patch 2/2 is coming from a FrameBuffer::Plane fd, so I\n> think we should just store that here if we need to store an fd_ ?\n\n[1] Uses a preset integer file descriptor, while we can worry about it\nlater if you insist this should be a SharedFD.\n\n>\n> > + DmaBufAllocator::SyncType type_;\n>\n> I also wonder if SyncType should be defined in the DmaSyncer class\n> instead of in the DmaBufAllocator class too!\n\nDefinitely :)\n\n>\n> > +};\n> > +\n> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaBufAllocator::DmaBufAllocatorFlag)\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp\n> > index be6efb89f..f0f068cc1 100644\n> > --- a/src/libcamera/dma_buf_allocator.cpp\n> > +++ b/src/libcamera/dma_buf_allocator.cpp\n> > @@ -78,6 +78,81 @@ LOG_DEFINE_CATEGORY(DmaBufAllocator)\n> > * \\brief A bitwise combination of DmaBufAllocator::DmaBufAllocatorFlag values\n> > */\n> >\n> > +/**\n> > + * \\enum DmaBufAllocator::SyncStep\n> > + * \\brief Either start or end of the synchronization\n> > + * \\var DmaBufAllocator::Start\n> > + * \\brief Indicates the start of a map access session\n> > + * \\var DmaBufAllocator::End\n> > + * \\brief Indicates the end of a map access session\n>\n> I think if the implementation moves into the DmaSyncer class, the 'step'\n> becomes a private implementation detail, as sync() is only called by the\n> constructor and destructor.\n>\n> I think at that point you could remove the 'Step' and just pass\n> DMA_BUF_SYNC_START, or DMA_BUF_SYNC_END to sync() as the starting\n> initialisation of flags.\n>\n> i.e.\n>\n> DmaSyncer::sync(SyncType type, uint64_t flags)\n> {\n> switch(type) {\n> case DmaBufAllocator::SyncType::Read:\n> flags |= DMA_BUF_SYNC_READ;\n> break;\n> }\n>\n> ...\n> ...\n> }\n>\n> In fact, now I see that - the type is then processed twice - so actually\n> it should do that in the constructor - store the flags and then use that\n> and this all gets shorter...\n>\n>\n> The following is not compiled... please expect bugs...\n>\n> class DmaSyncer final\n> {\n> public:\n> enum class SyncType {\n> Read = 0,\n> Write,\n> ReadWrite,\n> };\n>\n> explicit DmaSyncer(SharedFD fd, SyncType type = ReadWrite)\n> : fd_(fd)\n> {\n> switch(type) {\n> case SyncType::Read:\n> flags_ = DMA_BUF_SYNC_READ;\n> break;\n> case SyncType::Write:\n> flags_ = DMA_BUF_SYNC_WRITE;\n> break;\n> case SyncType::ReadWrite:\n> flags_ = DMA_BUF_SYNC_RW;\n> break;\n> }\n>\n> sync(DMA_BUF_SYNC_START);\n> }\n>\n> ~DmaSyncer()\n> {\n> sync(DMA_BUF_SYNC_END);\n> }\n>\n> private:\n> uint64_t flags = 0;\n>\n> void sync(uint64_t step)\n> {\n> struct dma_buf_sync sync = {\n> .flags = flags_ | step\n> };\n>\n> int ret;\n> do {\n> ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> } while (ret && (errno == EINTR || errno == EAGAIN));\n>\n> if (ret) {\n> ret = errno;\n> LOG(DmaBufAllocator, Error)\n> << \"Unable to sync dma fd: \" << fd\n> << \", err: \" << strerror(ret)\n> << \", flags: \" << sync.flags;\n> }\n> }\n> }\n>\n>\n> Depending on who would actually call this - and if they are already\n> expected to have full knowledge of the DMA_BUF_SYNC_{READ,WRITE,RW}\n> types - even that switch case could be removed from the constructor..\n> but it depends whether that's helpful/preferred to keep as a class enum\n> or not.\n\nThanks! Mostly adopted in the next version.\n\n>\n>\n> > + */\n> > +\n> > +/**\n> > + * \\enum DmaBufAllocator::SyncType\n> > + * \\brief Read and/or write access via the CPU map\n> > + * \\var DmaBufAllocator::Read\n> > + * \\brief Indicates that the mapped dma-buf will be read by the client via the\n> > + * CPU map\n> > + * \\var DmaBufAllocator::Write\n> > + * \\brief Indicates that the mapped dm-buf will be written by the client via the\n> > + * CPU map\n> > + * \\var DmaBufAllocator::ReadWrite\n> > + * \\brief Indicates that the mapped dma-buf will be read and written by the\n> > + * client via the CPU map\n> > + */\n> > +\n> > +/**\n> > + * \\brief Synchronize CPU access and hardware access\n> > + * \\param[in] fd The dma-buf's file descriptor to be synchronized\n> > + * \\param[in] step Either start or end of the synchronization\n> > + * \\param[in] type Read and/or write access during CPU mmap\n> > + *\n> > + * The client is expected to call this function with\n> > + * DmaBufAllocator::SyncStep::Start and DmaBufAllocator::SyncStep::End at the\n> > + * start and end of buffer access respectively.\n>\n> I think if this goes into the DmaSyncer class this method would become\n> private, and you won't need to document this. But some of the detail\n> might be suited to detail in the DmaSyncer class level documentation.\n\nDone, while I'm very bad at documentation. Please correct/amend me\nif necessary :p.\n\n>\n> > + */\n> > +void DmaBufAllocator::sync(int fd, DmaBufAllocator::SyncStep step, DmaBufAllocator::SyncType type)\n> > +{\n> > + uint64_t flags = 0;\n> > + switch (step) {\n> > + case DmaBufAllocator::SyncStep::Start:\n> > + flags = DMA_BUF_SYNC_START;\n> > + break;\n> > + case DmaBufAllocator::SyncStep::End:\n> > + flags = DMA_BUF_SYNC_END;\n> > + break;\n> > + }\n> > +\n> > + switch (type) {\n> > + case DmaBufAllocator::SyncType::Read:\n> > + flags = flags | DMA_BUF_SYNC_READ;\n>\n> flags |= DMA_BUF_SYNC_READ;\n>\n> same below.\n>\n>\n> > + break;\n> > + case DmaBufAllocator::SyncType::Write:\n> > + flags = flags | DMA_BUF_SYNC_WRITE;\n> > + break;\n> > + case DmaBufAllocator::SyncType::ReadWrite:\n> > + flags = flags | DMA_BUF_SYNC_RW;\n> > + break;\n> > + }\n> > +\n> > + struct dma_buf_sync sync = {\n> > + .flags = flags\n> > + };\n> > +\n> > + int ret;\n> > + do {\n> > + ret = ioctl(fd, DMA_BUF_IOCTL_SYNC, &sync);\n> > + } while (ret && (errno == EINTR || errno == EAGAIN));\n> > +\n> > + if (ret) {\n> > + ret = errno;\n> > + LOG(DmaBufAllocator, Error) << \"Unable to sync dma fd: \" << fd\n> > + << \", err: \" << strerror(ret)\n> > + << \", step: \" << static_cast(step)\n> > + << \", type: \" << static_cast(type);\n> > + }\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a DmaBufAllocator of a given type\n> > * \\param[in] type The type(s) of the dma-buf providers to allocate from\n> > @@ -205,4 +280,19 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)\n> > return allocFromHeap(name, size);\n> > }\n> >\n> > +/**\n> > + * \\class DmaSyncer\n> > + * \\brief Helper class for dma-buf's synchronization\n> > + *\n> > + * This class wraps a userspace dma-buf's synchronization process with an\n> > + * object's lifetime.\n>\n> Earlier you've called it a 'map session'. I think this is where the\n> documentation needs detail about why someone should use this and what it\n> solves.\n\nAdded a short description.\n\n>\n> Talking about a map session makes me think this should be associated in\n> some form with a MappedFrameBuffer more than the DmaBufAllocator ?\n\nApart from the usage with the upcoming InfoFramePool, I'm fine with\nputting it aside MappedFrameBuffer, or even in a new file. Just let me\nknow your preference.\n\nBR,\nHarvey\n\n>\n>\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn DmaSyncer::DmaSyncer(int fd, DmaBufAllocator::SyncType type = DmaBufAllocator::SyncType::ReadWrite)\n> > + * \\brief Construct a DmaSyncer with a dma-buf's fd and the access type\n> > + * \\param[in] fd The dma-buf's file descriptor to synchronize\n> > + * \\param[in] type Read and/or write access via the CPU map\n> > + */\n> > +\n> > } /* namespace libcamera */\n> > --\n> > 2.47.0.277.g8800431eea-goog\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 173FEC326C\n\tfor ;\n\tThu, 21 Nov 2024 05:51:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37C2B65FA1;\n\tThu, 21 Nov 2024 06:51:24 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B92765F54\n\tfor ;\n\tThu, 21 Nov 2024 06:51:22 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2fb584a8f81so5746191fa.3\n\tfor ;\n\tWed, 20 Nov 2024 21:51:22 -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=\"Rw4XLD8x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1732168281; x=1732773081;\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=sMdyxmmpeFi2zPdzWpV7wFbuYVe7hCAqZG5kfJus1VA=;\n\tb=Rw4XLD8xoswy85c7/wye0Q+/tGn7cYuMh1HkQ4e4VgjoSa292TLOztKuhSdBnuYBJ6\n\tikQWKSyBL5FEr83oiwiXy9rL3PdaY5Pi/PWli2gUrTxk6Q5Vda61uEbwAU7bya9W1FP3\n\tUvhr4OFqWgokneBmQIAOiVI0P/2ULdJTBy67Q=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732168281; x=1732773081;\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=sMdyxmmpeFi2zPdzWpV7wFbuYVe7hCAqZG5kfJus1VA=;\n\tb=QZmNwghR+HyoLcWLkqeC6Kt1euppqqCaYWOK4dsV2IoyFHFhT3etU55SkUNR3Y3Akc\n\tbKopoo7UQdUbYKg142wSmdigfi4EcN1hmqR3Nm6EPzzDzdS1mp0iYzMPHc7/xhONBply\n\tO+PXdrlNKHLihGll0cGO5GHUzNiBxuDCY63RzSzd5+aQP0Kvrw/IXiGoX09LNSMDZrr3\n\t2F9nvH/oa1VEkXjiXtuFivESSVQe/Xx3SryHDnY0sjqoLAThwEtY830M64+WxL2reMrp\n\t2AnVSUKbxZfJVW8bxZDCJAUDVwPVPIbQ4Sa9Hl5pChLdbDctJDspJcbM0n5zXpu2vFi0\n\tRClw==","X-Gm-Message-State":"AOJu0YwEcRo1xkl/Bdm+L6sSX+2RH5i+q3eC7ilpkqZsh24no02R9lyn\n\tuOtOpspBlRgeUFHMFlOPXJbUmmO1Tvrx+YAYocm2UN5OhkQqSYeiZcK8PArbcEoAtOHzq/iVByk\n\tROJUJ3Zb8qT9jXHCUT13jgSYdJrBQpGRVKQsx","X-Gm-Gg":"ASbGncskPt5E53NxWElBukJijQ6tC9uXS9M7XFs/4i+AsC3dIy5DsOHwwJ/0yA2TYGc\n\tDKMgbIkVGqDqVPxt+Lh9hJ08U3TXpLvbtCgYMl+koQZBPNRXo2AB54XSgUeY=","X-Google-Smtp-Source":"AGHT+IFdY8SNc5mrL9vAnpvl6cvxXT1McGg8ZMigFZClry76AYPSiJi/UkZiQgpxXgEThDmvBprA/1zqafoXwqW4TFA=","X-Received":"by 2002:a2e:a555:0:b0:2ff:5f94:e635 with SMTP id\n\t38308e7fff4ca-2ff8db66087mr28001871fa.3.1732168281010;\n\tWed, 20 Nov 2024 21:51:21 -0800 (PST)","MIME-Version":"1.0","References":"<20241113055524.3099340-1-chenghaoyang@chromium.org>\n\t<20241113055524.3099340-2-chenghaoyang@chromium.org>\n\t<173205840985.2275610.4149001672920463078@ping.linuxembedded.co.uk>","In-Reply-To":"<173205840985.2275610.4149001672920463078@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Thu, 21 Nov 2024 13:51:09 +0800","Message-ID":"","Subject":"Re: [PATCH v2 1/2] DmaBufAllocator: Add Dma Buffer synchronization\n\tfunction & helper class","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","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\" "}}]