[{"id":33612,"web_url":"https://patchwork.libcamera.org/comment/33612/","msgid":"<nr2yv6jfdrikvd3fyqqgyojd5jhd4gjatahld2l2pogqaytd66@khdugirutzcs>","date":"2025-03-17T18:33:19","subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Mar 10, 2025 at 06:03:26PM +0100, Barnabás Pőcze wrote:\n> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`\n> member is not used in contexts where its atomicity would matter.\n> So it does not need to be have an atomic type.\n>\n> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n\nThis had me do more reasoning than expected, so let me see if I got\nthis right.\n\nThe buffer cached is accessed at buffer queue time (get()) and at\nbuffer dequeue time (put()). Dequeue is an async event generated by\nthe video device while buffer queuing is triggered by the application.\n\ndequeue is triggered by an event dispatcher, while queueRequest runs in\nthe pipeline handler thread (which is the same thread where\nall other Object (but Threads) run in libcamera).\n\nAs the event dispatcher is setEnabled(true) at the first call to\nqueueBuffers, the thread it runs on is again the pipeline handler thread.\n\nSo yeah, seems safe indeed\n\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\n> ---\n>  include/libcamera/internal/v4l2_videodevice.h | 2 +-\n>  src/libcamera/v4l2_videodevice.cpp            | 4 ++--\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index f021c2a01..e52b50c67 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -158,7 +158,7 @@ private:\n>  \t\tstd::vector<Plane> planes_;\n>  \t};\n>\n> -\tstd::atomic<uint64_t> lastUsedCounter_;\n> +\tuint64_t lastUsedCounter_;\n>  \tstd::vector<Entry> cache_;\n>  \t/* \\todo Expose the miss counter through an instrumentation API. */\n>  \tunsigned int missCounter_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index e241eb47b..f5b3fa09d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n>  {\n>  \tfor (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n>  \t\tcache_.emplace_back(true,\n> -\t\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +\t\t\t\t    lastUsedCounter_++,\n>  \t\t\t\t    *buffer.get());\n>  }\n>\n> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n>  \t\treturn -ENOENT;\n>\n>  \tcache_[use] = Entry(false,\n> -\t\t\t    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +\t\t\t    lastUsedCounter_++,\n>  \t\t\t    buffer);\n>\n>  \treturn use;\n> --\n> 2.48.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4C860C32FA\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 17 Mar 2025 18:33:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D7A768942;\n\tMon, 17 Mar 2025 19:33:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E0D9617F8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 17 Mar 2025 19:33:23 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 822C6352;\n\tMon, 17 Mar 2025 19:31:41 +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=\"o64QtA5w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742236301;\n\tbh=6XN0pt508QQPgbiHa0ACKDWQz7pJ/Y9vA4arCjWObX8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o64QtA5woD9+N+UoLnSX1bGTc7D3XiYqHUNlj0A/mKQe0K4HxOLmV0Fw0PPAii1tm\n\tXSb31UfTE1yAw+TKkA/ufjG4vn/rPbcwWyndhyL758+B5SLxyuEhrouIlM5mXy5nHb\n\tJPVkgdSZbsB7w/V86LrztbGj19oPPZ4mxLsVNhEw=","Date":"Mon, 17 Mar 2025 19:33:19 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","Message-ID":"<nr2yv6jfdrikvd3fyqqgyojd5jhd4gjatahld2l2pogqaytd66@khdugirutzcs>","References":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33652,"web_url":"https://patchwork.libcamera.org/comment/33652/","msgid":"<174239601608.2025019.14312188626019501902@ping.linuxembedded.co.uk>","date":"2025-03-19T14:53:36","subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-10 17:03:26)\n> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`\n> member is not used in contexts where its atomicity would matter.\n> So it does not need to be have an atomic type.\n\nAny indications as to why this was using atomic ? Maybe just\ncautious development when it was first put in ?\n\nAs long as we're convinced it's still safe and passes the tests, I'm\nfine with it. It's certainly simpler code.\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n> ---\n>  include/libcamera/internal/v4l2_videodevice.h | 2 +-\n>  src/libcamera/v4l2_videodevice.cpp            | 4 ++--\n>  2 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index f021c2a01..e52b50c67 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -158,7 +158,7 @@ private:\n>                 std::vector<Plane> planes_;\n>         };\n>  \n> -       std::atomic<uint64_t> lastUsedCounter_;\n> +       uint64_t lastUsedCounter_;\n>         std::vector<Entry> cache_;\n>         /* \\todo Expose the miss counter through an instrumentation API. */\n>         unsigned int missCounter_;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index e241eb47b..f5b3fa09d 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n>  {\n>         for (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n>                 cache_.emplace_back(true,\n> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +                                   lastUsedCounter_++,\n>                                     *buffer.get());\n>  }\n>  \n> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n>                 return -ENOENT;\n>  \n>         cache_[use] = Entry(false,\n> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> +                           lastUsedCounter_++,\n>                             buffer);\n>  \n>         return use;\n> -- \n> 2.48.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B0685C32FE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 19 Mar 2025 14:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 226B968945;\n\tWed, 19 Mar 2025 15:53:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8FE8687FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Mar 2025 15:53:39 +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 BD64655A;\n\tWed, 19 Mar 2025 15:51:56 +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=\"IRJeyk9x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742395916;\n\tbh=zQQ7wy+p2UsB7+Cdxqqd4k0a3zNiMNiKPCHKAp5NBBM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=IRJeyk9xZqx9xeBnB18NzpcEApWmyq6ihoXoNoIq/HvmFF5pEd156OhjJWT8iMPGE\n\tNPzBx4d7fwRsMwreMMQ6YbGcbSY02EeX7/7Rzu22ucAbDnU9+iFuImXJn7KZ9YW7fB\n\tGJZj86rx5LS/sHv4NHPXtO9u1I4QDvuAQuu3jYa8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>","References":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 19 Mar 2025 14:53:36 +0000","Message-ID":"<174239601608.2025019.14312188626019501902@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33654,"web_url":"https://patchwork.libcamera.org/comment/33654/","msgid":"<b58b03c4-d337-4bd9-94ef-1909c5165196@ideasonboard.com>","date":"2025-03-19T15:34:44","subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 19. 15:53 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-03-10 17:03:26)\n>> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`\n>> member is not used in contexts where its atomicity would matter.\n>> So it does not need to be have an atomic type.\n> \n> Any indications as to why this was using atomic ? Maybe just\n> cautious development when it was first put in ?\n\nNot sure. It was Laurent who suggested it: https://patchwork.libcamera.org/patch/2878/#3889\n\nThere is only a single place where `lastUsedCounter_` might be used\nby multiple threads: `V4L2BufferCache::get()`. But that function\nis not thread safe, so either `lastUsedCounter_` is unnecessarily\natomic or race conditions are rampant already. As far as I can\ntell it is the former.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> As long as we're convinced it's still safe and passes the tests, I'm\n> fine with it. It's certainly simpler code.\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n>> ---\n>>   include/libcamera/internal/v4l2_videodevice.h | 2 +-\n>>   src/libcamera/v4l2_videodevice.cpp            | 4 ++--\n>>   2 files changed, 3 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>> index f021c2a01..e52b50c67 100644\n>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>> @@ -158,7 +158,7 @@ private:\n>>                  std::vector<Plane> planes_;\n>>          };\n>>   \n>> -       std::atomic<uint64_t> lastUsedCounter_;\n>> +       uint64_t lastUsedCounter_;\n>>          std::vector<Entry> cache_;\n>>          /* \\todo Expose the miss counter through an instrumentation API. */\n>>          unsigned int missCounter_;\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index e241eb47b..f5b3fa09d 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n>>   {\n>>          for (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n>>                  cache_.emplace_back(true,\n>> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n>> +                                   lastUsedCounter_++,\n>>                                      *buffer.get());\n>>   }\n>>   \n>> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n>>                  return -ENOENT;\n>>   \n>>          cache_[use] = Entry(false,\n>> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n>> +                           lastUsedCounter_++,\n>>                              buffer);\n>>   \n>>          return use;\n>> -- \n>> 2.48.1\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 977B6C32FE\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 19 Mar 2025 15:34:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4758668953;\n\tWed, 19 Mar 2025 16:34:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94ABE687FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Mar 2025 16:34:48 +0100 (CET)","from [192.168.33.18] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 610591E6;\n\tWed, 19 Mar 2025 16:33:05 +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=\"uQtJqKBe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742398385;\n\tbh=t1mZWdLLeFzvo5yHI1NspbhsAU3AihEvjyK007X1Pq4=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=uQtJqKBeIUDEWMeWMYVEAvRagBXUZp8Ul014ODyT4wndRFa4VbNt6v0PFJ2xdZpdn\n\tXqoITmUgiV+Siuam8p7vtPu7sa+ehBrYJQh6vvVARN6csn+7IZbjj7UCiLEV7DpTL0\n\tEtmjrmKiGU5XYeP/YhEJga5fX9ZlVp2YIH1Ua4rc=","Message-ID":"<b58b03c4-d337-4bd9-94ef-1909c5165196@ideasonboard.com>","Date":"Wed, 19 Mar 2025 16:34:44 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>\n\t<174239601608.2025019.14312188626019501902@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<174239601608.2025019.14312188626019501902@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33708,"web_url":"https://patchwork.libcamera.org/comment/33708/","msgid":"<20250326021046.GF23984@pendragon.ideasonboard.com>","date":"2025-03-26T02:10:46","subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Mar 19, 2025 at 04:34:44PM +0100, Barnabás Pőcze wrote:\n> 2025. 03. 19. 15:53 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-03-10 17:03:26)\n> >> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`\n> >> member is not used in contexts where its atomicity would matter.\n> >> So it does not need to be have an atomic type.\n> > \n> > Any indications as to why this was using atomic ? Maybe just\n> > cautious development when it was first put in ?\n> \n> Not sure. It was Laurent who suggested it: https://patchwork.libcamera.org/patch/2878/#3889\n> \n> There is only a single place where `lastUsedCounter_` might be used\n> by multiple threads: `V4L2BufferCache::get()`. But that function\n> is not thread safe, so either `lastUsedCounter_` is unnecessarily\n> atomic or race conditions are rampant already. As far as I can\n> tell it is the former.\n\nAgreed.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > As long as we're convinced it's still safe and passes the tests, I'm\n> > fine with it. It's certainly simpler code.\n> > \n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > \n> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>\n> >> ---\n> >>   include/libcamera/internal/v4l2_videodevice.h | 2 +-\n> >>   src/libcamera/v4l2_videodevice.cpp            | 4 ++--\n> >>   2 files changed, 3 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> >> index f021c2a01..e52b50c67 100644\n> >> --- a/include/libcamera/internal/v4l2_videodevice.h\n> >> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> >> @@ -158,7 +158,7 @@ private:\n> >>                  std::vector<Plane> planes_;\n> >>          };\n> >>   \n> >> -       std::atomic<uint64_t> lastUsedCounter_;\n> >> +       uint64_t lastUsedCounter_;\n> >>          std::vector<Entry> cache_;\n> >>          /* \\todo Expose the miss counter through an instrumentation API. */\n> >>          unsigned int missCounter_;\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index e241eb47b..f5b3fa09d 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>\n> >>   {\n> >>          for (const std::unique_ptr<FrameBuffer> &buffer : buffers)\n> >>                  cache_.emplace_back(true,\n> >> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> >> +                                   lastUsedCounter_++,\n> >>                                      *buffer.get());\n> >>   }\n> >>   \n> >> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)\n> >>                  return -ENOENT;\n> >>   \n> >>          cache_[use] = Entry(false,\n> >> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),\n> >> +                           lastUsedCounter_++,\n> >>                              buffer);\n> >>   \n> >>          return use;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AF52EC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Mar 2025 02:11:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE86D68952;\n\tWed, 26 Mar 2025 03:11:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDC65617F1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Mar 2025 03:11:09 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 284F5475;\n\tWed, 26 Mar 2025 03:09:22 +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=\"BP+Sl7Gu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742954962;\n\tbh=y3/uVzEcRsaMfVVh84H9pm6rE3IAWmtaBt1PaSPePbQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BP+Sl7GuDIBgc9yS97FdT0qxhbk+sfCrdg+ssXef7kJZjvuc3zGQ26FjweUEZwfKd\n\tIc5OBxES5/Kq4Eaa+t1j9oFDRRWjbW/nRnGZTsxtpuS01zi08hqbeJ9UUDBvfTx2YG\n\tTYpyAuPM4FGVgpO6SY9TJ9vSUGD2paStkIz0/4UI=","Date":"Wed, 26 Mar 2025 04:10:46 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need\n\tnot be atomic","Message-ID":"<20250326021046.GF23984@pendragon.ideasonboard.com>","References":"<20250310170326.185273-1-barnabas.pocze@ideasonboard.com>\n\t<174239601608.2025019.14312188626019501902@ping.linuxembedded.co.uk>\n\t<b58b03c4-d337-4bd9-94ef-1909c5165196@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<b58b03c4-d337-4bd9-94ef-1909c5165196@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]