[{"id":30882,"web_url":"https://patchwork.libcamera.org/comment/30882/","msgid":"","date":"2024-08-21T16:34:17","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Hans,\n\nThanks for the patch. I've tested it on mtkisp7, and it works fine.\n\nReviewed-by: Harvey Yang \n\nOne question that might not be related though:\n`PipelineHandler::release` is defined to be thread safe, while\nthere's no such description for `PipelineHandler::releaseDevice`.\nIn mtkisp7, we have a CL [1] to make sure\n`PipelineHandler::releaseDevice` is always blockingly executed\non PipelineHandler's thread.\n\nAlthough it makes the normal cases (that it's returning true\ndirectly) run longer, as it waits for another thread's execution,\nit prevents PipelineHandler's mis-implementation.\nI'd suggest either we do the same in the upstream, or we add\nthe description of being thread-safe for\n`PipelineHandler::releaseDevice` to remind the developers of\npipeline handlers.\n\nWDYT?\n\nBR,\nHarvey\n\n[1]:\nhttps://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n\nOn Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n\n> It is better / more logical to call releaseDevice() before unlocking\n> the devices. ATM the only pipeline handler implementing releaseDevice()\n> is the rpi pipeline handler which releases buffers from its releaseDevice()\n> implementation.\n>\n> Releasing buffers before unlocking the media devices is ok to do\n> and arguably it is better to release the buffers before unlocking.\n>\n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/pipeline_handler.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> index a20cd27d..1fc22d6a 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n>\n> ASSERT(useCount_);\n>\n> + releaseDevice(camera);\n> +\n> if (useCount_ == 1)\n> unlockMediaDevices();\n>\n> - releaseDevice(camera);\n> -\n> --useCount_;\n> }\n>\n> --\n> 2.46.0\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 54F09C323E\n\tfor ;\n\tWed, 21 Aug 2024 16:34:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 020B5633CF;\n\tWed, 21 Aug 2024 18:34:31 +0200 (CEST)","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 EFB0763394\n\tfor ;\n\tWed, 21 Aug 2024 18:34:28 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2f3ce5bc7d2so48935821fa.0\n\tfor ;\n\tWed, 21 Aug 2024 09:34:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"XoddRaNI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724258068; x=1724862868;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=qx1pqshLVzz+Q+JnZgKZCiG0t2HFQNo+QuGzCAo4yvg=;\n\tb=XoddRaNItU+eEZgcufGm6vtfXxg7tbQjaF0tl1OL2j0t0bvX3EAuhQ5ZUT4vxZ4nre\n\tEwmHLUr68Kr8kZTHxjAX0CVxVgUyBQa1dsSYv/h02eQTYDnPqX765KCE5Of9bOOspoeT\n\titHiUjsx3CT6p0Je4vzLajiKXDy3mb5zui7gk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724258068; x=1724862868;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=qx1pqshLVzz+Q+JnZgKZCiG0t2HFQNo+QuGzCAo4yvg=;\n\tb=I/WxMrHpi7poGJVMkKLtpaQB+SeCprRq6WBdpjVEXbrMRJoGjbp/tA2gk7Z4bSUafq\n\tVsCiMyBqs4yQ9sGXAepzWyAofKkKwaVi7eynvQZT/7izuJTQrj8PRkdTn4bQJLm4kBsE\n\teskV+l7p3I/3zjvvmvTl5R8746yr6/7g+7uo+oD7oG8PhW2e1O8RLMyAQFlub5axa1cK\n\tkx9uCTU/CI8jy+xGu9Ilnv5zrnJqCsvhw6OhpbUM/6PqJz1y3h64z9usbw6QaLP8LnA2\n\tXjATwwmyjfYAeE/j1pehRGellG8wzPrIrBMCcnHeUfROz2NQdZ2mjagVAGpZ2xZdJvCq\n\tBOYw==","X-Gm-Message-State":"AOJu0YwNtX6tjajg+gG+8k/RO7o5xhRfu4WmB3ns0n4M/fN0hXqLG5Gc\n\txyCJfG3Soe150kr4ecU3IfWML3ogMWk63GOxWtKdvLrzl9cCpnaxgqQB5hAMZD3ypMZKlL6Ovzj\n\tiOOw0VUmuLLwQif857t8Bjx4qci/1iYJWjOte","X-Google-Smtp-Source":"AGHT+IFYhjhtlKEpg8WONtO22PLN84TEZZFJfJGcRLVwwZTvv/muT6CNJTP19IpvkxS1f3fNEm0f0/0FbZn30AjHbjA=","X-Received":"by 2002:a2e:350d:0:b0:2f0:1fd5:2f29 with SMTP id\n\t38308e7fff4ca-2f3f8863d9bmr17506621fa.19.1724258067741;\n\tWed, 21 Aug 2024 09:34:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>","In-Reply-To":"<20240820195016.16028-3-hdegoede@redhat.com>","From":"Cheng-Hao Yang ","Date":"Wed, 21 Aug 2024 18:34:17 +0200","Message-ID":"","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","Content-Type":"multipart/alternative; boundary=\"000000000000a112b00620341dcd\"","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":30892,"web_url":"https://patchwork.libcamera.org/comment/30892/","msgid":"<20240825002712.GA17238@pendragon.ideasonboard.com>","date":"2024-08-25T00:27:12","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> Hi Hans,\n> \n> Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n> \n> Reviewed-by: Harvey Yang \n> \n> One question that might not be related though:\n> `PipelineHandler::release` is defined to be thread safe, while\n> there's no such description for `PipelineHandler::releaseDevice`.\n>\n> In mtkisp7, we have a CL [1] to make sure\n> `PipelineHandler::releaseDevice` is always blockingly executed\n> on PipelineHandler's thread.\n> \n> Although it makes the normal cases (that it's returning true\n> directly) run longer, as it waits for another thread's execution,\n> it prevents PipelineHandler's mis-implementation.\n> I'd suggest either we do the same in the upstream, or we add\n> the description of being thread-safe for\n> `PipelineHandler::releaseDevice` to remind the developers of\n> pipeline handlers.\n\nI don't think releaseDevice() qualifies as thread safe. Here's how\nlibcamera documents thread safety:\n\n * - A **reentrant** function may be called simultaneously from multiple\n * threads if and only if each invocation uses a different instance of the\n * class. This is the default for all member functions not explictly marked\n * otherwise.\n *\n * - \\anchor thread-safe A **thread-safe** function may be called\n * simultaneously from multiple threads on the same instance of a class. A\n * thread-safe function is thus reentrant. Thread-safe functions may also be\n * called simultaneously with any other reentrant function of the same class\n * on the same instance.\n\nPipeline handlers shouldn't have to deal with releaseDevice() being\ncalled concurrently on the same PipelineHandler instance for different\ncameras, which is why the caller serializes the calls.\n\nreleaseDevice() should never race with other pipeline handler calls for\nthe same camera. However, it could race with other calls for different\ncameras. We should document this, or change the behaviour.\n\n> WDYT?\n> \n> BR,\n> Harvey\n> \n> [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n\nThe commit message doesn't really explain why the change is needed.\nCould you elaborate, maybe providing an example of incorrect behaviour\nof a pipeline handler implementation with the existing calling\nconvention ?\n\n> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> \n> > It is better / more logical to call releaseDevice() before unlocking\n> > the devices. ATM the only pipeline handler implementing releaseDevice()\n\ns/ATM/At the moment/\n\n> > is the rpi pipeline handler which releases buffers from its releaseDevice()\n> > implementation.\n> >\n> > Releasing buffers before unlocking the media devices is ok to do\n> > and arguably it is better to release the buffers before unlocking.\n> >\n> > Signed-off-by: Hans de Goede \n\nReviewed-by: Laurent Pinchart \n\n> > ---\n> > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> > b/src/libcamera/pipeline_handler.cpp\n> > index a20cd27d..1fc22d6a 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n> >\n> > ASSERT(useCount_);\n> >\n> > + releaseDevice(camera);\n> > +\n> > if (useCount_ == 1)\n> > unlockMediaDevices();\n> >\n> > - releaseDevice(camera);\n> > -\n> > --useCount_;\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 4F467C323E\n\tfor ;\n\tSun, 25 Aug 2024 00:27:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C914633CF;\n\tSun, 25 Aug 2024 02:27:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8E1A63393\n\tfor ;\n\tSun, 25 Aug 2024 02:27:16 +0200 (CEST)","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 4790F480;\n\tSun, 25 Aug 2024 02:26:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p0mAphCz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724545571;\n\tbh=kHP6/QH/AkuPwqH4obG2dOsvNOSePbM8ymY+rQAQ6J4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p0mAphCzW9FaQjiUtGAF8tVsig9XiQ3JvKSMeYaaMOCRyfM3pYAXWcR4stZn1qS8N\n\tTsAuJJ1I3jnV7gUdH+AyVj8hdmY9/ATJ30GnmoE564cgX3kbN+lrU+T1c+zqedL964\n\tOcVNUts5qpl88TyWIQw45DEbyzp7OYCyiCZX0cNI=","Date":"Sun, 25 Aug 2024 03:27:12 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","Message-ID":"<20240825002712.GA17238@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":30900,"web_url":"https://patchwork.libcamera.org/comment/30900/","msgid":"","date":"2024-08-25T11:21:21","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> > Hi Hans,\n> >\n> > Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n> >\n> > Reviewed-by: Harvey Yang \n> >\n> > One question that might not be related though:\n> > `PipelineHandler::release` is defined to be thread safe, while\n> > there's no such description for `PipelineHandler::releaseDevice`.\n> >\n> > In mtkisp7, we have a CL [1] to make sure\n> > `PipelineHandler::releaseDevice` is always blockingly executed\n> > on PipelineHandler's thread.\n> >\n> > Although it makes the normal cases (that it's returning true\n> > directly) run longer, as it waits for another thread's execution,\n> > it prevents PipelineHandler's mis-implementation.\n> > I'd suggest either we do the same in the upstream, or we add\n> > the description of being thread-safe for\n> > `PipelineHandler::releaseDevice` to remind the developers of\n> > pipeline handlers.\n>\n> I don't think releaseDevice() qualifies as thread safe. Here's how\n> libcamera documents thread safety:\n>\n> * - A **reentrant** function may be called simultaneously from multiple\n> * threads if and only if each invocation uses a different instance of\n> the\n> * class. This is the default for all member functions not explictly\n> marked\n> * otherwise.\n> *\n> * - \\anchor thread-safe A **thread-safe** function may be called\n> * simultaneously from multiple threads on the same instance of a class.\n> A\n> * thread-safe function is thus reentrant. Thread-safe functions may\n> also be\n> * called simultaneously with any other reentrant function of the same\n> class\n> * on the same instance.\n>\n> Pipeline handlers shouldn't have to deal with releaseDevice() being\n> called concurrently on the same PipelineHandler instance for different\n> cameras, which is why the caller serializes the calls.\n>\n> releaseDevice() should never race with other pipeline handler calls for\n> the same camera. However, it could race with other calls for different\n> cameras. We should document this, or change the behaviour.\n>\n>\nYes, you're right. Thanks for the clarification.\n\n\n> > WDYT?\n> >\n> > BR,\n> > Harvey\n> >\n> > [1]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n>\n> The commit message doesn't really explain why the change is needed.\n> Could you elaborate, maybe providing an example of incorrect behaviour\n> of a pipeline handler implementation with the existing calling\n> convention ?\n>\n>\nThe main issue that CrOS wants to solve is: We use acquire() & release()\nto handle the IPA (proxy & the sandboxed process)'s lifetime [1][2]. Within\nthe\ncurrent libcamera API, the IPA should be active when configure() is called,\nand therefore it doesn't make sense to terminate the IPA (and release some\nDMA buffers) when the camera is stopped.\nacquire() and release() are the best places to construct & destruct the IPA\nproxy & sandboxed process.\nThe reason that we want to destruct the IPA proxy at some point is that it's\nthe easiest way to clean up proprietary libraries' memory usage, which is\ndifferent from how ipu3 works now. ipu3 creates the proxy in match() and\nnever destructs it.\n\n[1]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290\n[2]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298\n\nHowever, IIUC, an IPA proxy needs to be constructed, used, and destructed\non the same thread. (Please correct me if I'm wrong.)\nDuring the development, we spent some time debugging this issue.\nI think ipu3's proxy doesn't run into such an issue because it uses InThread\nmode?\nTherefore, I think it'd be better to at least leave comments to remind\ndevelopers\nthat releaseDevice() (and the potential acquireDevice()) might be called\nfrom\nany thread.\n\nI've just tried to call releaseDevice() directly in release() without\nswitching the\nthread, and got a FATAL error:\n```\n\nFATAL default event_dispatcher_poll.cpp:285 assertion \"iter !=\nnotifiers_.end()\" failed in processNotifiers()\n```\n\nPlease also check if constructing the IPA proxy in acquireDevice() makes\nsense.\n\nThanks!\n\nBR,\nHarvey\n\n> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> >\n> > > It is better / more logical to call releaseDevice() before unlocking\n> > > the devices. ATM the only pipeline handler implementing releaseDevice()\n>\n> s/ATM/At the moment/\n>\n> > > is the rpi pipeline handler which releases buffers from its\n> releaseDevice()\n> > > implementation.\n> > >\n> > > Releasing buffers before unlocking the media devices is ok to do\n> > > and arguably it is better to release the buffers before unlocking.\n> > >\n> > > Signed-off-by: Hans de Goede \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > > ---\n> > > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > b/src/libcamera/pipeline_handler.cpp\n> > > index a20cd27d..1fc22d6a 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n> > >\n> > > ASSERT(useCount_);\n> > >\n> > > + releaseDevice(camera);\n> > > +\n> > > if (useCount_ == 1)\n> > > unlockMediaDevices();\n> > >\n> > > - releaseDevice(camera);\n> > > -\n> > > --useCount_;\n> > > }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 E518DBDE17\n\tfor ;\n\tSun, 25 Aug 2024 11:21:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C0EF633CF;\n\tSun, 25 Aug 2024 13:21:36 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 260FF63382\n\tfor ;\n\tSun, 25 Aug 2024 13:21:34 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id\n\t38308e7fff4ca-2f3f07ac2dcso39335701fa.2\n\tfor ;\n\tSun, 25 Aug 2024 04:21:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fjc6HP3J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724584893; x=1725189693;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=P9xERjVakHOl6fLq0x9nGXfWqemdEdVvL25f7OqWQkI=;\n\tb=fjc6HP3JFCDIJzt1Lkztlq9HrG1+8je5+IZ96pVn+c2CRoXGX8RFBz/L2z3f9yYCqL\n\tPQtkJJ6kHZK863Gy7YZmvGLPXsdV72zEIJN8FQTPlgQ8vqSmUESE3V8julqg432+oGds\n\t0qRVpe+MTwv4PyQJURlcHJhWZvwhYuQ6kBbv0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724584893; x=1725189693;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=P9xERjVakHOl6fLq0x9nGXfWqemdEdVvL25f7OqWQkI=;\n\tb=tDk+XyTq5q5pFBdnESNcowaNCRvpvWzL6VtqpIoQJZ78nWG94/ZUkTVggm+gzEJ1qc\n\t1Lk3CrOxelQbzPMM4eqi/jhrQxt5VkBbuWF+N/hNSrlNAvOjBDNx7vupnDNH9+amU95+\n\taRU7Xf9gVWIo556jMykEl+HbQHJXqlrRyXj9JJVcvddbd5XRZwmLDccXaxne3Vmiy4qb\n\twyXbf7drvXwHAeAzz+cFoVHefpmT3umetvrks93HKnmkGD5SzXMMlfe8GaCixO8Pt7X2\n\thlbriioNXtlT9HqUOXTl34CwHQAlhzjxgV2dZD9eX/elvTl0vhfQo2Op+xFBoLfo5tLs\n\tBCCw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW0Kbo8WupxsOcpfB9QCJv8WnwOWHx3hae1MTsx6YNwguVIVYZq9bVnMPWrEW1VokOIlItlcId56OfQeZ3YMJw=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxUgIZ2JQHFtl8rUiPsFGv5ZofmhronZGQ0pPIOMHG2YhG/epWm\n\txNjQDSsuRqueNVxv+AyvSovBoRVcFYOn8fh33AJ+jeSokiep60RiA3IsokH+a+jdFDdh14TpYhh\n\tj/lzhmq+RZpHUNFFM81TssdregG1sRg2JVawO","X-Google-Smtp-Source":"AGHT+IEWUR/7PHP4TKs6nx/+IF0JOOCZav9WfdXN1iTB7yp7I4NO+dyNpKff+H77BcChPj1oEdiV4olycBeaDmjpZDQ=","X-Received":"by 2002:a2e:a54b:0:b0:2f3:e42a:afc2 with SMTP id\n\t38308e7fff4ca-2f4f57b193amr51840331fa.49.1724584892659;\n\tSun, 25 Aug 2024 04:21:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>","In-Reply-To":"<20240825002712.GA17238@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Sun, 25 Aug 2024 13:21:21 +0200","Message-ID":"","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Laurent Pinchart ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Content-Type":"multipart/alternative; boundary=\"000000000000e99534062080355c\"","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":30901,"web_url":"https://patchwork.libcamera.org/comment/30901/","msgid":"","date":"2024-08-26T08:06:35","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 8/25/24 2:27 AM, Laurent Pinchart wrote:\n> On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n>> Hi Hans,\n>>\n>> Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n>>\n>> Reviewed-by: Harvey Yang \n>>\n>> One question that might not be related though:\n>> `PipelineHandler::release` is defined to be thread safe, while\n>> there's no such description for `PipelineHandler::releaseDevice`.\n>>\n>> In mtkisp7, we have a CL [1] to make sure\n>> `PipelineHandler::releaseDevice` is always blockingly executed\n>> on PipelineHandler's thread.\n>>\n>> Although it makes the normal cases (that it's returning true\n>> directly) run longer, as it waits for another thread's execution,\n>> it prevents PipelineHandler's mis-implementation.\n>> I'd suggest either we do the same in the upstream, or we add\n>> the description of being thread-safe for\n>> `PipelineHandler::releaseDevice` to remind the developers of\n>> pipeline handlers.\n> \n> I don't think releaseDevice() qualifies as thread safe. Here's how\n> libcamera documents thread safety:\n> \n> * - A **reentrant** function may be called simultaneously from multiple\n> * threads if and only if each invocation uses a different instance of the\n> * class. This is the default for all member functions not explictly marked\n> * otherwise.\n> *\n> * - \\anchor thread-safe A **thread-safe** function may be called\n> * simultaneously from multiple threads on the same instance of a class. A\n> * thread-safe function is thus reentrant. Thread-safe functions may also be\n> * called simultaneously with any other reentrant function of the same class\n> * on the same instance.\n> \n> Pipeline handlers shouldn't have to deal with releaseDevice() being\n> called concurrently on the same PipelineHandler instance for different\n> cameras, which is why the caller serializes the calls.\n> \n> releaseDevice() should never race with other pipeline handler calls for\n> the same camera. However, it could race with other calls for different\n> cameras. We should document this, or change the behaviour.\n\nreleaseDevice() is only called from: PipelineHandler::release() which starts with:\n\nvoid PipelineHandler::release(Camera *camera)\n{\n MutexLocker locker(lock_);\n\nso even if an app is releasing 2 cameras from the same pipeline-handler\nat the same time (from 2 different threads) then the releaseDevice() calls\nwill be serialized.\n\nShould I document for v2 that releaseDevice() and the new acquireDevice() calls\nare serialized by the PipelineHandler base class ?\n\nRegards,\n\nHans\n\n\n\n> \n>> WDYT?\n>>\n>> BR,\n>> Harvey\n>>\n>> [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n> \n> The commit message doesn't really explain why the change is needed.\n> Could you elaborate, maybe providing an example of incorrect behaviour\n> of a pipeline handler implementation with the existing calling\n> convention ?\n> \n>> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n>>\n>>> It is better / more logical to call releaseDevice() before unlocking\n>>> the devices. ATM the only pipeline handler implementing releaseDevice()\n> \n> s/ATM/At the moment/\n> \n>>> is the rpi pipeline handler which releases buffers from its releaseDevice()\n>>> implementation.\n>>>\n>>> Releasing buffers before unlocking the media devices is ok to do\n>>> and arguably it is better to release the buffers before unlocking.\n>>>\n>>> Signed-off-by: Hans de Goede \n> \n> Reviewed-by: Laurent Pinchart \n> \n>>> ---\n>>> src/libcamera/pipeline_handler.cpp | 4 ++--\n>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline_handler.cpp\n>>> b/src/libcamera/pipeline_handler.cpp\n>>> index a20cd27d..1fc22d6a 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n>>>\n>>> ASSERT(useCount_);\n>>>\n>>> + releaseDevice(camera);\n>>> +\n>>> if (useCount_ == 1)\n>>> unlockMediaDevices();\n>>>\n>>> - releaseDevice(camera);\n>>> -\n>>> --useCount_;\n>>> }\n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D6BE8C323E\n\tfor ;\n\tMon, 26 Aug 2024 08:06:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80E21633CF;\n\tMon, 26 Aug 2024 10:06:45 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00F9361906\n\tfor ;\n\tMon, 26 Aug 2024 10:06:42 +0200 (CEST)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-383-Yx9t1xyXM1iqxqq2pGkNtg-1; Mon, 26 Aug 2024 04:06:40 -0400","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5bedba9894dso3559137a12.1\n\tfor ;\n\tMon, 26 Aug 2024 01:06:38 -0700 (PDT)","from [10.40.98.157] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5c04a3ca4d2sm5309214a12.36.2024.08.26.01.06.35\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 26 Aug 2024 01:06:36 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"bpWWXK2D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1724659601;\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=tbIG/VCS96lTnldzx4wMFk49iX1DyFT95y8US+CxK+Y=;\n\tb=bpWWXK2DlKoyBpvYyk35XDm1SLlaBTOHI4hLdfqQ8L5xyk0fnn4TkCnQD0B8lroQgIyQWA\n\tfw8aTF1GJVta1BClQdxT4ZDimF+C0fQZZD310zZQ/0q/dJ23bAmGsMV1zqJG1jp5lkzfI2\n\tlfjr8nrQjPTYdOhPDGIqq9P9D9xIl5g=","X-MC-Unique":"Yx9t1xyXM1iqxqq2pGkNtg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724659598; x=1725264398;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tbIG/VCS96lTnldzx4wMFk49iX1DyFT95y8US+CxK+Y=;\n\tb=R/jrOTo8WqpI7sdxYUnyGovTOCikdj22Jp7K7jwMjepl/265ZAVntcziRkUHRrHpzO\n\tVE5ntqk80XhePDW/U1V1a04oRbcU/fAbWSg24SHGHmTWm6zRD0iecQQK7LLwZ3ptz+tl\n\t9DbvMQixopCWvFvHhirq+/ow1lSLptVtnqevZt3L/5XeOTd29t5KDeDOZy0GHS6iM0PU\n\tTfNr6l50N5ob9wwHHlk5X/SarTwX3GoQNLvQg2a0AJmsFxGH9a3OYv1LL5oMl0MtfiqM\n\t02iKGOW6FXrhNLQB/0zHAS+6VKA9Szv2m2E7yKkPYTrO2AzGr+NE8eE83rkJFOijNl/P\n\tGPQQ==","X-Gm-Message-State":"AOJu0YwV8as0VLfdGBMHaGwWAj/U7IjYqn58f+4KRqIWNOFR7Ug6vyBg\n\t4D1aAZW0MLiNXsGf0QMpPy/xg+if0vznMRIz3Pvab4puqRhUFLzhzq58ddC7aw6ouyLf3pGea/D\n\tiUEHjIoobBaoz7r0+/4PI8lkVvQEQoWIfN4Ca1tBM0KlnmVT+m7vDbHSdsj2HdoS0GX20JUQ=","X-Received":["by 2002:a05:6402:34ca:b0:5be:e94b:dc2b with SMTP id\n\t4fb4d7f45d1cf-5c08915adeamr6496414a12.8.1724659597517; \n\tMon, 26 Aug 2024 01:06:37 -0700 (PDT)","by 2002:a05:6402:34ca:b0:5be:e94b:dc2b with SMTP id\n\t4fb4d7f45d1cf-5c08915adeamr6496400a12.8.1724659597052; \n\tMon, 26 Aug 2024 01:06:37 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEZMRc1AZnhp4rWcuoNMTGwbDHLoxUCtCchFyirCz+nJ6CwK0ohIoPzJ9wsExWRhPwn53M4lA==","Message-ID":"","Date":"Mon, 26 Aug 2024 10:06:35 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Laurent Pinchart ,\n\tCheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240825002712.GA17238@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30902,"web_url":"https://patchwork.libcamera.org/comment/30902/","msgid":"<6c718299-b06d-46ca-8dfa-edb3b05e5946@redhat.com>","date":"2024-08-26T08:10:56","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 8/25/24 2:27 AM, Laurent Pinchart wrote:\n\n> The commit message doesn't really explain why the change is needed.\n> Could you elaborate, maybe providing an example of incorrect behaviour\n> of a pipeline handler implementation with the existing calling\n> convention ?\n\nThis change is mostly to make this function do the releaseDevice() +\nunlockMediaDevices() in opposite order of acquire() once acquireDevice()\nis added.\n\nFor acquireDevice() we clearly only want to call that after having\nsuccessfully locked all media devices.\n\nAnd then the logical (mirrored) approach on release() is to first\nundo the acquireDevice() and then unlock the media devices.\n\nI'll move it to be after the patch adding acquireDevice() and modify\nthe commit message.\n\nRegards,\n\nHans\n\n\n\n> \n>> On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n>>\n>>> It is better / more logical to call releaseDevice() before unlocking\n>>> the devices. ATM the only pipeline handler implementing releaseDevice()\n> \n> s/ATM/At the moment/\n> \n>>> is the rpi pipeline handler which releases buffers from its releaseDevice()\n>>> implementation.\n>>>\n>>> Releasing buffers before unlocking the media devices is ok to do\n>>> and arguably it is better to release the buffers before unlocking.\n>>>\n>>> Signed-off-by: Hans de Goede \n> \n> Reviewed-by: Laurent Pinchart \n> \n>>> ---\n>>> src/libcamera/pipeline_handler.cpp | 4 ++--\n>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline_handler.cpp\n>>> b/src/libcamera/pipeline_handler.cpp\n>>> index a20cd27d..1fc22d6a 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n>>>\n>>> ASSERT(useCount_);\n>>>\n>>> + releaseDevice(camera);\n>>> +\n>>> if (useCount_ == 1)\n>>> unlockMediaDevices();\n>>>\n>>> - releaseDevice(camera);\n>>> -\n>>> --useCount_;\n>>> }\n>>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50E7AC323E\n\tfor ;\n\tMon, 26 Aug 2024 08:11:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62AB3633CF;\n\tMon, 26 Aug 2024 10:11:06 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 15E2061906\n\tfor ;\n\tMon, 26 Aug 2024 10:11:03 +0200 (CEST)","from mail-ej1-f69.google.com (mail-ej1-f69.google.com\n\t[209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-18-xyxtRZ8EPPamQog6CnRHDw-1; Mon, 26 Aug 2024 04:10:59 -0400","by mail-ej1-f69.google.com with SMTP id\n\ta640c23a62f3a-a8677df5ba1so382947166b.0\n\tfor ;\n\tMon, 26 Aug 2024 01:10:59 -0700 (PDT)","from [10.40.98.157] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a868f4f4a67sm622860566b.214.2024.08.26.01.10.56\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 26 Aug 2024 01:10:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"O4i1yqFk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1724659862;\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=mlqfOJxds1giF7czj+QMRgnIdz+oFRYGnzr2TBxKCL0=;\n\tb=O4i1yqFkjYs4/znLccL2xb0MMtEtqEA6d2Sk1GQq18KT6JWKu0Y54zHzE7gjKMVjQwsi2v\n\td+In17Iz+GsWlabTJ6QbxpwVLI0VKyv+xjxoODNiGrnXGPZ6Bc8DQNPko2YOakVRLtGbGO\n\tuZI0Oj2Bhd4Aj+4ExwOfGTYp2Pwu0dY=","X-MC-Unique":"xyxtRZ8EPPamQog6CnRHDw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724659858; x=1725264658;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=mlqfOJxds1giF7czj+QMRgnIdz+oFRYGnzr2TBxKCL0=;\n\tb=jW6nCEuHW9T+r7W4OKlpmBfAn3zta3SYl1d3IHoey8OAkpqydDba0q121kOPSift2O\n\tg/k2Tn0q6QvHziQoQBIGCtwekpcD3q1Z0T57hxRIoRPvn8rBstzfJgafjqNMLw5zDuzp\n\tu4/0enzs1AWav3JYuCZFZCVpVM2Fymq7x4upR5mkbsLK7jYqSyO1Xur7hBD1GEAwrwrr\n\txUONURzkty4H90ULM25tYLLTGjxi9ro0fmaPKuJU/z0bbrLygGHfshGWJtFHFDaajt6t\n\t6VreaekF0z2ofjrMbK1oMLcbm8cn+ZpezXZ3ocRB6jrFwhgkAHhPy6sBcM0C5nhb4tcg\n\tjRDQ==","X-Gm-Message-State":"AOJu0YyEbNAj3j8l+oZ2FW8RI88U4gdF/eYXHPsTl+e70a6XqEzDUy3S\n\tJeiTZnVgVTNX8pAmqvkeT4ipt19KEo2ehS4z+vtD2xWUVvSppxL8/i5ZQ+B8MJCal4AZhNVFnP+\n\tLgBtnch+pF/6Q4bg0Vh72NYmgAWu+GHJ8I4HZ4wzwywHW7m5MYlwcPCeZWZ1blvtArLTNlI8=","X-Received":["by 2002:a17:907:2da2:b0:a86:a976:68b8 with SMTP id\n\ta640c23a62f3a-a86a9766beamr708641266b.65.1724659858108; \n\tMon, 26 Aug 2024 01:10:58 -0700 (PDT)","by 2002:a17:907:2da2:b0:a86:a976:68b8 with SMTP id\n\ta640c23a62f3a-a86a9766beamr708639266b.65.1724659857567; \n\tMon, 26 Aug 2024 01:10:57 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IErY7UslJ5m0N2b3U/y26xjGC5yLRxQPmwGXMgfS78FJ8eLPYvxlqrxLYrUt6wbnJfBNknalQ==","Message-ID":"<6c718299-b06d-46ca-8dfa-edb3b05e5946@redhat.com>","Date":"Mon, 26 Aug 2024 10:10:56 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Laurent Pinchart ,\n\tCheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tMilan Zamazal ","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>","From":"Hans de Goede ","In-Reply-To":"<20240825002712.GA17238@pendragon.ideasonboard.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30961,"web_url":"https://patchwork.libcamera.org/comment/30961/","msgid":"<20240829205854.GD15799@pendragon.ideasonboard.com>","date":"2024-08-29T20:58:54","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:\n> On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:\n> > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> > > Hi Hans,\n> > >\n> > > Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n> > >\n> > > Reviewed-by: Harvey Yang \n> > >\n> > > One question that might not be related though:\n> > > `PipelineHandler::release` is defined to be thread safe, while\n> > > there's no such description for `PipelineHandler::releaseDevice`.\n> > >\n> > > In mtkisp7, we have a CL [1] to make sure\n> > > `PipelineHandler::releaseDevice` is always blockingly executed\n> > > on PipelineHandler's thread.\n> > >\n> > > Although it makes the normal cases (that it's returning true\n> > > directly) run longer, as it waits for another thread's execution,\n> > > it prevents PipelineHandler's mis-implementation.\n> > > I'd suggest either we do the same in the upstream, or we add\n> > > the description of being thread-safe for\n> > > `PipelineHandler::releaseDevice` to remind the developers of\n> > > pipeline handlers.\n> >\n> > I don't think releaseDevice() qualifies as thread safe. Here's how\n> > libcamera documents thread safety:\n> >\n> > * - A **reentrant** function may be called simultaneously from multiple\n> > * threads if and only if each invocation uses a different instance of the\n> > * class. This is the default for all member functions not explictly marked\n> > * otherwise.\n> > *\n> > * - \\anchor thread-safe A **thread-safe** function may be called\n> > * simultaneously from multiple threads on the same instance of a class. A\n> > * thread-safe function is thus reentrant. Thread-safe functions may also be\n> > * called simultaneously with any other reentrant function of the same class\n> > * on the same instance.\n> >\n> > Pipeline handlers shouldn't have to deal with releaseDevice() being\n> > called concurrently on the same PipelineHandler instance for different\n> > cameras, which is why the caller serializes the calls.\n> >\n> > releaseDevice() should never race with other pipeline handler calls for\n> > the same camera. However, it could race with other calls for different\n> > cameras. We should document this, or change the behaviour.\n> \n> Yes, you're right. Thanks for the clarification.\n> \n> > > WDYT?\n> > >\n> > > BR,\n> > > Harvey\n> > >\n> > > [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n> >\n> > The commit message doesn't really explain why the change is needed.\n> > Could you elaborate, maybe providing an example of incorrect behaviour\n> > of a pipeline handler implementation with the existing calling\n> > convention ?\n> \n> The main issue that CrOS wants to solve is: We use acquire() & release()\n> to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2]. Within the\n> current libcamera API, the IPA should be active when configure() is called,\n> and therefore it doesn't make sense to terminate the IPA (and release some\n> DMA buffers) when the camera is stopped.\n> acquire() and release() are the best places to construct & destruct the IPA\n> proxy & sandboxed process.\n> The reason that we want to destruct the IPA proxy at some point is that it's\n> the easiest way to clean up proprietary libraries' memory usage, which is\n> different from how ipu3 works now. ipu3 creates the proxy in match() and\n> never destructs it.\n\nI'd argue that proprietary libraries are the problem here, but that's a\nlonger term problem.\n\n> [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290\n> [2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298\n> \n> However, IIUC, an IPA proxy needs to be constructed, used, and destructed\n> on the same thread. (Please correct me if I'm wrong.)\n\nThey are thread-bound, which means they indeed have to be used and\ndestroyed in the same thread. Construction can happen in another thread\nif the objects are then moved to the pipeline handler thread with\nmoveToThread(), but it's easier to just construct the objects in the\nsame thread.\n\n> During the development, we spent some time debugging this issue.\n> I think ipu3's proxy doesn't run into such an issue because it uses InThread mode?\n> Therefore, I think it'd be better to at least leave comments to remind developers\n> that releaseDevice() (and the potential acquireDevice()) might be called from\n> any thread.\n> \n> I've just tried to call releaseDevice() directly in release() without switching the\n> thread, and got a FATAL error:\n> ```\n> \n> FATAL default event_dispatcher_poll.cpp:285 assertion \"iter !=\n> notifiers_.end()\" failed in processNotifiers()\n> ```\n> \n> Please also check if constructing the IPA proxy in acquireDevice() makes\n> sense.\n\nIn the context of the problem you mention above, yes, it does make sense\n(until we can stop dealing with memory leaks from closed-source blobs).\nHowever, it introduces a problem. The IPA module is responsible for\nadvertising the controls it supports, and applications can expect from\nthe current API to list those controls before acquiring a camera. That's\na conflicting requirement, how do you think we should address it ?\n\n> > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> > >\n> > > > It is better / more logical to call releaseDevice() before unlocking\n> > > > the devices. ATM the only pipeline handler implementing releaseDevice()\n> >\n> > s/ATM/At the moment/\n> >\n> > > > is the rpi pipeline handler which releases buffers from its releaseDevice()\n> > > > implementation.\n> > > >\n> > > > Releasing buffers before unlocking the media devices is ok to do\n> > > > and arguably it is better to release the buffers before unlocking.\n> > > >\n> > > > Signed-off-by: Hans de Goede \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > > ---\n> > > > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > > b/src/libcamera/pipeline_handler.cpp\n> > > > index a20cd27d..1fc22d6a 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n> > > >\n> > > > ASSERT(useCount_);\n> > > >\n> > > > + releaseDevice(camera);\n> > > > +\n> > > > if (useCount_ == 1)\n> > > > unlockMediaDevices();\n> > > >\n> > > > - releaseDevice(camera);\n> > > > -\n> > > > --useCount_;\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 12A63C323E\n\tfor ;\n\tThu, 29 Aug 2024 20:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0728363461;\n\tThu, 29 Aug 2024 22:59:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 450A963456\n\tfor ;\n\tThu, 29 Aug 2024 22:59:25 +0200 (CEST)","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 5FA16226;\n\tThu, 29 Aug 2024 22:58:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OeLV9B4Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724965096;\n\tbh=9EUZkITLj2NSCFehS4lsw5llxxD4v2Kaw7HU+U/ofBk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OeLV9B4YZK8kRZojEIfVRJSvl8D89xh/rcQ5pGJGLyZYhfKPNgz9I6CnWvO91xQUE\n\tzhFfFsdfmFVnJFWRfE8X10IrPB49R7wqwAhhGNe8VAZ89TpBMNWwc0p1hlOn8plZYS\n\t0wXV+jYrtOEvpi7s5AYh3Tjy1Xbr4Oqqna0pxvSs=","Date":"Thu, 29 Aug 2024 23:58:54 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","Message-ID":"<20240829205854.GD15799@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":30965,"web_url":"https://patchwork.libcamera.org/comment/30965/","msgid":"","date":"2024-08-29T21:34:13","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:\n> > On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:\n> > > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> > > > Hi Hans,\n> > > >\n> > > > Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n> > > >\n> > > > Reviewed-by: Harvey Yang \n> > > >\n> > > > One question that might not be related though:\n> > > > `PipelineHandler::release` is defined to be thread safe, while\n> > > > there's no such description for `PipelineHandler::releaseDevice`.\n> > > >\n> > > > In mtkisp7, we have a CL [1] to make sure\n> > > > `PipelineHandler::releaseDevice` is always blockingly executed\n> > > > on PipelineHandler's thread.\n> > > >\n> > > > Although it makes the normal cases (that it's returning true\n> > > > directly) run longer, as it waits for another thread's execution,\n> > > > it prevents PipelineHandler's mis-implementation.\n> > > > I'd suggest either we do the same in the upstream, or we add\n> > > > the description of being thread-safe for\n> > > > `PipelineHandler::releaseDevice` to remind the developers of\n> > > > pipeline handlers.\n> > >\n> > > I don't think releaseDevice() qualifies as thread safe. Here's how\n> > > libcamera documents thread safety:\n> > >\n> > > * - A **reentrant** function may be called simultaneously from\n> multiple\n> > > * threads if and only if each invocation uses a different instance\n> of the\n> > > * class. This is the default for all member functions not explictly\n> marked\n> > > * otherwise.\n> > > *\n> > > * - \\anchor thread-safe A **thread-safe** function may be called\n> > > * simultaneously from multiple threads on the same instance of a\n> class. A\n> > > * thread-safe function is thus reentrant. Thread-safe functions may\n> also be\n> > > * called simultaneously with any other reentrant function of the\n> same class\n> > > * on the same instance.\n> > >\n> > > Pipeline handlers shouldn't have to deal with releaseDevice() being\n> > > called concurrently on the same PipelineHandler instance for different\n> > > cameras, which is why the caller serializes the calls.\n> > >\n> > > releaseDevice() should never race with other pipeline handler calls for\n> > > the same camera. However, it could race with other calls for different\n> > > cameras. We should document this, or change the behaviour.\n> >\n> > Yes, you're right. Thanks for the clarification.\n> >\n> > > > WDYT?\n> > > >\n> > > > BR,\n> > > > Harvey\n> > > >\n> > > > [1]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n> > >\n> > > The commit message doesn't really explain why the change is needed.\n> > > Could you elaborate, maybe providing an example of incorrect behaviour\n> > > of a pipeline handler implementation with the existing calling\n> > > convention ?\n> >\n> > The main issue that CrOS wants to solve is: We use acquire() & release()\n> > to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2].\n> Within the\n> > current libcamera API, the IPA should be active when configure() is\n> called,\n> > and therefore it doesn't make sense to terminate the IPA (and release\n> some\n> > DMA buffers) when the camera is stopped.\n> > acquire() and release() are the best places to construct & destruct the\n> IPA\n> > proxy & sandboxed process.\n> > The reason that we want to destruct the IPA proxy at some point is that\n> it's\n> > the easiest way to clean up proprietary libraries' memory usage, which is\n> > different from how ipu3 works now. ipu3 creates the proxy in match() and\n> > never destructs it.\n>\n> I'd argue that proprietary libraries are the problem here, but that's a\n> longer term problem.\n>\n> > [1]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290\n> > [2]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298\n> >\n> > However, IIUC, an IPA proxy needs to be constructed, used, and destructed\n> > on the same thread. (Please correct me if I'm wrong.)\n>\n> They are thread-bound, which means they indeed have to be used and\n> destroyed in the same thread. Construction can happen in another thread\n> if the objects are then moved to the pipeline handler thread with\n> moveToThread(), but it's easier to just construct the objects in the\n> same thread.\n>\n> > During the development, we spent some time debugging this issue.\n> > I think ipu3's proxy doesn't run into such an issue because it uses\n> InThread mode?\n>\n\nJust realized that ipu3's proxy is created in `PipelineHandler::match`,\nwhich is called on the pipeline handler thread, and it's never destructed.\n\n\n> > Therefore, I think it'd be better to at least leave comments to remind\n> developers\n> > that releaseDevice() (and the potential acquireDevice()) might be called\n> from\n> > any thread.\n> >\n> > I've just tried to call releaseDevice() directly in release() without\n> switching the\n> > thread, and got a FATAL error:\n> > ```\n> >\n> > FATAL default event_dispatcher_poll.cpp:285 assertion \"iter !=\n> > notifiers_.end()\" failed in processNotifiers()\n> > ```\n> >\n> > Please also check if constructing the IPA proxy in acquireDevice() makes\n> > sense.\n>\n> In the context of the problem you mention above, yes, it does make sense\n> (until we can stop dealing with memory leaks from closed-source blobs).\n> However, it introduces a problem. The IPA module is responsible for\n> advertising the controls it supports, and applications can expect from\n> the current API to list those controls before acquiring a camera. That's\n> a conflicting requirement, how do you think we should address it ?\n>\n\nRight. I think the easiest way is to construct the IPA proxy during\n`PipelineHandler::match`, where a camera is registered, and choose to\ndestruct it right after using it to list controls.\n\nIt can also be kept there, if proprietary libraries don't consume lots of\nmemory just to list controls before starting to process statistics. The\n`acquireDevice()` would need to check if an IPA proxy already exists\nor not though.\n\n\n> > > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> > > >\n> > > > > It is better / more logical to call releaseDevice() before\n> unlocking\n> > > > > the devices. ATM the only pipeline handler implementing\n> releaseDevice()\n> > >\n> > > s/ATM/At the moment/\n> > >\n> > > > > is the rpi pipeline handler which releases buffers from its\n> releaseDevice()\n> > > > > implementation.\n> > > > >\n> > > > > Releasing buffers before unlocking the media devices is ok to do\n> > > > > and arguably it is better to release the buffers before unlocking.\n> > > > >\n> > > > > Signed-off-by: Hans de Goede \n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > > ---\n> > > > > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > > > b/src/libcamera/pipeline_handler.cpp\n> > > > > index a20cd27d..1fc22d6a 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n> > > > >\n> > > > > ASSERT(useCount_);\n> > > > >\n> > > > > + releaseDevice(camera);\n> > > > > +\n> > > > > if (useCount_ == 1)\n> > > > > unlockMediaDevices();\n> > > > >\n> > > > > - releaseDevice(camera);\n> > > > > -\n> > > > > --useCount_;\n> > > > > }\n> > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nBR,\nHarvey","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 11704C323E\n\tfor ;\n\tThu, 29 Aug 2024 21:34:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE8A063456;\n\tThu, 29 Aug 2024 23:34:28 +0200 (CEST)","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 0D5C763456\n\tfor ;\n\tThu, 29 Aug 2024 23:34:26 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2f3e071eb64so14657121fa.1\n\tfor ;\n\tThu, 29 Aug 2024 14:34:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nfcgHILK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724967265; x=1725572065;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=hL34UXAk3Pw5VDYkgR+Xb8dZEgsSamdKi2/HEvP0pW0=;\n\tb=nfcgHILKpJ7BWAo4RGUtL22vsXkNkfsL92rzy1Il9ntZHAD56TlYf7ACak4MU2aDY/\n\tvmvZ7VLEDOQfHr7nCTNJn6nO+hVlTO5qYXTY96TGYgNSM6INk3jgUrjoMFcoyzzZGEgi\n\tqoBP8vAw/bpkoith9ReyDj4kWo37KEPgeeDxI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724967265; x=1725572065;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=hL34UXAk3Pw5VDYkgR+Xb8dZEgsSamdKi2/HEvP0pW0=;\n\tb=r0Y1amjVaA5uyIcAK8CEzvJmUiLtiM5oQFOYw1hct2CIP4SlDXv5Pw2C7+CnaTWlRt\n\tTDiz/EuLryYrpTDuKe+CaKnTj9jtYmIKFTT6lDTtX4ZUFxY5Bdcf23imH8CeMnd4tjEt\n\tU5vhXHFNrdl8N0jfXDHrL15XOATkAy9xvwONbYBjhfZWHjN6aZSiDcJEMxr+OhQYCIz+\n\tBlnKpL3p/eFPU0ocNgNkEMcQ1iJcDHKp9DG+FD4EI6UV9E8v12OtNktB57bHMqvVuEwH\n\tx+4l/B6tQ4qHi1/ztJoKXMQNGhtjzVyYGAAJgrA07mq/IngzjqlRPAUZHakOu+p1jfCv\n\ttqEg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCW8W/phxDV4H0+kyoOG9SJEhqT2Jzh9HPPvvfP5V6y1DaWjlBJTW/mj+Co2Ec5RivxJfmYxNvn9u2iYvzr0pyk=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzrEhlcvMn7g8s6MY7636LFewikFUSr0iojZWbfywx9UAgrfOa5\n\tqgbeaIcW2FSAftyHlSdypuHhIFeHF3cs/8y6srujJoXskGweB7plz59EqsEtL6CUF9tOgkr9zwn\n\tL9jlm2qp80BGx/q4Y6JmCcz0lK9wPtYfMoRdq","X-Google-Smtp-Source":"AGHT+IFbE0DpB4sQfSjZuUcpyJ1V/bt0kQNkpez3QSr4ESbT20f7trQe0WIG9D8kWloN2N/LNuBOpWlH9cgOsoWdC4o=","X-Received":"by 2002:a2e:b8d6:0:b0:2f1:750d:53a7 with SMTP id\n\t38308e7fff4ca-2f6105c4993mr37583791fa.8.1724967265088;\n\tThu, 29 Aug 2024 14:34:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>\n\t\n\t<20240829205854.GD15799@pendragon.ideasonboard.com>","In-Reply-To":"<20240829205854.GD15799@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Thu, 29 Aug 2024 23:34:13 +0200","Message-ID":"","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Laurent Pinchart ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Content-Type":"multipart/alternative; boundary=\"00000000000015cc960620d93dba\"","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":30969,"web_url":"https://patchwork.libcamera.org/comment/30969/","msgid":"<20240829220643.GJ15799@pendragon.ideasonboard.com>","date":"2024-08-29T22:06:43","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 29, 2024 at 11:34:13PM +0200, Cheng-Hao Yang wrote:\n> On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart wrote:\n> > On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:\n> > > On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:\n> > > > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> > > > > Hi Hans,\n> > > > >\n> > > > > Thanks for the patch. I've tested it on mtkisp7, and it works fine.\n> > > > >\n> > > > > Reviewed-by: Harvey Yang \n> > > > >\n> > > > > One question that might not be related though:\n> > > > > `PipelineHandler::release` is defined to be thread safe, while\n> > > > > there's no such description for `PipelineHandler::releaseDevice`.\n> > > > >\n> > > > > In mtkisp7, we have a CL [1] to make sure\n> > > > > `PipelineHandler::releaseDevice` is always blockingly executed\n> > > > > on PipelineHandler's thread.\n> > > > >\n> > > > > Although it makes the normal cases (that it's returning true\n> > > > > directly) run longer, as it waits for another thread's execution,\n> > > > > it prevents PipelineHandler's mis-implementation.\n> > > > > I'd suggest either we do the same in the upstream, or we add\n> > > > > the description of being thread-safe for\n> > > > > `PipelineHandler::releaseDevice` to remind the developers of\n> > > > > pipeline handlers.\n> > > >\n> > > > I don't think releaseDevice() qualifies as thread safe. Here's how\n> > > > libcamera documents thread safety:\n> > > >\n> > > > * - A **reentrant** function may be called simultaneously from multiple\n> > > > * threads if and only if each invocation uses a different instance of the\n> > > > * class. This is the default for all member functions not explictly marked\n> > > > * otherwise.\n> > > > *\n> > > > * - \\anchor thread-safe A **thread-safe** function may be called\n> > > > * simultaneously from multiple threads on the same instance of a class. A\n> > > > * thread-safe function is thus reentrant. Thread-safe functions may also be\n> > > > * called simultaneously with any other reentrant function of the same class\n> > > > * on the same instance.\n> > > >\n> > > > Pipeline handlers shouldn't have to deal with releaseDevice() being\n> > > > called concurrently on the same PipelineHandler instance for different\n> > > > cameras, which is why the caller serializes the calls.\n> > > >\n> > > > releaseDevice() should never race with other pipeline handler calls for\n> > > > the same camera. However, it could race with other calls for different\n> > > > cameras. We should document this, or change the behaviour.\n> > >\n> > > Yes, you're right. Thanks for the clarification.\n> > >\n> > > > > WDYT?\n> > > > >\n> > > > > BR,\n> > > > > Harvey\n> > > > >\n> > > > > [1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n> > > >\n> > > > The commit message doesn't really explain why the change is needed.\n> > > > Could you elaborate, maybe providing an example of incorrect behaviour\n> > > > of a pipeline handler implementation with the existing calling\n> > > > convention ?\n> > >\n> > > The main issue that CrOS wants to solve is: We use acquire() & release()\n> > > to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2]. Within the\n> > > current libcamera API, the IPA should be active when configure() is called,\n> > > and therefore it doesn't make sense to terminate the IPA (and release some\n> > > DMA buffers) when the camera is stopped.\n> > > acquire() and release() are the best places to construct & destruct the IPA\n> > > proxy & sandboxed process.\n> > > The reason that we want to destruct the IPA proxy at some point is that it's\n> > > the easiest way to clean up proprietary libraries' memory usage, which is\n> > > different from how ipu3 works now. ipu3 creates the proxy in match() and\n> > > never destructs it.\n> >\n> > I'd argue that proprietary libraries are the problem here, but that's a\n> > longer term problem.\n> >\n> > > [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290\n> > > [2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298\n> > >\n> > > However, IIUC, an IPA proxy needs to be constructed, used, and destructed\n> > > on the same thread. (Please correct me if I'm wrong.)\n> >\n> > They are thread-bound, which means they indeed have to be used and\n> > destroyed in the same thread. Construction can happen in another thread\n> > if the objects are then moved to the pipeline handler thread with\n> > moveToThread(), but it's easier to just construct the objects in the\n> > same thread.\n> >\n> > > During the development, we spent some time debugging this issue.\n> > > I think ipu3's proxy doesn't run into such an issue because it uses\n> > InThread mode?\n> \n> Just realized that ipu3's proxy is created in `PipelineHandler::match`,\n> which is called on the pipeline handler thread, and it's never destructed.\n\nYes that's the reason. Sorry, I forgot to reply to your question.\n\n> > > Therefore, I think it'd be better to at least leave comments to remind developers\n> > > that releaseDevice() (and the potential acquireDevice()) might be called from\n> > > any thread.\n> > >\n> > > I've just tried to call releaseDevice() directly in release() without switching the\n> > > thread, and got a FATAL error:\n> > > ```\n> > >\n> > > FATAL default event_dispatcher_poll.cpp:285 assertion \"iter !=\n> > > notifiers_.end()\" failed in processNotifiers()\n> > > ```\n> > >\n> > > Please also check if constructing the IPA proxy in acquireDevice() makes\n> > > sense.\n> >\n> > In the context of the problem you mention above, yes, it does make sense\n> > (until we can stop dealing with memory leaks from closed-source blobs).\n> > However, it introduces a problem. The IPA module is responsible for\n> > advertising the controls it supports, and applications can expect from\n> > the current API to list those controls before acquiring a camera. That's\n> > a conflicting requirement, how do you think we should address it ?\n> \n> Right. I think the easiest way is to construct the IPA proxy during\n> `PipelineHandler::match`, where a camera is registered, and choose to\n> destruct it right after using it to list controls.\n> \n> It can also be kept there, if proprietary libraries don't consume lots of\n> memory just to list controls before starting to process statistics. The\n> `acquireDevice()` would need to check if an IPA proxy already exists\n> or not though.\n\nThose could be options, yes. Neither sound optimal, but I'm not sure if\nthere's a better solution. Let's sleep over this. I'd also be happy to\nforget about the problem, but I'm sure you'll remind me with patches :-)\n\n> > > > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> > > > >\n> > > > > > It is better / more logical to call releaseDevice() before unlocking\n> > > > > > the devices. ATM the only pipeline handler implementing releaseDevice()\n> > > >\n> > > > s/ATM/At the moment/\n> > > >\n> > > > > > is the rpi pipeline handler which releases buffers from its releaseDevice()\n> > > > > > implementation.\n> > > > > >\n> > > > > > Releasing buffers before unlocking the media devices is ok to do\n> > > > > > and arguably it is better to release the buffers before unlocking.\n> > > > > >\n> > > > > > Signed-off-by: Hans de Goede \n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > >\n> > > > > > ---\n> > > > > > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > > > > b/src/libcamera/pipeline_handler.cpp\n> > > > > > index a20cd27d..1fc22d6a 100644\n> > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera *camera)\n> > > > > >\n> > > > > > ASSERT(useCount_);\n> > > > > >\n> > > > > > + releaseDevice(camera);\n> > > > > > +\n> > > > > > if (useCount_ == 1)\n> > > > > > unlockMediaDevices();\n> > > > > >\n> > > > > > - releaseDevice(camera);\n> > > > > > -\n> > > > > > --useCount_;\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 92A92C324C\n\tfor ;\n\tThu, 29 Aug 2024 22:07:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47DBF63458;\n\tFri, 30 Aug 2024 00:07:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF3586002E\n\tfor ;\n\tFri, 30 Aug 2024 00:07:13 +0200 (CEST)","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 EF7DC524;\n\tFri, 30 Aug 2024 00:06:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"saJVrZ6k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724969165;\n\tbh=XVULXp/5BusOxaZ+P/f3qZ17Y/Lp5xryD12ZFY9TEhU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=saJVrZ6kp6qZWtP+jW9h0yhmUPporBru+Z5RiLX2GtdHOLwC0061KHxYrGFl7HPWr\n\tAtCzBs9iTPeZC8mBAEN+tO0XCJNpaIhF9o/H1TpBKvRYL4x51gyItjlkHJFVFEg8pB\n\tkNijkSGARJU7oMHq2QV/Wwm7KCAZ8E0yaLqoofnI=","Date":"Fri, 30 Aug 2024 01:06:43 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","Message-ID":"<20240829220643.GJ15799@pendragon.ideasonboard.com>","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>\n\t\n\t<20240829205854.GD15799@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":30990,"web_url":"https://patchwork.libcamera.org/comment/30990/","msgid":"","date":"2024-08-30T11:45:49","subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Aug 30, 2024 at 12:07 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Thu, Aug 29, 2024 at 11:34:13PM +0200, Cheng-Hao Yang wrote:\n> > On Thu, Aug 29, 2024 at 10:59 PM Laurent Pinchart wrote:\n> > > On Sun, Aug 25, 2024 at 01:21:21PM +0200, Cheng-Hao Yang wrote:\n> > > > On Sun, Aug 25, 2024 at 2:27 AM Laurent Pinchart wrote:\n> > > > > On Wed, Aug 21, 2024 at 06:34:17PM +0200, Cheng-Hao Yang wrote:\n> > > > > > Hi Hans,\n> > > > > >\n> > > > > > Thanks for the patch. I've tested it on mtkisp7, and it works\n> fine.\n> > > > > >\n> > > > > > Reviewed-by: Harvey Yang \n> > > > > >\n> > > > > > One question that might not be related though:\n> > > > > > `PipelineHandler::release` is defined to be thread safe, while\n> > > > > > there's no such description for `PipelineHandler::releaseDevice`.\n> > > > > >\n> > > > > > In mtkisp7, we have a CL [1] to make sure\n> > > > > > `PipelineHandler::releaseDevice` is always blockingly executed\n> > > > > > on PipelineHandler's thread.\n> > > > > >\n> > > > > > Although it makes the normal cases (that it's returning true\n> > > > > > directly) run longer, as it waits for another thread's execution,\n> > > > > > it prevents PipelineHandler's mis-implementation.\n> > > > > > I'd suggest either we do the same in the upstream, or we add\n> > > > > > the description of being thread-safe for\n> > > > > > `PipelineHandler::releaseDevice` to remind the developers of\n> > > > > > pipeline handlers.\n> > > > >\n> > > > > I don't think releaseDevice() qualifies as thread safe. Here's how\n> > > > > libcamera documents thread safety:\n> > > > >\n> > > > > * - A **reentrant** function may be called simultaneously from\n> multiple\n> > > > > * threads if and only if each invocation uses a different\n> instance of the\n> > > > > * class. This is the default for all member functions not\n> explictly marked\n> > > > > * otherwise.\n> > > > > *\n> > > > > * - \\anchor thread-safe A **thread-safe** function may be called\n> > > > > * simultaneously from multiple threads on the same instance of\n> a class. A\n> > > > > * thread-safe function is thus reentrant. Thread-safe functions\n> may also be\n> > > > > * called simultaneously with any other reentrant function of\n> the same class\n> > > > > * on the same instance.\n> > > > >\n> > > > > Pipeline handlers shouldn't have to deal with releaseDevice() being\n> > > > > called concurrently on the same PipelineHandler instance for\n> different\n> > > > > cameras, which is why the caller serializes the calls.\n> > > > >\n> > > > > releaseDevice() should never race with other pipeline handler\n> calls for\n> > > > > the same camera. However, it could race with other calls for\n> different\n> > > > > cameras. We should document this, or change the behaviour.\n> > > >\n> > > > Yes, you're right. Thanks for the clarification.\n> > > >\n> > > > > > WDYT?\n> > > > > >\n> > > > > > BR,\n> > > > > > Harvey\n> > > > > >\n> > > > > > [1]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5528925\n> > > > >\n> > > > > The commit message doesn't really explain why the change is needed.\n> > > > > Could you elaborate, maybe providing an example of incorrect\n> behaviour\n> > > > > of a pipeline handler implementation with the existing calling\n> > > > > convention ?\n> > > >\n> > > > The main issue that CrOS wants to solve is: We use acquire() &\n> release()\n> > > > to handle the IPA (proxy & the sandboxed process)'s lifetime [1][2].\n> Within the\n> > > > current libcamera API, the IPA should be active when configure() is\n> called,\n> > > > and therefore it doesn't make sense to terminate the IPA (and\n> release some\n> > > > DMA buffers) when the camera is stopped.\n> > > > acquire() and release() are the best places to construct & destruct\n> the IPA\n> > > > proxy & sandboxed process.\n> > > > The reason that we want to destruct the IPA proxy at some point is\n> that it's\n> > > > the easiest way to clean up proprietary libraries' memory usage,\n> which is\n> > > > different from how ipu3 works now. ipu3 creates the proxy in match()\n> and\n> > > > never destructs it.\n> > >\n> > > I'd argue that proprietary libraries are the problem here, but that's a\n> > > longer term problem.\n> > >\n> > > > [1]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1290\n> > > > [2]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=1298\n> > > >\n> > > > However, IIUC, an IPA proxy needs to be constructed, used, and\n> destructed\n> > > > on the same thread. (Please correct me if I'm wrong.)\n> > >\n> > > They are thread-bound, which means they indeed have to be used and\n> > > destroyed in the same thread. Construction can happen in another thread\n> > > if the objects are then moved to the pipeline handler thread with\n> > > moveToThread(), but it's easier to just construct the objects in the\n> > > same thread.\n> > >\n> > > > During the development, we spent some time debugging this issue.\n> > > > I think ipu3's proxy doesn't run into such an issue because it uses\n> > > InThread mode?\n> >\n> > Just realized that ipu3's proxy is created in `PipelineHandler::match`,\n> > which is called on the pipeline handler thread, and it's never\n> destructed.\n>\n> Yes that's the reason. Sorry, I forgot to reply to your question.\n>\n\nNo worries. Thanks for the confirmation.\n\n\n>\n> > > > Therefore, I think it'd be better to at least leave comments to\n> remind developers\n> > > > that releaseDevice() (and the potential acquireDevice()) might be\n> called from\n> > > > any thread.\n> > > >\n> > > > I've just tried to call releaseDevice() directly in release()\n> without switching the\n> > > > thread, and got a FATAL error:\n> > > > ```\n> > > >\n> > > > FATAL default event_dispatcher_poll.cpp:285 assertion \"iter !=\n> > > > notifiers_.end()\" failed in processNotifiers()\n> > > > ```\n> > > >\n> > > > Please also check if constructing the IPA proxy in acquireDevice()\n> makes\n> > > > sense.\n> > >\n> > > In the context of the problem you mention above, yes, it does make\n> sense\n> > > (until we can stop dealing with memory leaks from closed-source blobs).\n> > > However, it introduces a problem. The IPA module is responsible for\n> > > advertising the controls it supports, and applications can expect from\n> > > the current API to list those controls before acquiring a camera.\n> That's\n> > > a conflicting requirement, how do you think we should address it ?\n> >\n> > Right. I think the easiest way is to construct the IPA proxy during\n> > `PipelineHandler::match`, where a camera is registered, and choose to\n> > destruct it right after using it to list controls.\n> >\n> > It can also be kept there, if proprietary libraries don't consume lots of\n> > memory just to list controls before starting to process statistics. The\n> > `acquireDevice()` would need to check if an IPA proxy already exists\n> > or not though.\n>\n> Those could be options, yes. Neither sound optimal, but I'm not sure if\n> there's a better solution. Let's sleep over this. I'd also be happy to\n> forget about the problem, but I'm sure you'll remind me with patches :-)\n>\n\nActually, I might not :)\nIn mtkisp7, the proprietary libraries don't provide the control list, and\nall the\ncontrols are hard-coded in the match() function. I don't think Intel's\nproprietary\nlibraries provide the info either.\nLet's see though haha...\n\nBR,\nHarvey\n\n\n>\n> > > > > On Tue, Aug 20, 2024 at 9:50 PM Hans de Goede wrote:\n> > > > > >\n> > > > > > > It is better / more logical to call releaseDevice() before\n> unlocking\n> > > > > > > the devices. ATM the only pipeline handler implementing\n> releaseDevice()\n> > > > >\n> > > > > s/ATM/At the moment/\n> > > > >\n> > > > > > > is the rpi pipeline handler which releases buffers from its\n> releaseDevice()\n> > > > > > > implementation.\n> > > > > > >\n> > > > > > > Releasing buffers before unlocking the media devices is ok to\n> do\n> > > > > > > and arguably it is better to release the buffers before\n> unlocking.\n> > > > > > >\n> > > > > > > Signed-off-by: Hans de Goede \n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > >\n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline_handler.cpp | 4 ++--\n> > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp\n> > > > > > > b/src/libcamera/pipeline_handler.cpp\n> > > > > > > index a20cd27d..1fc22d6a 100644\n> > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > @@ -205,11 +205,11 @@ void PipelineHandler::release(Camera\n> *camera)\n> > > > > > >\n> > > > > > > ASSERT(useCount_);\n> > > > > > >\n> > > > > > > + releaseDevice(camera);\n> > > > > > > +\n> > > > > > > if (useCount_ == 1)\n> > > > > > > unlockMediaDevices();\n> > > > > > >\n> > > > > > > - releaseDevice(camera);\n> > > > > > > -\n> > > > > > > --useCount_;\n> > > > > > > }\n> > > > > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 E6B20C324C\n\tfor ;\n\tFri, 30 Aug 2024 11:46:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8C7363466;\n\tFri, 30 Aug 2024 13:46:02 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2076A61E4A\n\tfor ;\n\tFri, 30 Aug 2024 13:46:02 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id\n\t38308e7fff4ca-2f3f90295a9so19437491fa.0\n\tfor ;\n\tFri, 30 Aug 2024 04:46:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"WjRvTUG+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725018361; x=1725623161;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=DJRzK5JftXD+d/H/PE+5heJAcXhFQyhp/dYnBta2YVs=;\n\tb=WjRvTUG+unxgeM3g0Li0SKqdEqrj3CXZTPPy1RUG+4BT0Ylhk7rmGf6EMVSMRXSVTv\n\tksg/TkVUVbxjh5+VyrvlGZMeCajwg5WVqvEbVltPJXEVHeNPnvaaSpAcQg3hyGWhiLK2\n\tnNvWaldlUTOi6OtSOMwlFSZ8R+gmMALpDe2Xo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725018361; x=1725623161;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=DJRzK5JftXD+d/H/PE+5heJAcXhFQyhp/dYnBta2YVs=;\n\tb=NWcl2N5ToOh08vz70DbZFnwA++oEuvAM+f1OTOSEN80rcIavtoAGsfnivHnpuzz3/H\n\tzByR67dlju6TrSlqkFzqXQlzMD8nQRhPygu+I+qf4ia2TgGKklbwTXVTS5Ps2MdPKO4I\n\t5H4jEkTSozFGkQoVf36lbTfKefI+DoRtwXlXopXErllxP2/9N1Ew0bd+uKYMegUpST5B\n\tqUBdeEaBiosPYUqd9WjoaIw2YvODiCcGimL6I6ixI/Q9WMznBnnoR/9vI3NuqdjV+Z8B\n\tYVBLcHKv5qaGLNU31NtemlbcJ6e9v4BWyARBRkwtd+IGTBPKt1xLhv/yHsPE/sPG9/qz\n\tbBkw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUYO0zJ8ZY2A8hx9WX1NYyGCLVeqSEmOLleqKLhxexSopxcXSHWRtGxkFvMXsuPWrzTwdAHRLNhetVzaf+WgAg=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyoSP/7iUBQ1Sy0Ob8MFeZ3X2UhU0AnQO52xiZdW2//atB1YBmk\n\tgX1XgIyMOLfQpsdtnVTnPJ99Gk9RSlMrxmAMVUy4qBMHngmMJaOowYFRdMik+ctscwVu7SgdAep\n\tTg8x3jpsBUeiQlIMTH7T4Y7tgFiaS6+UlmrDlbTV85fI5HDCqjZjp","X-Google-Smtp-Source":"AGHT+IG2yB0cb2WmDM18Ck0BJ9RJmIzzd3feXjGPiQQlnmUUGE02WvpmaWbXaXvN9DUyqS//ESQw3G0nmVMf5UOYvL0=","X-Received":"by 2002:a2e:f11:0:b0:2f3:f441:af24 with SMTP id\n\t38308e7fff4ca-2f6103c8726mr31843421fa.26.1725018360795;\n\tFri, 30 Aug 2024 04:46:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20240820195016.16028-1-hdegoede@redhat.com>\n\t<20240820195016.16028-3-hdegoede@redhat.com>\n\t\n\t<20240825002712.GA17238@pendragon.ideasonboard.com>\n\t\n\t<20240829205854.GD15799@pendragon.ideasonboard.com>\n\t\n\t<20240829220643.GJ15799@pendragon.ideasonboard.com>","In-Reply-To":"<20240829220643.GJ15799@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Fri, 30 Aug 2024 13:45:49 +0200","Message-ID":"","Subject":"Re: [PATCH 2/5] pipeline_handler: Call releaseDevice() before\n\tunlocking media devices","To":"Laurent Pinchart ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Milan Zamazal ","Content-Type":"multipart/alternative; boundary=\"000000000000a07ebc0620e522ec\"","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\" "}}]