[{"id":23093,"web_url":"https://patchwork.libcamera.org/comment/23093/","msgid":"<165297248864.368702.6826368874724191099@Monstersaurus>","date":"2022-05-19T15:01:28","subject":"Re: [libcamera-devel] [PATCH 2/8] Allow only one camera being\n\tstarted","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang via libcamera-devel (2022-05-12 11:32:52)\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\nI think 'not having many usecases' for something isn't a good reason to\nforcibly disable it. If the hardware *can not* do it, then that's\nacceptable.\n\nIs there any use case where it is possible to capture from two cameras\nat the same time? I think it may simply not be possible on the IPU3 -\nbut if that's the case, then that should be the documented rationale\nbehind this patch.\n\nIf we could for instance capture from both the IR sensor (which may not\nneed the ISP?), and the image sensor at the same time on a front facing\ncamera device like on the SGo2 - then I don't think we should be\npreventing that here.\n\n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fd989e61..111ba053 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -166,6 +166,8 @@ private:\n> MediaDevice *cio2MediaDev_;\n> MediaDevice *imguMediaDev_;\n> \n> + Camera *inUseCamera_ = nullptr;\n> +\n> std::vector ipaBuffers_;\n> };\n> \n> @@ -765,6 +767,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> \n> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> + // Deny second camera being started.\n\nlibcamera code style here for a single line comment should be\n\t/* Deny second camera being started. */\n\nBut what about the third?\n\nIf this is /not/ possible - then I think it should be more like:\n\n\t/* \n\t * Enforce that only a single camera can be used at a time due\n\t * to the limitations of the IPU3 IMGU which can only ..\n\t * .\n\t */\n\n> + if (inUseCamera_ && inUseCamera_ != camera)\n> + return -1;\n\nErrnos rather than -1 ...\n\nPerhaps -EBUSY ...\n\n> +\n> IPU3CameraData *data = cameraData(camera);\n> CIO2Device *cio2 = &data->cio2_;\n> ImgUDevice *imgu = data->imgu_;\n> @@ -781,6 +787,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n> if (ret)\n> return ret;\n> \n> + inUseCamera_ = camera;\n> +\n> ret = data->ipa_->start();\n> if (ret)\n> goto error;\n> @@ -808,6 +816,8 @@ error:\n> freeBuffers(camera);\n> LOG(IPU3, Error) << \"Failed to start camera \" << camera->id();\n> \n> + inUseCamera_ = nullptr;\n> +\n> return ret;\n> }\n> \n> @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> LOG(IPU3, Warning) << \"Failed to stop camera \" << camera->id();\n> \n> freeBuffers(camera);\n> +\n> + inUseCamera_ = nullptr;\n> }\n> \n> void IPU3CameraData::cancelPendingRequests()\n> -- \n> 2.36.0.512.ge40c2bad7a-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B9EC4C0F2A\n\tfor ;\n\tThu, 19 May 2022 15:01:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD2BD65663;\n\tThu, 19 May 2022 17:01:32 +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 675E160420\n\tfor ;\n\tThu, 19 May 2022 17:01:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E37CA6E0;\n\tThu, 19 May 2022 17:01:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1652972492;\n\tbh=ImAerzLrkCS9+eykGdxQUJ9VEFEfUZEtaKFbDmvt7a0=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Fq7phRv+f3R9icLsiApUfyemooanDSymG2+UJVnfR6E/TK8f/SNLyvZxw+xSiIUy6\n\tpysfoRhcc7O6zZA8JEu3ZqYrmT8QnL3wXD3DtNgSvtPsFzwdvHbLWF18OwLxtamK7Q\n\t6t8cUkEdFtZ867hY+aFn8LbjAFqs5IohDFfe4h+D9R+P/ed+/Ey4lrMZrUf1v4EDdo\n\tsSb53bbTMaYETc1TDLS347kvtLJnXgX4jX56yCfR1HI7H3xqzEN8pOVHS3m4GgzPqD\n\tNgy3vKN1bvQ1guQf0+Wp3tvWtQbeIuZA6bJFjvY5qWDMc6POZT9yPXXPujWEgDEfV4\n\tFrnc9V3/AG0fg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1652972491;\n\tbh=ImAerzLrkCS9+eykGdxQUJ9VEFEfUZEtaKFbDmvt7a0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=HVWjf1LbH6GTz33h80WqV0Grd+p7YREpNyFhJUWXGbWFtxDEqkT7gB/fXc0UrYs2v\n\taYr6Ws0B16sZ0aFAYxEtQXP3L0jE83C6YLHvEGlCHMh5CKviKv6Bkb4DjZ3oVgm0QN\n\tz3j9z8WgFEDP7zc5xZlL7WnpKST5spdbC1vxCOLE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"HVWjf1Lb\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220512103258.324339-3-chenghaoyang@google.com>","References":"<20220512103258.324339-1-chenghaoyang@google.com>\n\t<20220512103258.324339-3-chenghaoyang@google.com>","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 19 May 2022 16:01:28 +0100","Message-ID":"<165297248864.368702.6826368874724191099@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/8] Allow only one camera being\n\tstarted","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Harvey Yang ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23099,"web_url":"https://patchwork.libcamera.org/comment/23099/","msgid":"","date":"2022-05-20T05:05:00","subject":"Re: [libcamera-devel] [PATCH 2/8] Allow only one camera being\n\tstarted","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"> I think 'not having many usecases' for something isn't a good reason to\n> forcibly disable it. If the hardware *can not* do it, then that's\n> acceptable.\n\n> Is there any use case where it is possible to capture from two cameras\n> at the same time? I think it may simply not be possible on the IPU3 -\n> but if that's the case, then that should be the documented rationale\n> behind this patch.\n\nFrom Han-lin's perspective, basically there is no such a use case to enable\nboth cameras at the same time. Maybe it's only true for ipu3. I'm not sure.\n\nI also thought that we can dynamically assign ImgUs: if only one camera is\nbeing used, it can occupy both the ImgUs. However, when another camera\nis enabled, the first camera needs to release one of the ImgU, and fall back\nthe StillCapture to VideoSnapshot.\nHan-lin and I think that ideally it's possible, while we're not sure if\nit's worth\nthe effort, as there's simply no such a use case, and there's very likely\nto be\nsome lag during the transition (i.e. the first camera's stream might pause\nwhen reconfiguring ISP).\n\n> If we could for instance capture from both the IR sensor (which may not\n> need the ISP?), and the image sensor at the same time on a front facing\n> camera device like on the SGo2 - then I don't think we should be\n> preventing that here.\n\nIIUC the ipu3 pipeline handler prevents cases that use only StreamRole::Raw,\nwhich means the ISP is necessary. Right?\n\n--------------\n\nFor other suggestions, I'll fix them in the following patch. Let's wait for\nothers'\ncomments on all the patches to prevent the spam :)\n\nThanks for looking into this!\n\nBR,\nHarvey\n\nOn Thu, May 19, 2022 at 11:01 PM Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Harvey Yang via libcamera-devel (2022-05-12 11:32:52)\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> I think 'not having many usecases' for something isn't a good reason to\n> forcibly disable it. If the hardware *can not* do it, then that's\n> acceptable.\n>\n> Is there any use case where it is possible to capture from two cameras\n> at the same time? I think it may simply not be possible on the IPU3 -\n> but if that's the case, then that should be the documented rationale\n> behind this patch.\n>\n> If we could for instance capture from both the IR sensor (which may not\n> need the ISP?), and the image sensor at the same time on a front facing\n> camera device like on the SGo2 - then I don't think we should be\n> preventing that here.\n>\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index fd989e61..111ba053 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -166,6 +166,8 @@ private:\n> > MediaDevice *cio2MediaDev_;\n> > MediaDevice *imguMediaDev_;\n> >\n> > + Camera *inUseCamera_ = nullptr;\n> > +\n> > std::vector ipaBuffers_;\n> > };\n> >\n> > @@ -765,6 +767,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)\n> >\n> > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const\n> ControlList *controls)\n> > {\n> > + // Deny second camera being started.\n>\n> libcamera code style here for a single line comment should be\n> /* Deny second camera being started. */\n>\n> But what about the third?\n>\n> If this is /not/ possible - then I think it should be more like:\n>\n> /*\n> * Enforce that only a single camera can be used at a time due\n> * to the limitations of the IPU3 IMGU which can only ..\n> * .\n> */\n>\n> > + if (inUseCamera_ && inUseCamera_ != camera)\n> > + return -1;\n>\n> Errnos rather than -1 ...\n>\n> Perhaps -EBUSY ...\n>\n> > +\n> > IPU3CameraData *data = cameraData(camera);\n> > CIO2Device *cio2 = &data->cio2_;\n> > ImgUDevice *imgu = data->imgu_;\n> > @@ -781,6 +787,8 @@ int PipelineHandlerIPU3::start(Camera *camera,\n> [[maybe_unused]] const ControlLis\n> > if (ret)\n> > return ret;\n> >\n> > + inUseCamera_ = camera;\n> > +\n> > ret = data->ipa_->start();\n> > if (ret)\n> > goto error;\n> > @@ -808,6 +816,8 @@ error:\n> > freeBuffers(camera);\n> > LOG(IPU3, Error) << \"Failed to start camera \" << camera->id();\n> >\n> > + inUseCamera_ = nullptr;\n> > +\n> > return ret;\n> > }\n> >\n> > @@ -826,6 +836,8 @@ void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> > LOG(IPU3, Warning) << \"Failed to stop camera \" <<\n> camera->id();\n> >\n> > freeBuffers(camera);\n> > +\n> > + inUseCamera_ = nullptr;\n> > }\n> >\n> > void IPU3CameraData::cancelPendingRequests()\n> > --\n> > 2.36.0.512.ge40c2bad7a-goog\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 03BF5C0F2A\n\tfor ;\n\tFri, 20 May 2022 05:05:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5570365665;\n\tFri, 20 May 2022 07:05:14 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1342B60418\n\tfor ;\n\tFri, 20 May 2022 07:05:12 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id u23so12449032lfc.1\n\tfor ;\n\tThu, 19 May 2022 22:05:12 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653023114;\n\tbh=1+sO6yhshabRJyDdnUfW8ZN0ArRsKAew6Tb7B6gE5rg=;\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=tDNbAxqmNVcUjYwXYn4Ve0/IwVGV/8WHW7eYEkXGV2DD0b1gyVuR8JXy/E6Nn4s24\n\tw8T7iXcAX0Eno4T8hi0QdkjWzZH/dtq7hm26laJbwui70YB9Ce9Fpb8zbwi8bV7QOo\n\tglpl4pXtJ8T+/eTguXIJjmN9+oGmDu+Vjb9s/ZtraCaI6dKm9pBoTk3JI1FZIPlt8q\n\tXGOTqLNJ64Xy3u6HH72N0un3FJHa6EzZOBtqhLGZdhqnprXBAC2pCauQjQaSDLDrtg\n\t4cFbC7HCY8hesm1yLfr8uQcTo3vvNHyh6WG558CmiC9EcQ/FLmLsu34IqmxnVulyqy\n\tt38pqaNM6Hcpg==","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=o6aaU7psgch5F9BLjy2AHxdwG9w+oDFsbE28owVnrgM=;\n\tb=MAy/H5LoH/FDxM+E9H1lTmXIKiS1XL0e+xybagGAu1mvEwB2sW6j+Rre5oKSQmLLxl\n\ta/sUJ1rOQcIqSxmqcCOvfa9LaKw6Hu10zwU9eDszHwNYnbDwCuAPFMsywi++dNupd1oB\n\tUXCGa0cxg6SL7/G94PINEolhI5mO23Me943S4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"MAy/H5Lo\"; \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=o6aaU7psgch5F9BLjy2AHxdwG9w+oDFsbE28owVnrgM=;\n\tb=SYwh2R27mbST/6xJx1u94N3mEvL8/7FzdV9AhK0UreqKJXYF20SX1ECA99jalEzVVZ\n\tyXtX1bhtvNwbF1SXLh3l3LcejGhon+Jj07RLbrE3ZBxRgTNRE9w/X49a9uHy2Fj4B/zx\n\trz29WKZc6s8eTI+xb1OQ0uj6Hl27iWNQ+PT5RVk7O/qpktfwYkK7blLWS5AdwC8XvWnZ\n\tbgXxo5oDqKYTlJDIbPvnizpyiQq0lrrwyGAfQnAau0BtTFzG6vbTCQod34g9NXS4Jp6z\n\tdjEriJSr0gt8LFjqA94HvDuB1oLToHCpLOQUWwJt7Sl+yWU4rDbbdY0DVoX7LlJiad1B\n\tT6ow==","X-Gm-Message-State":"AOAM530wZuErsm5nalF4myRxm4soRmQ4YnB2n8juRZl1jhvuVqm8i0kh\n\tImjdSPKmv2F0/zAl66AR+m6hpk5UIQOOOfGxR6Pznw==","X-Google-Smtp-Source":"ABdhPJwKX63rngY3Mykgbw9s2h178vutdvE0YsoS0Ce6OJPbBsDSLYKQa6Gxp8cAXfYau9vcoYEgFZ7L27wMuYJrjwo=","X-Received":"by 2002:a05:6512:2613:b0:448:5164:689d with SMTP id\n\tbt19-20020a056512261300b004485164689dmr5576385lfb.526.1653023111138;\n\tThu, 19 May 2022 22:05:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20220512103258.324339-1-chenghaoyang@google.com>\n\t<20220512103258.324339-3-chenghaoyang@google.com>\n\t<165297248864.368702.6826368874724191099@Monstersaurus>","In-Reply-To":"<165297248864.368702.6826368874724191099@Monstersaurus>","Date":"Fri, 20 May 2022 13:05:00 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"00000000000058623705df6a6f02\"","Subject":"Re: [libcamera-devel] [PATCH 2/8] Allow only one camera being\n\tstarted","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\" "}}]