[{"id":14060,"web_url":"https://patchwork.libcamera.org/comment/14060/","msgid":"<20201204160806.GA2018713@oden.dyn.berto.se>","date":"2020-12-04T16:08:06","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Stop camera\n\twhen re-configuring it","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-12-04 15:39:55 +0100, Jacopo Mondi wrote:\n> The Android camera device HAL3 specification does not require a\n> camera to go through any explicit close() call between configurations.\n> It is legit for a camera to be configured, a number of requests\n> processed and the re-configured again without any explicit stop.\n> \n> The libcamera Android camera HAL starts the Camera at the first handled\n> request, and only stops it at camera close time. This mean, that two\n> camera configuration attempts part of the same streaming session are only\n> interleaved by capture requests handling.\n> \n> The libcamera::Camera state machine requires the Camera to be stopped,\n> before any configuration take place, and this currently doesn't happen\n> in the camera HAL CameraDevice class.\n> \n> Fix this by stopping the camera and the associated worker thread if\n> a configuration attempt is performed while the Camera is in running\n> state.\n> \n> This patch fixes cros_camera_test:\n> Camera3PreviewTest/Camera3SinglePreviewTest.Camera3BasicPreviewTest/0\n> \n> Signed-off-by: Jacopo Mondi \n> \n> ---\n> Alternatively, the camera can be stopped at the very beginning of the\n> configureStream() function. But in that case the camera gets stopped\n> even if the configuration attempt fails (ie at validation time).\n> \n> As both behaviour seems to be compliant with what Android expects, I decided\n> to stop the camera only if the configuration is valid.\n\nI'm leaning a bit towards also stopping the camera if the configuration \nfails, from a power perspective it would it not make most sens? I'm head \ndeep in other PM work so maybe I'm biased ;-) As our current goal is to \npass CTS I don't think there is a need to bikesheed on this and this is \nclearly a step in the right direction,\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> \n> src/android/camera_device.cpp | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 675af5705055..2500994e2f6a 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1341,8 +1341,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \n> \t/*\n> \t * Once the CameraConfiguration has been adjusted/validated\n> -\t * it can be applied to the camera.\n> +\t * it can be applied to the camera, which has to be stopped if\n> +\t * in running state.\n> \t */\n> +\tif (running_) {\n> +\t\tworker_.stop();\n> +\t\tcamera_->stop();\n> +\t\trunning_ = false;\n> +\t}\n> +\n> \tint ret = camera_->configure(config_.get());\n> \tif (ret) {\n> \t\tLOG(HAL, Error) << \"Failed to configure camera '\"\n> --\n> 2.29.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 19BC7BE177\n\tfor ;\n\tFri, 4 Dec 2020 16:08:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7327635D6;\n\tFri, 4 Dec 2020 17:08:10 +0100 (CET)","from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3C0F635D0\n\tfor ;\n\tFri, 4 Dec 2020 17:08:08 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id j10so7185451lja.5\n\tfor ;\n\tFri, 04 Dec 2020 08:08:08 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf2sm1880695ljc.118.2020.12.04.08.08.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 04 Dec 2020 08:08:07 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"k7Vpud4p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=9Ti+R2G2WjxaUL2J1QpgK//KJGJyfry/rnc+fTcgWnE=;\n\tb=k7Vpud4pS7jIzd5X2m4zZ24qte7sl4/68jZ+Z/q2EJue7Z9j/9X4a8I9Vd2xQQ4/2w\n\t3mUfSC2HyJF8NUAAyWd2cPlihXQoWqOi/XOceMDvodJ9jDjueacbR/IOGIutUbPz4bDG\n\tij6cwfte8BiYJoRiGRT/94JoR7ND5Ji9cueFrejih+9Ml2w9HrIAKFK1vzN5+mtLi8eX\n\t71wulSxElJhK/G0UV9GnteYr96CervBZfJIdvnkpFnrhO+i1BMN/DgmnvYNGiA+rHXrv\n\tfXNOC2MyHzzl3ERc5l1h+LePJiRNxtZMZTWhJ6VEmmVH0t8kUda/vKx2nkbgyB210ZUH\n\thNUg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=9Ti+R2G2WjxaUL2J1QpgK//KJGJyfry/rnc+fTcgWnE=;\n\tb=t6zH0VCaXewLKP+1E7Fvqu3GTqMRxkEFBTn0S8a9gEXfUQl5JLz3lIg0/2edq8q9vi\n\tcBH7JRcZP/V37IJbr84BYNS+6zGbZ9NYZPQy+0Ul6QQlVkqiCpT6GSFVQ2yO7t/+omfr\n\tR7iQR3ik+7On9AmVyPx5csbnmtc90Tfc3n80SYNYu9xUqPlw9hWZR1amLWCzAV53D13O\n\tAd6M0AzVg9nw+e4Y5xyt8qF95csKyC0wl90MRrF9B/Q4wCY037gTZDHZ7v6YVHSm388W\n\tNXKAfAPiv12YBgE40dMUR3JMW5ON2woI9hv41gWU5O5dMLMsvoc3cPHczbNArsreZMnm\n\tGSLA==","X-Gm-Message-State":"AOAM5331xokr4nRUgGwtsxiFejm0BS73pxHmdgSzuw3OQdiY247+vyhA\n\t9FSpQVdRun0nrgCIGRmF2ZZ8hcXT7BDc8w==","X-Google-Smtp-Source":"ABdhPJyGvnpic7+38qAduWBmfqGFkfL/FDPSMud5JbUY2pQGqk3A6e25yWC1pGZv7DdyctpfxF6S8A==","X-Received":"by 2002:a2e:85d1:: with SMTP id\n\th17mr3741898ljj.235.1607098088106; \n\tFri, 04 Dec 2020 08:08:08 -0800 (PST)","Date":"Fri, 4 Dec 2020 17:08:06 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20201204160806.GA2018713@oden.dyn.berto.se>","References":"<20201204143955.294320-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201204143955.294320-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Stop camera\n\twhen re-configuring it","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14064,"web_url":"https://patchwork.libcamera.org/comment/14064/","msgid":"<20201204222958.GI4109@pendragon.ideasonboard.com>","date":"2020-12-04T22:29:58","subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Stop camera\n\twhen re-configuring it","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 04, 2020 at 05:08:06PM +0100, Niklas Söderlund wrote:\n> On 2020-12-04 15:39:55 +0100, Jacopo Mondi wrote:\n> > The Android camera device HAL3 specification does not require a\n> > camera to go through any explicit close() call between configurations.\n> > It is legit for a camera to be configured, a number of requests\n> > processed and the re-configured again without any explicit stop.\n> > \n> > The libcamera Android camera HAL starts the Camera at the first handled\n> > request, and only stops it at camera close time. This mean, that two\n> > camera configuration attempts part of the same streaming session are only\n> > interleaved by capture requests handling.\n> > \n> > The libcamera::Camera state machine requires the Camera to be stopped,\n> > before any configuration take place, and this currently doesn't happen\n> > in the camera HAL CameraDevice class.\n> > \n> > Fix this by stopping the camera and the associated worker thread if\n> > a configuration attempt is performed while the Camera is in running\n> > state.\n> > \n> > This patch fixes cros_camera_test:\n> > Camera3PreviewTest/Camera3SinglePreviewTest.Camera3BasicPreviewTest/0\n> > \n> > Signed-off-by: Jacopo Mondi \n> > \n> > ---\n> > Alternatively, the camera can be stopped at the very beginning of the\n> > configureStream() function. But in that case the camera gets stopped\n> > even if the configuration attempt fails (ie at validation time).\n> > \n> > As both behaviour seems to be compliant with what Android expects, I decided\n> > to stop the camera only if the configuration is valid.\n> \n> I'm leaning a bit towards also stopping the camera if the configuration \n> fails, from a power perspective it would it not make most sens? I'm head \n> deep in other PM work so maybe I'm biased ;-) As our current goal is to \n> pass CTS I don't think there is a need to bikesheed on this and this is \n> clearly a step in the right direction,\n\nI'm not sure if there's any test covering this specifically, but if an\ninvalid configuration is requested, I believe Android would still expect\nthe camera to be \"stopped\" and won't queue any further requests. I'm\nthus also leaning towards stopping it earlier, even if it probably\ndoesn't matter too much.\n\nReviewed-by: Laurent Pinchart \n\n> Reviewed-by: Niklas Söderlund \n> \n> > ---\n> > \n> > src/android/camera_device.cpp | 9 ++++++++-\n> > 1 file changed, 8 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 675af5705055..2500994e2f6a 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1341,8 +1341,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > \n> > \t/*\n> > \t * Once the CameraConfiguration has been adjusted/validated\n> > -\t * it can be applied to the camera.\n> > +\t * it can be applied to the camera, which has to be stopped if\n> > +\t * in running state.\n> > \t */\n> > +\tif (running_) {\n> > +\t\tworker_.stop();\n> > +\t\tcamera_->stop();\n> > +\t\trunning_ = false;\n> > +\t}\n> > +\n> > \tint ret = camera_->configure(config_.get());\n> > \tif (ret) {\n> > \t\tLOG(HAL, Error) << \"Failed to configure camera '\"","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 82585BE177\n\tfor ;\n\tFri, 4 Dec 2020 22:30:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFA3A635DE;\n\tFri, 4 Dec 2020 23:30:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59F6B635D0\n\tfor ;\n\tFri, 4 Dec 2020 23:30:00 +0100 (CET)","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 BB9A399A;\n\tFri, 4 Dec 2020 23:29:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EixybvwC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607120999;\n\tbh=pgO2KDCAzTHg5SgHzqbiFYEwVC1exBtlHG3aJbmy5Ek=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EixybvwCLqn00SDFtId+yplB9vuYa1RqJb4EyH2GnSu+X5DEllhpUpE59uVoZQdpJ\n\t5aNRrHG4LloZe1OKQPX/IaD/Dv8iGvxKQRPmRLn02BY8/MNX6RaQ8uyp6eGDvBqELJ\n\t8GUOHghS/b8hBIkbvJZsw45gktGFwAPwkLJ1l0TQ=","Date":"Sat, 5 Dec 2020 00:29:58 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201204222958.GI4109@pendragon.ideasonboard.com>","References":"<20201204143955.294320-1-jacopo@jmondi.org>\n\t<20201204160806.GA2018713@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201204160806.GA2018713@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] android: camera_device: Stop camera\n\twhen re-configuring it","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]