[{"id":24318,"web_url":"https://patchwork.libcamera.org/comment/24318/","msgid":"","date":"2022-08-03T05:52:01","subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn Tue, Aug 02, 2022 at 10:29:36AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Harvey Yang \n> \n> As we hardly have use cases/applications that need both cameras at the\n> same time, this patch adds a rule that only one camera can be started\n> one time. This also allows the following patches that use both imgus to\n> process frames from one single camera.\n\nWhile I think this is right for the IPU3, this is the wrong reason. The\nfact that Chrome OS has little use for capturing from the two cameras at\nthe same time doesn't meant there are no use cases on other platforms.\n\nThe reason why I think this makes sense is that the two ImgU instances\nare actually not independent, both need to be configured before any gets\nstarted. That conflicts with how the libcamera API operates. I would\nlike to not suffer from this limitation though, but that would require\nbroader changes in the public API, I think it's fine to limit operation\nto one camera at a time for now.\n\nOn a side note, when I select a camera in a video call in the Chrome\nbrowser, I get a small preview of each available camera. Isn't this a\nuse case that is of interest to Chrome OS ?\n\nKieran told me that this works under Windows on a Surface Go 2.\nTechnically speaking, I don't know how we could implement it though, as\nfar as I understand, only one instance of the ImgU can operate in video\nmode and guarantee real time operation, while the other instance needs\nto operate in still image capture mode and will work in \"batch\" mode,\nusing the free processing time of the hardware that isn't reserved for\nvideo mode.\n\n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/internal/pipeline_handler.h | 5 ++++\n> src/libcamera/camera.cpp | 11 ++++++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 18 +++++++++++++\n> src/libcamera/pipeline_handler.cpp | 26 +++++++++++++++++++\n> 4 files changed, 60 insertions(+)\n\nThis patch should be split in two, with one patch that extends the\nCamera and PipelineHandler API, and a second patch to implement it in\nthe IPU3 pipeline handler.\n\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c3e4c258..8277ceda 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -55,6 +55,9 @@ public:\n> \tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> \t\t\t\t std::vector> *buffers) = 0;\n> \n> +\tvirtual bool acquire(Camera *camera);\n> +\tvirtual void release(Camera *camera);\n> +\n> \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> \tvoid stop(Camera *camera);\n> \tbool hasPendingRequests(const Camera *camera) const;\n> @@ -76,6 +79,8 @@ protected:\n> \n> \tCameraManager *manager_;\n> \n> +\tstd::set acquiredCameras_;\n\nSpace between Camera and *.\n\n> +\n> private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 713543fd..a31f8769 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -826,6 +826,8 @@ int Camera::exportFrameBuffers(Stream *stream,\n> * \\return 0 on success or a negative error code otherwise\n> * \\retval -ENODEV The camera has been disconnected from the system\n> * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> + * \\retval -EUSERS The maximum number of cameras that can be opened concurrently\n> + * were opened already\n\nThat's not how acquire() should work. The purpose of acquire() is to\nreserve a camera for usage by the application, preventing other\napplications that use libcamera from doing the same. It's a simple\nlocking mechanism really. With this change, it won't be possible for an\napplication to acquire the front and back cameras. This will cause\nissues, for instance, when switching between the front and back cameras\nin an application, if the application wants to acquire the other camera\nbefore releasing the current one, to make sure that, if the acquire\nfails, the current camera can still be used. It also breaks the use case\nof acquiring multiple cameras to switch between them while preventing\nother applications from using any.\n\nI'd like to see a design proposal for how the public API should evolve\nto make mutual exclusion possible, while keeping use cases such as the\nabove supported (unless, of course, you show that those use cases\nactually don't make sense, in which case we could reconsider them).\n\n> */\n> int Camera::acquire()\n> {\n> @@ -845,6 +847,14 @@ int Camera::acquire()\n> \t\treturn -EBUSY;\n> \t}\n> \n> +\tif (!d->pipe_->acquire(this)) {\n> +\t\tLOG(Camera, Info)\n> +\t\t\t<< \"The maximum number of cameras that can be opened\"\n> +\t\t\t \"concurrently were opened already\";\n> +\t\td->pipe_->unlock();\n> +\t\treturn -EUSERS;\n> +\t}\n> +\n> \td->setState(Private::CameraAcquired);\n> \n> \treturn 0;\n> @@ -873,6 +883,7 @@ int Camera::release()\n> \tif (ret < 0)\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> +\td->pipe_->release(this);\n> \td->pipe_->unlock();\n> \n> \td->setState(Private::CameraAvailable);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fd989e61..091a40e1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -141,6 +141,8 @@ public:\n> \tint exportFrameBuffers(Camera *camera, Stream *stream,\n> \t\t\t std::vector> *buffers) override;\n> \n> +\tbool acquire(Camera *camera) override;\n> +\n> \tint start(Camera *camera, const ControlList *controls) override;\n> \tvoid stopDevice(Camera *camera) override;\n> \n> @@ -763,8 +765,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> \treturn 0;\n> }\n> \n> +bool PipelineHandlerIPU3::acquire(Camera *camera) {\n\nThe curly brace should be on the next line.\n\n> +\t/*\n> +\t * Enforce that only a single camera can be used at a time to use both\n> +\t * ImgUs on the camera, so that StillCapture stream can adopt another\n> +\t * set of configuration.\n> +\t */\n> +\tif (!acquiredCameras_.empty())\n\nGiven that the set can only contain a single camera, you could replace\nit with just a pointer.\n\n> +\t\treturn false;\n> +\n> +\treturn PipelineHandler::acquire(camera);\n> +\n> +}\n> +\n> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> +\tif (acquiredCameras_.empty() || *acquiredCameras_.begin() != camera)\n> +\t\treturn -EUSERS;\n> +\n> \tIPU3CameraData *data = cameraData(camera);\n> \tCIO2Device *cio2 = &data->cio2_;\n> \tImgUDevice *imgu = data->imgu_;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 7ebd76ad..fff9ee59 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -270,6 +270,32 @@ void PipelineHandler::unlock()\n> * otherwise\n> */\n> \n> +/**\n> + * \\fn PipelineHandler::acquire()\n> + * \\brief Check if \\a camera can be acquired and supported with the current\n> + * pipeline handler usage. If \\a camera has already been acquired (by the same\n> + * or another process), return false.\n> + * \\param[in] camera The camera\n> + */\n> +bool PipelineHandler::acquire(Camera *camera)\n> +{\n> +\tif (acquiredCameras_.find(camera) != acquiredCameras_.end())\n> +\t\treturn false;\n> +\n> +\tacquiredCameras_.emplace(camera);\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\fn PipelineHandler::release()\n> + * \\brief Release exclusive access to \\a camera\n> + * \\param[in] camera The camera\n> + */\n> +void PipelineHandler::release(Camera *camera)\n> +{\n> +\tacquiredCameras_.erase(camera);\n> +}\n> +\n> /**\n> * \\fn PipelineHandler::start()\n> * \\brief Start capturing from a group of streams","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 23B9FC3272\n\tfor ;\n\tWed, 3 Aug 2022 05:52:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68ADC63312;\n\tWed, 3 Aug 2022 07:52:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B13826330E\n\tfor ;\n\tWed, 3 Aug 2022 07:52:07 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1529A8B;\n\tWed, 3 Aug 2022 07:52:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659505929;\n\tbh=DTrSnRcvF/GWmb5H4fPpwBetqkZT4ThtU5T5zEXc8eY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ynMkks4ZWuJf1BZNFUgfKagU0Cz5NUVwmz36we8D3M/bfgb8KqWXoItq6KmRGEl2/\n\tM9mMi62sTGG0VtGXFRXzN6ZveII+vTKjvs/XXFOlEnYTae5vdt4HlqkeT9uGDtMEn+\n\tl+gLGytOqgS6AM8reDLbPSDsmlI1kA8ScEFcd7nb5pi7DAh1fCntshke+URlUJL3JI\n\tDVU2kc4X6vdgDiEE3wSe0dMWxm/3SGWLzZF2Gr4rfVclom+x/37FsvpYuH1vOk1ttE\n\t2DYUhuDz8brwcK+97yxCBG3rZnu9H+b5pzERt8sWqDLCSzeFFlECuENxvIpDmwmUvW\n\tOh1N1mDDdKQpA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659505927;\n\tbh=DTrSnRcvF/GWmb5H4fPpwBetqkZT4ThtU5T5zEXc8eY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PMenJe/oiN8VDVH+l4iCuwcyYuygUZ3v4hJ9ZG/rgPAnHHyji0z3pvgq/e6woK+Dy\n\tLPT97zbwVwhKdPKLkkzWxwHWw3AWLeiJLwiR/C/3fV3hv37TlvonawaSH5pKR+vCOL\n\tAkt4T5VSYgm4ufgtD+xda4wDCeJz7V5zQrJAoq9U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PMenJe/o\"; dkim-atps=neutral","Date":"Wed, 3 Aug 2022 08:52:01 +0300","To":"Harvey Yang ","Message-ID":"","References":"<20220802102943.3221109-1-chenghaoyang@google.com>\n\t<20220802102943.3221109-3-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220802102943.3221109-3-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24319,"web_url":"https://patchwork.libcamera.org/comment/24319/","msgid":"","date":"2022-08-03T05:58:21","subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 03, 2022 at 08:52:01AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Harvey,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 02, 2022 at 10:29:36AM +0000, Harvey Yang via libcamera-devel wrote:\n> > From: Harvey Yang \n> > \n> > As we hardly have use cases/applications that need both cameras at the\n> > same time, this patch adds a rule that only one camera can be started\n> > one time. This also allows the following patches that use both imgus to\n> > process frames from one single camera.\n> \n> While I think this is right for the IPU3, this is the wrong reason. The\n> fact that Chrome OS has little use for capturing from the two cameras at\n> the same time doesn't meant there are no use cases on other platforms.\n> \n> The reason why I think this makes sense is that the two ImgU instances\n> are actually not independent, both need to be configured before any gets\n> started. That conflicts with how the libcamera API operates. I would\n> like to not suffer from this limitation though, but that would require\n> broader changes in the public API, I think it's fine to limit operation\n> to one camera at a time for now.\n> \n> On a side note, when I select a camera in a video call in the Chrome\n> browser, I get a small preview of each available camera. Isn't this a\n> use case that is of interest to Chrome OS ?\n> \n> Kieran told me that this works under Windows on a Surface Go 2.\n> Technically speaking, I don't know how we could implement it though, as\n> far as I understand, only one instance of the ImgU can operate in video\n> mode and guarantee real time operation, while the other instance needs\n> to operate in still image capture mode and will work in \"batch\" mode,\n> using the free processing time of the hardware that isn't reserved for\n> video mode.\n> \n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 5 ++++\n> > src/libcamera/camera.cpp | 11 ++++++++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 18 +++++++++++++\n> > src/libcamera/pipeline_handler.cpp | 26 +++++++++++++++++++\n> > 4 files changed, 60 insertions(+)\n> \n> This patch should be split in two, with one patch that extends the\n> Camera and PipelineHandler API, and a second patch to implement it in\n> the IPU3 pipeline handler.\n> \n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index c3e4c258..8277ceda 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -55,6 +55,9 @@ public:\n> > \tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> > \t\t\t\t std::vector> *buffers) = 0;\n> > \n> > +\tvirtual bool acquire(Camera *camera);\n> > +\tvirtual void release(Camera *camera);\n> > +\n> > \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> > \tvoid stop(Camera *camera);\n> > \tbool hasPendingRequests(const Camera *camera) const;\n> > @@ -76,6 +79,8 @@ protected:\n> > \n> > \tCameraManager *manager_;\n> > \n> > +\tstd::set acquiredCameras_;\n> \n> Space between Camera and *.\n> \n> > +\n> > private:\n> > \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> > \tvirtual void disconnect();\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 713543fd..a31f8769 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -826,6 +826,8 @@ int Camera::exportFrameBuffers(Stream *stream,\n> > * \\return 0 on success or a negative error code otherwise\n> > * \\retval -ENODEV The camera has been disconnected from the system\n> > * \\retval -EBUSY The camera is not free and can't be acquired by the caller\n> > + * \\retval -EUSERS The maximum number of cameras that can be opened concurrently\n> > + * were opened already\n> \n> That's not how acquire() should work. The purpose of acquire() is to\n> reserve a camera for usage by the application, preventing other\n> applications that use libcamera from doing the same. It's a simple\n> locking mechanism really. With this change, it won't be possible for an\n> application to acquire the front and back cameras. This will cause\n> issues, for instance, when switching between the front and back cameras\n> in an application, if the application wants to acquire the other camera\n> before releasing the current one, to make sure that, if the acquire\n> fails, the current camera can still be used. It also breaks the use case\n> of acquiring multiple cameras to switch between them while preventing\n> other applications from using any.\n> \n> I'd like to see a design proposal for how the public API should evolve\n> to make mutual exclusion possible, while keeping use cases such as the\n> above supported (unless, of course, you show that those use cases\n> actually don't make sense, in which case we could reconsider them).\n\nAnother point I forgot to mention, don't we need an API to let\napplications query mutual exclusion constraints between cameras ?\n\n> > */\n> > int Camera::acquire()\n> > {\n> > @@ -845,6 +847,14 @@ int Camera::acquire()\n> > \t\treturn -EBUSY;\n> > \t}\n> > \n> > +\tif (!d->pipe_->acquire(this)) {\n> > +\t\tLOG(Camera, Info)\n> > +\t\t\t<< \"The maximum number of cameras that can be opened\"\n> > +\t\t\t \"concurrently were opened already\";\n> > +\t\td->pipe_->unlock();\n> > +\t\treturn -EUSERS;\n> > +\t}\n> > +\n> > \td->setState(Private::CameraAcquired);\n> > \n> > \treturn 0;\n> > @@ -873,6 +883,7 @@ int Camera::release()\n> > \tif (ret < 0)\n> > \t\treturn ret == -EACCES ? -EBUSY : ret;\n> > \n> > +\td->pipe_->release(this);\n> > \td->pipe_->unlock();\n> > \n> > \td->setState(Private::CameraAvailable);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index fd989e61..091a40e1 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -141,6 +141,8 @@ public:\n> > \tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > \t\t\t std::vector> *buffers) override;\n> > \n> > +\tbool acquire(Camera *camera) override;\n> > +\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > \tvoid stopDevice(Camera *camera) override;\n> > \n> > @@ -763,8 +765,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> > \treturn 0;\n> > }\n> > \n> > +bool PipelineHandlerIPU3::acquire(Camera *camera) {\n> \n> The curly brace should be on the next line.\n> \n> > +\t/*\n> > +\t * Enforce that only a single camera can be used at a time to use both\n> > +\t * ImgUs on the camera, so that StillCapture stream can adopt another\n> > +\t * set of configuration.\n> > +\t */\n> > +\tif (!acquiredCameras_.empty())\n> \n> Given that the set can only contain a single camera, you could replace\n> it with just a pointer.\n> \n> > +\t\treturn false;\n> > +\n> > +\treturn PipelineHandler::acquire(camera);\n> > +\n> > +}\n> > +\n> > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> > {\n> > +\tif (acquiredCameras_.empty() || *acquiredCameras_.begin() != camera)\n> > +\t\treturn -EUSERS;\n> > +\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tCIO2Device *cio2 = &data->cio2_;\n> > \tImgUDevice *imgu = data->imgu_;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 7ebd76ad..fff9ee59 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -270,6 +270,32 @@ void PipelineHandler::unlock()\n> > * otherwise\n> > */\n> > \n> > +/**\n> > + * \\fn PipelineHandler::acquire()\n> > + * \\brief Check if \\a camera can be acquired and supported with the current\n> > + * pipeline handler usage. If \\a camera has already been acquired (by the same\n> > + * or another process), return false.\n> > + * \\param[in] camera The camera\n> > + */\n> > +bool PipelineHandler::acquire(Camera *camera)\n> > +{\n> > +\tif (acquiredCameras_.find(camera) != acquiredCameras_.end())\n> > +\t\treturn false;\n> > +\n> > +\tacquiredCameras_.emplace(camera);\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\fn PipelineHandler::release()\n> > + * \\brief Release exclusive access to \\a camera\n> > + * \\param[in] camera The camera\n> > + */\n> > +void PipelineHandler::release(Camera *camera)\n> > +{\n> > +\tacquiredCameras_.erase(camera);\n> > +}\n> > +\n> > /**\n> > * \\fn PipelineHandler::start()\n> > * \\brief Start capturing from a group of streams","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 EDEE4BE173\n\tfor ;\n\tWed, 3 Aug 2022 05:58:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E44F63315;\n\tWed, 3 Aug 2022 07:58:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D73A96330E\n\tfor ;\n\tWed, 3 Aug 2022 07:58:27 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4B58B8B;\n\tWed, 3 Aug 2022 07:58:27 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659506309;\n\tbh=ZvnPHvg1LTeEkj2Xlp8DoiyAFiSLUbqJE53ywI/R7vo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=0nayO0UMldd3LydGZGbGrPsZ4S46gEAbYZpO8ThLlzzCgE0O1LkopvvUZ8Klxll1q\n\t9Rx57oLz3cF+NWeVM4VSK2dkN+E5E/5/k2K0sixDECk/fp0SDYJd3hcZoc0D35E2YW\n\tJikNZHsh5KioDQJQUSIVtJaAgXat6WmOQ8hcnTPQ9nGb9ILgRcujJU5tERnU/Eo8uZ\n\tDiC8qtIGxW0UuCzarWkGwh4fua8DTDRQyOHKIy0nRkKoqLO1JvdCHYl0mg3bl2obPw\n\tQhqra6ERKSuVyaRBhaRXtScM+s8pBmMt23FxNvX54+/juwW4QRQaTvkqQAxMK7vbWI\n\tTqDISwu+hUFrg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659506307;\n\tbh=ZvnPHvg1LTeEkj2Xlp8DoiyAFiSLUbqJE53ywI/R7vo=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=RvYwh0ahSh4Bs16HOYYtnv76QWx0Y+ihM6DG6hl7TSgMHzkElCJsxu1VTzIxGiQMu\n\tOTuujXo2KyPVovTdW5Sqg+JPerMZwsbhbhQmHEWJmtQ07KVwVq/HzxfzSK25k4fhYY\n\tCfEDGkgIl3rKf4hVu4EEjTOD+FmX5TLS46f2+fIE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RvYwh0ah\"; dkim-atps=neutral","Date":"Wed, 3 Aug 2022 08:58:21 +0300","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220802102943.3221109-1-chenghaoyang@google.com>\n\t<20220802102943.3221109-3-chenghaoyang@google.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24362,"web_url":"https://patchwork.libcamera.org/comment/24362/","msgid":"","date":"2022-08-04T07:23:33","subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nThanks for your comments!\n\nOn Wed, Aug 3, 2022 at 1:58 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Wed, Aug 03, 2022 at 08:52:01AM +0300, Laurent Pinchart via\n> libcamera-devel wrote:\n> > Hi Harvey,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Aug 02, 2022 at 10:29:36AM +0000, Harvey Yang via\n> libcamera-devel wrote:\n> > > From: Harvey Yang \n> > >\n> > > As we hardly have use cases/applications that need both cameras at the\n> > > same time, this patch adds a rule that only one camera can be started\n> > > one time. This also allows the following patches that use both imgus to\n> > > process frames from one single camera.\n> >\n> > While I think this is right for the IPU3, this is the wrong reason. The\n> > fact that Chrome OS has little use for capturing from the two cameras at\n> > the same time doesn't meant there are no use cases on other platforms.\n> >\n\n\nBefore I update the commit message, let's go through the issues below first\n:)\n\n\n>\n> > The reason why I think this makes sense is that the two ImgU instances\n> > are actually not independent, both need to be configured before any gets\n> > started. That conflicts with how the libcamera API operates. I would\n> > like to not suffer from this limitation though, but that would require\n> > broader changes in the public API, I think it's fine to limit operation\n> > to one camera at a time for now.\n> >\n\n\nSo Han-lin and I have never tested soraka (with IPU3) on using both cameras\nsimultaneously. Do you mean that it never works, even if we assign one ImgU\nto each camera in PipelineHandlerIPU3?\nIs the culprit here [1]?\n\n[1]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n550\n\n\n>\n> > On a side note, when I select a camera in a video call in the Chrome\n> > browser, I get a small preview of each available camera. Isn't this a\n> > use case that is of interest to Chrome OS ?\n> >\n\n\nWhich web app did you use? I didn't find this feature on Google Meet\nthough...\nI'd like to try how the current libcamera on soraka support this feature :)\n\n\n>\n> > Kieran told me that this works under Windows on a Surface Go 2.\n> > Technically speaking, I don't know how we could implement it though, as\n> > far as I understand, only one instance of the ImgU can operate in video\n> > mode and guarantee real time operation, while the other instance needs\n> > to operate in still image capture mode and will work in \"batch\" mode,\n> > using the free processing time of the hardware that isn't reserved for\n> > video mode.\n> >\n\n\nYou're talking about the current situation of IPU3 ImgUs, right?\nI thought that libcamera's PipelineHandlerIPU3 only uses the two ImgUs in\nvideo mode? Is the constraint mentioned somewhere in the code or\ndocumentation (in libcamera or the shipped HAL)?\n\n\n>\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 5 ++++\n> > > src/libcamera/camera.cpp | 11 ++++++++\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 18 +++++++++++++\n> > > src/libcamera/pipeline_handler.cpp | 26 +++++++++++++++++++\n> > > 4 files changed, 60 insertions(+)\n> >\n> > This patch should be split in two, with one patch that extends the\n> > Camera and PipelineHandler API, and a second patch to implement it in\n> > the IPU3 pipeline handler.\n> >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h\n> b/include/libcamera/internal/pipeline_handler.h\n> > > index c3e4c258..8277ceda 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -55,6 +55,9 @@ public:\n> > > virtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> > >\n> std::vector> *buffers) = 0;\n> > >\n> > > + virtual bool acquire(Camera *camera);\n> > > + virtual void release(Camera *camera);\n> > > +\n> > > virtual int start(Camera *camera, const ControlList *controls) = 0;\n> > > void stop(Camera *camera);\n> > > bool hasPendingRequests(const Camera *camera) const;\n> > > @@ -76,6 +79,8 @@ protected:\n> > >\n> > > CameraManager *manager_;\n> > >\n> > > + std::set acquiredCameras_;\n> >\n> > Space between Camera and *.\n> >\n> > > +\n> > > private:\n> > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > virtual void disconnect();\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 713543fd..a31f8769 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -826,6 +826,8 @@ int Camera::exportFrameBuffers(Stream *stream,\n> > > * \\return 0 on success or a negative error code otherwise\n> > > * \\retval -ENODEV The camera has been disconnected from the system\n> > > * \\retval -EBUSY The camera is not free and can't be acquired by the\n> caller\n> > > + * \\retval -EUSERS The maximum number of cameras that can be opened\n> concurrently\n> > > + * were opened already\n> >\n> > That's not how acquire() should work. The purpose of acquire() is to\n> > reserve a camera for usage by the application, preventing other\n> > applications that use libcamera from doing the same. It's a simple\n> > locking mechanism really. With this change, it won't be possible for an\n> > application to acquire the front and back cameras. This will cause\n> > issues, for instance, when switching between the front and back cameras\n> > in an application, if the application wants to acquire the other camera\n> > before releasing the current one, to make sure that, if the acquire\n> > fails, the current camera can still be used. It also breaks the use case\n> > of acquiring multiple cameras to switch between them while preventing\n> > other applications from using any.\n> >\n> > I'd like to see a design proposal for how the public API should evolve\n> > to make mutual exclusion possible, while keeping use cases such as the\n> > above supported (unless, of course, you show that those use cases\n> > actually don't make sense, in which case we could reconsider them).\n>\n> Another point I forgot to mention, don't we need an API to let\n> applications query mutual exclusion constraints between cameras ?\n>\n>\nI see. Sorry that Han-lin and I didn't fully understand how\nlibcamera::Camera::acquire() works.\n\nAs currently the Android adapter calls libcamera::Camera::start() only when\nthe first frame request comes, I don't think it's a proper code path that\nwe tell the user/application about the mutual exclusion constraints (i.e.\nfail the start operation).\nTherefore, I agree that a new API to let applications query mutual\nexclusion constraints makes more sense. In the Android API, we have\n|conflicting_devices| [2] and |resource_cost| [3]. Do you think it's\nappropriate to simply add the same API in libcamera?\n\n[2]: https://source.android.com/devices/camera/versioning#24\n[3]:\nhttps://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/camera/mojo/camera_common.mojom#30\n\n\n> > > */\n> > > int Camera::acquire()\n> > > {\n> > > @@ -845,6 +847,14 @@ int Camera::acquire()\n> > > return -EBUSY;\n> > > }\n> > >\n> > > + if (!d->pipe_->acquire(this)) {\n> > > + LOG(Camera, Info)\n> > > + << \"The maximum number of cameras that can be\n> opened\"\n> > > + \"concurrently were opened already\";\n> > > + d->pipe_->unlock();\n> > > + return -EUSERS;\n> > > + }\n> > > +\n> > > d->setState(Private::CameraAcquired);\n> > >\n> > > return 0;\n> > > @@ -873,6 +883,7 @@ int Camera::release()\n> > > if (ret < 0)\n> > > return ret == -EACCES ? -EBUSY : ret;\n> > >\n> > > + d->pipe_->release(this);\n> > > d->pipe_->unlock();\n> > >\n> > > d->setState(Private::CameraAvailable);\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index fd989e61..091a40e1 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -141,6 +141,8 @@ public:\n> > > int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > std::vector>\n> *buffers) override;\n> > >\n> > > + bool acquire(Camera *camera) override;\n> > > +\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > void stopDevice(Camera *camera) override;\n> > >\n> > > @@ -763,8 +765,24 @@ int PipelineHandlerIPU3::freeBuffers(Camera\n> *camera)\n> > > return 0;\n> > > }\n> > >\n> > > +bool PipelineHandlerIPU3::acquire(Camera *camera) {\n> >\n> > The curly brace should be on the next line.\n> >\n> > > + /*\n> > > + * Enforce that only a single camera can be used at a time to use\n> both\n> > > + * ImgUs on the camera, so that StillCapture stream can adopt\n> another\n> > > + * set of configuration.\n> > > + */\n> > > + if (!acquiredCameras_.empty())\n> >\n> > Given that the set can only contain a single camera, you could replace\n> > it with just a pointer.\n> >\n> > > + return false;\n> > > +\n> > > + return PipelineHandler::acquire(camera);\n> > > +\n> > > +}\n> > > +\n> > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const\n> ControlList *controls)\n> > > {\n> > > + if (acquiredCameras_.empty() || *acquiredCameras_.begin() !=\n> camera)\n> > > + return -EUSERS;\n> > > +\n> > > IPU3CameraData *data = cameraData(camera);\n> > > CIO2Device *cio2 = &data->cio2_;\n> > > ImgUDevice *imgu = data->imgu_;\n> > > diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> > > index 7ebd76ad..fff9ee59 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -270,6 +270,32 @@ void PipelineHandler::unlock()\n> > > * otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\fn PipelineHandler::acquire()\n> > > + * \\brief Check if \\a camera can be acquired and supported with the\n> current\n> > > + * pipeline handler usage. If \\a camera has already been acquired (by\n> the same\n> > > + * or another process), return false.\n> > > + * \\param[in] camera The camera\n> > > + */\n> > > +bool PipelineHandler::acquire(Camera *camera)\n> > > +{\n> > > + if (acquiredCameras_.find(camera) != acquiredCameras_.end())\n> > > + return false;\n> > > +\n> > > + acquiredCameras_.emplace(camera);\n> > > + return true;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn PipelineHandler::release()\n> > > + * \\brief Release exclusive access to \\a camera\n> > > + * \\param[in] camera The camera\n> > > + */\n> > > +void PipelineHandler::release(Camera *camera)\n> > > +{\n> > > + acquiredCameras_.erase(camera);\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn PipelineHandler::start()\n> > > * \\brief Start capturing from a group of streams\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n\nThanks again!","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 AF521C3272\n\tfor ;\n\tThu, 4 Aug 2022 07:23:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEBC26332B;\n\tThu, 4 Aug 2022 09:23:48 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92C4B603E4\n\tfor ;\n\tThu, 4 Aug 2022 09:23:46 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id t1so29564276lft.8\n\tfor ;\n\tThu, 04 Aug 2022 00:23:46 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659597829;\n\tbh=lyamxnoMvtglA5gpYuIG5tC0TLWledr92VDZMyurATc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=exGUn6MJEIKJmJfqyo5YpoC+KGk7LT6pt4ChAx9wVmZjn4FPZC3Vge4VPW/1bp89v\n\tRSE4/5XRbW9TpGV/3pLgorczaLFPwC9UsxEqkTvpngW0UVlaZ5+RsbAbb63Au7FLjO\n\tMYnyQy0Ofrig6efXa/l76Ic8G24eN5GEO3G8mNARCmrXGaDr08KCLBJuI2sBhKYahZ\n\t2I7lArbGN0woBfZyW00EliCYTHXTkYw1Nh4Lhbgim9Kc79yP5rcDsmWDehBwv/AOqZ\n\th07mfxSdS1hgcozxaMUsoHO1XXfV/oqbJ+NGPw1Dvf+3tLbMKZE/Ts/TSTO6DOAZVd\n\tBHtiXhunas9ow==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=vMiEy3x1d+0xHnq83lvRsj9naJYdHFKukxAcb6UXPOM=;\n\tb=BiUxn8MVzt2dSxjQu6Xa9SVr7otyM/6w4Cpoj+pS9sVzpYBorIUC11iUaxgi3nnRJC\n\tmoF7NibZN/ae5Y/SBdHDsrup6IWf9/b1Bx5uF0skBcu1B8aZ8fbY1kbzZc/oq2B/Xniq\n\t4DNDq7ozoE5TSc4rIUodrxdXzTmkcJOTYU9Is="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"BiUxn8MV\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=vMiEy3x1d+0xHnq83lvRsj9naJYdHFKukxAcb6UXPOM=;\n\tb=m9SOdlH/3GNtDp+9cc1qQHKhpJduWuNqLdoLCruoDg2lkocx5OWr9a/8/aoKqI1WrR\n\tmcUtbEkGx6f90bxHxDSa+vvNTUf1GyiCsy6D//9LEpvapXPMUdS+rVei1EFUCqzltYXm\n\tptIOK/hRwN1c66exYjP6bib0trVncTEw7Iy+msuJfYbEF6lr9um1FMTfIdqBsXmVmLx3\n\tHVTLhZGidVUlMzDqIOOUT+1UK6pPQjoE5kgr7t8P/0N51s0Vmxb/OEh8hYepmfImnHTk\n\tsB6ftFHwvHoFFaaRkNzCTyWTZ4Yw5Ey1MELrFor4Eliu2SEdn3seocHTkiWT1GxTN0Wq\n\tyKcQ==","X-Gm-Message-State":"ACgBeo30d//7OhuuKqZ4AGPW6KojFOdP/kXffBg3gg0xqqPL3m5sMLTI\n\tJVs/mjHJ6HFewA6ods6BogzTSPhdBCpn/lsDV1nIrvmUnlSzpA==","X-Google-Smtp-Source":"AA6agR568bEdk+PWfhN2JgpsZrWGf5qC1L26azwWHpLniVPdHLC2Mwl0gUKhtx3vC3BXNcj0TQyynLcFW3dJJ57xid8=","X-Received":"by 2002:a05:6512:ad6:b0:48a:20f9:c0e6 with SMTP id\n\tn22-20020a0565120ad600b0048a20f9c0e6mr245509lfu.287.1659597825591;\n\tThu, 04 Aug 2022 00:23:45 -0700 (PDT)","MIME-Version":"1.0","References":"<20220802102943.3221109-1-chenghaoyang@google.com>\n\t<20220802102943.3221109-3-chenghaoyang@google.com>\n\t\n\t","In-Reply-To":"","Date":"Thu, 4 Aug 2022 15:23:33 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000dd64c005e5653ae4\"","Subject":"Re: [libcamera-devel] [PATCH v4 2/9] ipu3: Allow only one camera\n\tbeing started","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","From":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]