[{"id":16388,"web_url":"https://patchwork.libcamera.org/comment/16388/","msgid":"","date":"2021-04-20T17:17:55","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the patch !\n\nOn 20/04/2021 15:07, Kieran Bingham wrote:\n> When the camera manager calls stop on a pipeline, it is expected that\n> the pipeline handler guarantees all requests are returned back to the\n> application before the camera has stopped.\n> \n> Ensure that this guarantee is met by providing an accessor on the\n> pipeline handler to validate that all pending requests are removed.\n> \n> Signed-off-by: Kieran Bingham \nReviewed-by: Jean-Michel Hautbois \n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/camera.cpp | 2 ++\n> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> 3 files changed, 18 insertions(+)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c6454db6b2c4..27d45d8ffc20 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -80,6 +80,7 @@ public:\n> \n> \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n> +\tbool active(const Camera *camera) const;\n> \n> \tvoid queueRequest(Request *request);\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 763f3b9926fd..c3fc3dd91bd6 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1084,6 +1084,8 @@ int Camera::stop()\n> \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> \t\t\t this);\n> \n> +\tASSERT(!d->pipe_->active(this));\n> +\n> \td->setState(Private::CameraConfigured);\n> \n> \treturn 0;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 3b3150bdbbf7..8629e3578f02 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> * \\context This function is called from the CameraManager thread.\n> */\n> \n> +/**\n> + * \\brief Determine if the camera has any requests pending\n> + * \\param[in] camera The camera to check\n> + *\n> + * This method determines if there are any requests queued to the pipeline\n> + * awaiting processing.\n> + *\n> + * \\return True if there are pending requests, or false otherwise\n> + */\n> +bool PipelineHandler::active(const Camera *camera) const\n> +{\n> +\tconst CameraData *data = cameraData(camera);\n> +\treturn !data->queuedRequests_.empty();\n> +}\n> +\n> /**\n> * \\fn PipelineHandler::queueRequest()\n> * \\brief Queue a request\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 4404ABDB15\n\tfor ;\n\tTue, 20 Apr 2021 17:17:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79B7268840;\n\tTue, 20 Apr 2021 19:17:58 +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 7BAE460516\n\tfor ;\n\tTue, 20 Apr 2021 19:17:56 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13] (unknown\n\t[IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 079C4411;\n\tTue, 20 Apr 2021 19:17:56 +0200 (CEST)"],"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=\"rH0aKpQm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618939076;\n\tbh=UbEPVVokWKkt31XT02w/6QrvpTEhAKJoG+3ShQCxoq8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=rH0aKpQm/RepXk4gjKUe8uE/8czPMQl3WAhp2QbQuj0U5XKchsX3Xt/KlMADmwh8p\n\t0Ua9Jf0c/xCoAVeSjwr6vQ7yTJqj/utosdNVxJqakgOsBLZoAOgtia9fS5AVKPXH7A\n\t1MvQyoViE/YnJUAd77vtV08NklBPAVxev+E9n0/c=","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Tue, 20 Apr 2021 19:17:55 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210420130741.236848-2-kieran.bingham@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16417,"web_url":"https://patchwork.libcamera.org/comment/16417/","msgid":"","date":"2021-04-20T22:22:39","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n> When the camera manager calls stop on a pipeline, it is expected that\n> the pipeline handler guarantees all requests are returned back to the\n> application before the camera has stopped.\n> \n> Ensure that this guarantee is met by providing an accessor on the\n> pipeline handler to validate that all pending requests are removed.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/camera.cpp | 2 ++\n> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> 3 files changed, 18 insertions(+)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c6454db6b2c4..27d45d8ffc20 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -80,6 +80,7 @@ public:\n> \n> \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n> +\tbool active(const Camera *camera) const;\n> \n> \tvoid queueRequest(Request *request);\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 763f3b9926fd..c3fc3dd91bd6 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1084,6 +1084,8 @@ int Camera::stop()\n> \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> \t\t\t this);\n> \n> +\tASSERT(!d->pipe_->active(this));\n> +\n> \td->setState(Private::CameraConfigured);\n> \n> \treturn 0;\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 3b3150bdbbf7..8629e3578f02 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> * \\context This function is called from the CameraManager thread.\n> */\n> \n> +/**\n> + * \\brief Determine if the camera has any requests pending\n> + * \\param[in] camera The camera to check\n> + *\n> + * This method determines if there are any requests queued to the pipeline\n> + * awaiting processing.\n> + *\n> + * \\return True if there are pending requests, or false otherwise\n> + */\n> +bool PipelineHandler::active(const Camera *camera) const\n\nHow about naming the function hasPendingRequests() to make it more\nexplicit ?\n\nReviewed-by: Laurent Pinchart \n\n> +{\n> +\tconst CameraData *data = cameraData(camera);\n> +\treturn !data->queuedRequests_.empty();\n> +}\n> +\n> /**\n> * \\fn PipelineHandler::queueRequest()\n> * \\brief Queue a request","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 452A2BDB16\n\tfor ;\n\tTue, 20 Apr 2021 22:22:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFE716883E;\n\tWed, 21 Apr 2021 00:22:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6F4B68839\n\tfor ;\n\tWed, 21 Apr 2021 00:22:43 +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 5F83A45E;\n\tWed, 21 Apr 2021 00:22:43 +0200 (CEST)"],"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=\"v+aSrS6I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618957363;\n\tbh=kp1Y4+f6/PQQnEVPUxo2FpTHXcY83cms+XgCmYaesqY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v+aSrS6I+KFUcqmjZxEB3Pfn0cs71kExpTgY21fYihHTm49e/WZwoc6XhpPCyPmLz\n\te8QsPiVXaxYbpoD8XZmESHAYTI0xCCKa+xiOJ1FXUXrnPL77wSE83FIBbUup6wo/mj\n\tVLPtnJZ07cB6blSg2Fbs+C+gRRJ6/h4rA7k0THxs=","Date":"Wed, 21 Apr 2021 01:22:39 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210420130741.236848-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16424,"web_url":"https://patchwork.libcamera.org/comment/16424/","msgid":"","date":"2021-04-21T04:46:12","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran, thanks for the patch.\n\nOn Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n> > When the camera manager calls stop on a pipeline, it is expected that\n> > the pipeline handler guarantees all requests are returned back to the\n> > application before the camera has stopped.\n> >\n> > Ensure that this guarantee is met by providing an accessor on the\n> > pipeline handler to validate that all pending requests are removed.\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 1 +\n> > src/libcamera/camera.cpp | 2 ++\n> > src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> > 3 files changed, 18 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index c6454db6b2c4..27d45d8ffc20 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -80,6 +80,7 @@ public:\n> >\n> > virtual int start(Camera *camera, const ControlList *controls) = 0;\n> > virtual void stop(Camera *camera) = 0;\n> > + bool active(const Camera *camera) const;\n> >\n> > void queueRequest(Request *request);\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 763f3b9926fd..c3fc3dd91bd6 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1084,6 +1084,8 @@ int Camera::stop()\n> > d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> > this);\n> >\n> > + ASSERT(!d->pipe_->active(this));\n> > +\n> > d->setState(Private::CameraConfigured);\n> >\n> > return 0;\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 3b3150bdbbf7..8629e3578f02 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> > * \\context This function is called from the CameraManager thread.\n> > */\n> >\n> > +/**\n> > + * \\brief Determine if the camera has any requests pending\n> > + * \\param[in] camera The camera to check\n> > + *\n> > + * This method determines if there are any requests queued to the pipeline\n> > + * awaiting processing.\n> > + *\n> > + * \\return True if there are pending requests, or false otherwise\n> > + */\n> > +bool PipelineHandler::active(const Camera *camera) const\n>\n> How about naming the function hasPendingRequests() to make it more\n> explicit ?\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nDo you think to make this a virtual function?\nSince I introduce a pending requests queue in IPU3CameraData, I would\nlike to IPU3PipelineHandler checks data->queuedRequests_ and\ndata->pendingRequests_.empty(), or this should be a virtual function\nof CameraData.\nHow do you think?\n\n-Hiro\n\n> > +{\n> > + const CameraData *data = cameraData(camera);\n> > + return !data->queuedRequests_.empty();\n> > +}\n> > +\n> > /**\n> > * \\fn PipelineHandler::queueRequest()\n> > * \\brief Queue a request\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 53AD7BDB16\n\tfor ;\n\tWed, 21 Apr 2021 04:46:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB82F602C3;\n\tWed, 21 Apr 2021 06:46:24 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5A97602C3\n\tfor ;\n\tWed, 21 Apr 2021 06:46:23 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id z5so11201083edr.11\n\tfor ;\n\tTue, 20 Apr 2021 21:46:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"G+qPcw71\"; dkim-atps=neutral","DKIM-Signature":"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=twMIdm0sIz0TLqyo3tW7K9j+3AWo2+rDjSE3Fe62/ss=;\n\tb=G+qPcw71REHgEtGaJ8YWybIqAC/wQTC8FCBIw/sy3x6uxYejyad27GrAJEhDhLSYtK\n\tm+Fze42Q9krgIj9yrGD79WQEl9qBPVSxeL/wobTg6ywXTtg11Nt2mKJCwJ5X7DHm1mwx\n\tPwl7sqPfiOzpy4vuuVS5OIi3xv3q4iTyl6Ze0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=twMIdm0sIz0TLqyo3tW7K9j+3AWo2+rDjSE3Fe62/ss=;\n\tb=GY8JVEOyzFhb5rvs9+tpe7opwqHdlGPhUMQwfrRpgcyr3pXZaJ3kmKvrWfvC6bjdUh\n\tOIKXiVIO7KT7FzAm1f0IRp2KpEyzlhGd1ULmVPcNxTWK+WkykpVacIjVc6xbFSOfZJA5\n\tVhsxTjYKRTHyyClm6xC9VR6SY4terr9IqYS3LmKmTcEO23nIOWtytfj8xK+3vCdppx1y\n\tOrGQsvY18UwAqK8wMMZHoieP3RwVMdeugzPc+2/aSEgj80mRqa1RV6DFRu7SJ+Bg258G\n\tz/YLTl7mspDmc4yygNYG3SQnDK4qTF0xSwyWWBjQ1hVCYfVFxzMgWbw/BohaHsF6o9jL\n\t9sQw==","X-Gm-Message-State":"AOAM530RUdDA4oGBe81Kyg5sgPX5BSBhpiNurxk8ZAI6PtIvntGyFZwV\n\tlgid5NOocc1dmZgWvNWtAIrgvRkPDAABYXNuNSThmw==","X-Google-Smtp-Source":"ABdhPJyGgzNBf4oAhjAzlXPbNz4d6qWDLg/dB3IpxHh8eV8XaLHmS46Rb8f7wjeAt5NLtchOVxT9CkoyBJh7j70IYjU=","X-Received":"by 2002:a05:6402:176c:: with SMTP id\n\tda12mr35849990edb.325.1618980383275; \n\tTue, 20 Apr 2021 21:46:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 21 Apr 2021 13:46:12 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16431,"web_url":"https://patchwork.libcamera.org/comment/16431/","msgid":"","date":"2021-04-21T08:50:56","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 21/04/2021 05:46, Hirokazu Honda wrote:\n> Hi Kieran, thanks for the patch.\n> \n> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart\n> wrote:\n>>\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n>>> When the camera manager calls stop on a pipeline, it is expected that\n>>> the pipeline handler guarantees all requests are returned back to the\n>>> application before the camera has stopped.\n>>>\n>>> Ensure that this guarantee is met by providing an accessor on the\n>>> pipeline handler to validate that all pending requests are removed.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> include/libcamera/internal/pipeline_handler.h | 1 +\n>>> src/libcamera/camera.cpp | 2 ++\n>>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n>>> 3 files changed, 18 insertions(+)\n>>>\n>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>> index c6454db6b2c4..27d45d8ffc20 100644\n>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>> @@ -80,6 +80,7 @@ public:\n>>>\n>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n>>> virtual void stop(Camera *camera) = 0;\n>>> + bool active(const Camera *camera) const;\n>>>\n>>> void queueRequest(Request *request);\n>>>\n>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>> index 763f3b9926fd..c3fc3dd91bd6 100644\n>>> --- a/src/libcamera/camera.cpp\n>>> +++ b/src/libcamera/camera.cpp\n>>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n>>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>>> this);\n>>>\n>>> + ASSERT(!d->pipe_->active(this));\n>>> +\n>>> d->setState(Private::CameraConfigured);\n>>>\n>>> return 0;\n>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>> index 3b3150bdbbf7..8629e3578f02 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n>>> * \\context This function is called from the CameraManager thread.\n>>> */\n>>>\n>>> +/**\n>>> + * \\brief Determine if the camera has any requests pending\n>>> + * \\param[in] camera The camera to check\n>>> + *\n>>> + * This method determines if there are any requests queued to the pipeline\n>>> + * awaiting processing.\n>>> + *\n>>> + * \\return True if there are pending requests, or false otherwise\n>>> + */\n>>> +bool PipelineHandler::active(const Camera *camera) const\n>>\n>> How about naming the function hasPendingRequests() to make it more\n>> explicit ?\n\nYes, I can rename it.\n\n\n>>\n>> Reviewed-by: Laurent Pinchart \n>>\n> \n> Do you think to make this a virtual function?\n\nYes, it might be sensible to be virtual so that the Pipeline handler can\noverride it if there are more specific things to add.\n\n\n> Since I introduce a pending requests queue in IPU3CameraData, I would\n> like to IPU3PipelineHandler checks data->queuedRequests_ and\n> data->pendingRequests_.empty(), or this should be a virtual function\n> of CameraData.\n> How do you think?\n\nAt the moment, I would think it's better that the Pipeline handler ...\nhandles it. The PH has access to the pipeline specific camera data anyway.\n\nI'll make this\n\nvirtual bool hasPendingRequests(const Camera *camera) const;\n\nin the next version.\n\n--\nKieran\n\n> \n> -Hiro\n> \n>>> +{\n>>> + const CameraData *data = cameraData(camera);\n>>> + return !data->queuedRequests_.empty();\n>>> +}\n>>> +\n>>> /**\n>>> * \\fn PipelineHandler::queueRequest()\n>>> * \\brief Queue a request\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\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 97873BDB15\n\tfor ;\n\tWed, 21 Apr 2021 08:51:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8743868843;\n\tWed, 21 Apr 2021 10:51:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 89F0B602C8\n\tfor ;\n\tWed, 21 Apr 2021 10:51:00 +0200 (CEST)","from [192.168.0.20]\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 EDEFF3EE;\n\tWed, 21 Apr 2021 10:50:59 +0200 (CEST)"],"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=\"RG2UjXgl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618995060;\n\tbh=FhOxsmtje+cqaVcOLx0oGVp9rughDbV91T4w4aKXaHM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=RG2UjXglAtLCF0etBoB0VvQjEOH1yAKsh34ya+FR+T4CWKBtgGYXN/+xVES27wVfs\n\tJ/+JrUMtmKNw1B64UtY5Qpxn0xQaXNbbxjBPT0JaXsCvJTFAh8gXpT8QuhNLoyJb4V\n\tPPAI2s8JKalp7Xzj9d24Llb/Zv1nGSb5K3kcSIfg=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 21 Apr 2021 09:50:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16438,"web_url":"https://patchwork.libcamera.org/comment/16438/","msgid":"","date":"2021-04-21T09:14:20","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:\n> On 21/04/2021 05:46, Hirokazu Honda wrote:\n> > On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:\n> >> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n> >>> When the camera manager calls stop on a pipeline, it is expected that\n> >>> the pipeline handler guarantees all requests are returned back to the\n> >>> application before the camera has stopped.\n> >>>\n> >>> Ensure that this guarantee is met by providing an accessor on the\n> >>> pipeline handler to validate that all pending requests are removed.\n> >>>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> include/libcamera/internal/pipeline_handler.h | 1 +\n> >>> src/libcamera/camera.cpp | 2 ++\n> >>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> >>> 3 files changed, 18 insertions(+)\n> >>>\n> >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >>> index c6454db6b2c4..27d45d8ffc20 100644\n> >>> --- a/include/libcamera/internal/pipeline_handler.h\n> >>> +++ b/include/libcamera/internal/pipeline_handler.h\n> >>> @@ -80,6 +80,7 @@ public:\n> >>>\n> >>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n> >>> virtual void stop(Camera *camera) = 0;\n> >>> + bool active(const Camera *camera) const;\n> >>>\n> >>> void queueRequest(Request *request);\n> >>>\n> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>> index 763f3b9926fd..c3fc3dd91bd6 100644\n> >>> --- a/src/libcamera/camera.cpp\n> >>> +++ b/src/libcamera/camera.cpp\n> >>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n> >>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> >>> this);\n> >>>\n> >>> + ASSERT(!d->pipe_->active(this));\n> >>> +\n> >>> d->setState(Private::CameraConfigured);\n> >>>\n> >>> return 0;\n> >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >>> index 3b3150bdbbf7..8629e3578f02 100644\n> >>> --- a/src/libcamera/pipeline_handler.cpp\n> >>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> >>> * \\context This function is called from the CameraManager thread.\n> >>> */\n> >>>\n> >>> +/**\n> >>> + * \\brief Determine if the camera has any requests pending\n> >>> + * \\param[in] camera The camera to check\n> >>> + *\n> >>> + * This method determines if there are any requests queued to the pipeline\n> >>> + * awaiting processing.\n> >>> + *\n> >>> + * \\return True if there are pending requests, or false otherwise\n> >>> + */\n> >>> +bool PipelineHandler::active(const Camera *camera) const\n> >>\n> >> How about naming the function hasPendingRequests() to make it more\n> >> explicit ?\n> \n> Yes, I can rename it.\n> \n> >> Reviewed-by: Laurent Pinchart \n> > \n> > Do you think to make this a virtual function?\n> \n> Yes, it might be sensible to be virtual so that the Pipeline handler can\n> override it if there are more specific things to add.\n\nWhat use cases do you foresee for this ? With a function called\nhasPendingRequests(), the semantics is clear, and the base\nPipelineHandler class should have all the information it needs. If we\nwant to also take into account more information, the function name may\nnot be a good match anymore. I'd like to make a decision based on use\ncases. If we don't have any yet, I'd keep this function non-virtual, and\npossibly change it later once use cases arise.\n\n> > Since I introduce a pending requests queue in IPU3CameraData, I would\n> > like to IPU3PipelineHandler checks data->queuedRequests_ and\n> > data->pendingRequests_.empty(), or this should be a virtual function\n> > of CameraData.\n> > How do you think?\n> \n> At the moment, I would think it's better that the Pipeline handler ...\n> handles it. The PH has access to the pipeline specific camera data anyway.\n> \n> I'll make this\n> \n> virtual bool hasPendingRequests(const Camera *camera) const;\n> \n> in the next version.\n> \n> >>> +{\n> >>> + const CameraData *data = cameraData(camera);\n> >>> + return !data->queuedRequests_.empty();\n> >>> +}\n> >>> +\n> >>> /**\n> >>> * \\fn PipelineHandler::queueRequest()\n> >>> * \\brief Queue a request","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 E1736BDB16\n\tfor ;\n\tWed, 21 Apr 2021 09:14:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E45168841;\n\tWed, 21 Apr 2021 11:14:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 72356602D1\n\tfor ;\n\tWed, 21 Apr 2021 11:14:25 +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 D7D2A3EE;\n\tWed, 21 Apr 2021 11:14:24 +0200 (CEST)"],"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=\"FxXf1sn7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618996465;\n\tbh=2tRp29E4y+Q2v2aWJyOvpAlpNLDo9Tu2I6KVH1ujLds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FxXf1sn7JNmpkI5DReXP3cnYfozQg1V7JoXoyROrEnNi4RQL8I4dRjt8FdPM1uEqX\n\t23qRyv/VXwWF1wUUBwxqiC/TV3bk+XC078KwDUj7aUYAm219xT3zOJPUWsD0sjjzW4\n\txrGydqUTXvj5EbKllsSL/7vdCNN5OaXPFr0cB5PI=","Date":"Wed, 21 Apr 2021 12:14:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16450,"web_url":"https://patchwork.libcamera.org/comment/16450/","msgid":"<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>","date":"2021-04-21T10:27:09","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 21/04/2021 10:14, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:\n>> On 21/04/2021 05:46, Hirokazu Honda wrote:\n>>> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:\n>>>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n>>>>> When the camera manager calls stop on a pipeline, it is expected that\n>>>>> the pipeline handler guarantees all requests are returned back to the\n>>>>> application before the camera has stopped.\n>>>>>\n>>>>> Ensure that this guarantee is met by providing an accessor on the\n>>>>> pipeline handler to validate that all pending requests are removed.\n>>>>>\n>>>>> Signed-off-by: Kieran Bingham \n>>>>> ---\n>>>>> include/libcamera/internal/pipeline_handler.h | 1 +\n>>>>> src/libcamera/camera.cpp | 2 ++\n>>>>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n>>>>> 3 files changed, 18 insertions(+)\n>>>>>\n>>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>>> index c6454db6b2c4..27d45d8ffc20 100644\n>>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>>> @@ -80,6 +80,7 @@ public:\n>>>>>\n>>>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n>>>>> virtual void stop(Camera *camera) = 0;\n>>>>> + bool active(const Camera *camera) const;\n>>>>>\n>>>>> void queueRequest(Request *request);\n>>>>>\n>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>>>> index 763f3b9926fd..c3fc3dd91bd6 100644\n>>>>> --- a/src/libcamera/camera.cpp\n>>>>> +++ b/src/libcamera/camera.cpp\n>>>>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n>>>>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>>>>> this);\n>>>>>\n>>>>> + ASSERT(!d->pipe_->active(this));\n>>>>> +\n>>>>> d->setState(Private::CameraConfigured);\n>>>>>\n>>>>> return 0;\n>>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>>> index 3b3150bdbbf7..8629e3578f02 100644\n>>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n>>>>> * \\context This function is called from the CameraManager thread.\n>>>>> */\n>>>>>\n>>>>> +/**\n>>>>> + * \\brief Determine if the camera has any requests pending\n>>>>> + * \\param[in] camera The camera to check\n>>>>> + *\n>>>>> + * This method determines if there are any requests queued to the pipeline\n>>>>> + * awaiting processing.\n>>>>> + *\n>>>>> + * \\return True if there are pending requests, or false otherwise\n>>>>> + */\n>>>>> +bool PipelineHandler::active(const Camera *camera) const\n>>>>\n>>>> How about naming the function hasPendingRequests() to make it more\n>>>> explicit ?\n>>\n>> Yes, I can rename it.\n>>\n>>>> Reviewed-by: Laurent Pinchart \n>>>\n>>> Do you think to make this a virtual function?\n>>\n>> Yes, it might be sensible to be virtual so that the Pipeline handler can\n>> override it if there are more specific things to add.\n> \n> What use cases do you foresee for this ? With a function called\n> hasPendingRequests(), the semantics is clear, and the base\n> PipelineHandler class should have all the information it needs. If we\n> want to also take into account more information, the function name may\n> not be a good match anymore. I'd like to make a decision based on use\n> cases. If we don't have any yet, I'd keep this function non-virtual, and\n> possibly change it later once use cases arise.\n\nHave you read this below?\n\n> \n>>> Since I introduce a pending requests queue in IPU3CameraData, I would\n>>> like to IPU3PipelineHandler checks data->queuedRequests_ and\n>>> data->pendingRequests_.empty(), or this should be a virtual function\n>>> of CameraData.\n>>> How do you think?\n\n\nThis is already an extended use case. Hiro is suggesting he will / has\nadded a queuedRequests in the IPU3CameraData (not CameraData).\n\nTo validate that hasPendingRequests() is true, that queue should also be\nchecked.\n\nAs it is IPU3 specific, it needs to be overridden in the IPU3 pipeline\nhandler.\n\nOf course, if this were to be a generic thing, where we track two lists,\npending, and queued in the base CameraData(), then the\nhasPendingRequests() can check both there too.\n\n\n--\nKieran\n\n\n>>\n>> At the moment, I would think it's better that the Pipeline handler ...\n>> handles it. The PH has access to the pipeline specific camera data anyway.\n>>\n>> I'll make this\n>>\n>> virtual bool hasPendingRequests(const Camera *camera) const;\n>>\n>> in the next version.\n>>\n>>>>> +{\n>>>>> + const CameraData *data = cameraData(camera);\n>>>>> + return !data->queuedRequests_.empty();\n>>>>> +}\n>>>>> +\n>>>>> /**\n>>>>> * \\fn PipelineHandler::queueRequest()\n>>>>> * \\brief Queue a request\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 4F950BDB16\n\tfor ;\n\tWed, 21 Apr 2021 10:27:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F4EA68848;\n\tWed, 21 Apr 2021 12:27:14 +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 2DFFD68840\n\tfor ;\n\tWed, 21 Apr 2021 12:27:13 +0200 (CEST)","from [192.168.0.20]\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 778CD3EE;\n\tWed, 21 Apr 2021 12:27:12 +0200 (CEST)"],"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=\"OwdCkPDu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619000832;\n\tbh=n1plVg4cFO2cYUQUCKOBspoJveQbhLFCzCCyn1xNTaE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=OwdCkPDu/SjoHIeYQY/RUG6RsCa4PT6MIxuEFTaQV/+oUdVIgI0kyKFgmKqcw/qQV\n\t/32iNtOXIqdXgzOiTXne105Wdndn4YIXuQcNk15VCB6s+1Xgc8Ud6z5SaDZKYzpIUv\n\tLNs4b6CuiufDUDuW+RmN3L0cYwHBO9OdXFGb82TE=","To":"Laurent Pinchart ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>","Date":"Wed, 21 Apr 2021 11:27:09 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16474,"web_url":"https://patchwork.libcamera.org/comment/16474/","msgid":"","date":"2021-04-22T02:59:55","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Apr 21, 2021 at 11:27:09AM +0100, Kieran Bingham wrote:\n> On 21/04/2021 10:14, Laurent Pinchart wrote:\n> > On Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:\n> >> On 21/04/2021 05:46, Hirokazu Honda wrote:\n> >>> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:\n> >>>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n> >>>>> When the camera manager calls stop on a pipeline, it is expected that\n> >>>>> the pipeline handler guarantees all requests are returned back to the\n> >>>>> application before the camera has stopped.\n> >>>>>\n> >>>>> Ensure that this guarantee is met by providing an accessor on the\n> >>>>> pipeline handler to validate that all pending requests are removed.\n> >>>>>\n> >>>>> Signed-off-by: Kieran Bingham \n> >>>>> ---\n> >>>>> include/libcamera/internal/pipeline_handler.h | 1 +\n> >>>>> src/libcamera/camera.cpp | 2 ++\n> >>>>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> >>>>> 3 files changed, 18 insertions(+)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >>>>> index c6454db6b2c4..27d45d8ffc20 100644\n> >>>>> --- a/include/libcamera/internal/pipeline_handler.h\n> >>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n> >>>>> @@ -80,6 +80,7 @@ public:\n> >>>>>\n> >>>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n> >>>>> virtual void stop(Camera *camera) = 0;\n> >>>>> + bool active(const Camera *camera) const;\n> >>>>>\n> >>>>> void queueRequest(Request *request);\n> >>>>>\n> >>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>>>> index 763f3b9926fd..c3fc3dd91bd6 100644\n> >>>>> --- a/src/libcamera/camera.cpp\n> >>>>> +++ b/src/libcamera/camera.cpp\n> >>>>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n> >>>>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> >>>>> this);\n> >>>>>\n> >>>>> + ASSERT(!d->pipe_->active(this));\n> >>>>> +\n> >>>>> d->setState(Private::CameraConfigured);\n> >>>>>\n> >>>>> return 0;\n> >>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >>>>> index 3b3150bdbbf7..8629e3578f02 100644\n> >>>>> --- a/src/libcamera/pipeline_handler.cpp\n> >>>>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> >>>>> * \\context This function is called from the CameraManager thread.\n> >>>>> */\n> >>>>>\n> >>>>> +/**\n> >>>>> + * \\brief Determine if the camera has any requests pending\n> >>>>> + * \\param[in] camera The camera to check\n> >>>>> + *\n> >>>>> + * This method determines if there are any requests queued to the pipeline\n> >>>>> + * awaiting processing.\n> >>>>> + *\n> >>>>> + * \\return True if there are pending requests, or false otherwise\n> >>>>> + */\n> >>>>> +bool PipelineHandler::active(const Camera *camera) const\n> >>>>\n> >>>> How about naming the function hasPendingRequests() to make it more\n> >>>> explicit ?\n> >>\n> >> Yes, I can rename it.\n> >>\n> >>>> Reviewed-by: Laurent Pinchart \n> >>>\n> >>> Do you think to make this a virtual function?\n> >>\n> >> Yes, it might be sensible to be virtual so that the Pipeline handler can\n> >> override it if there are more specific things to add.\n> > \n> > What use cases do you foresee for this ? With a function called\n> > hasPendingRequests(), the semantics is clear, and the base\n> > PipelineHandler class should have all the information it needs. If we\n> > want to also take into account more information, the function name may\n> > not be a good match anymore. I'd like to make a decision based on use\n> > cases. If we don't have any yet, I'd keep this function non-virtual, and\n> > possibly change it later once use cases arise.\n> \n> Have you read this below?\n> \n> >>> Since I introduce a pending requests queue in IPU3CameraData, I would\n> >>> like to IPU3PipelineHandler checks data->queuedRequests_ and\n> >>> data->pendingRequests_.empty(), or this should be a virtual function\n> >>> of CameraData.\n> >>> How do you think?\n> \n> This is already an extended use case. Hiro is suggesting he will / has\n> added a queuedRequests in the IPU3CameraData (not CameraData).\n> \n> To validate that hasPendingRequests() is true, that queue should also be\n> checked.\n> \n> As it is IPU3 specific, it needs to be overridden in the IPU3 pipeline\n> handler.\n> \n> Of course, if this were to be a generic thing, where we track two lists,\n> pending, and queued in the base CameraData(), then the\n> hasPendingRequests() can check both there too.\n\nI may be missing something, but won't data->queuedRequests_ work in that\ncase too ? It lists all pending requests from a libcamera core point of\nview, regardless of how the pipeline handler handles them.\n\n> >> At the moment, I would think it's better that the Pipeline handler ...\n> >> handles it. The PH has access to the pipeline specific camera data anyway.\n> >>\n> >> I'll make this\n> >>\n> >> virtual bool hasPendingRequests(const Camera *camera) const;\n> >>\n> >> in the next version.\n> >>\n> >>>>> +{\n> >>>>> + const CameraData *data = cameraData(camera);\n> >>>>> + return !data->queuedRequests_.empty();\n> >>>>> +}\n> >>>>> +\n> >>>>> /**\n> >>>>> * \\fn PipelineHandler::queueRequest()\n> >>>>> * \\brief Queue a request","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 DA5A2BDB17\n\tfor ;\n\tThu, 22 Apr 2021 03:00:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6323568847;\n\tThu, 22 Apr 2021 05:00:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7935460514\n\tfor ;\n\tThu, 22 Apr 2021 05:00:00 +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 DB2383EE;\n\tThu, 22 Apr 2021 04:59:59 +0200 (CEST)"],"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=\"OkSM1nvF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619060400;\n\tbh=DyhHYOE/GGZIcG8OTAAXDlUCPQyVmnEyw8bCamcc41E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OkSM1nvFtj1bTMHpHHVmAER096w0L3/kSMhxHUtBNZfT8498nZX3EpqBAaQ735rah\n\tlxZZJ9dWgnGyIHm+0m8U9DiDOhWz+uMKoc79WauKea5Y/qOU/pQnOTLlQXq7ngpoJo\n\tkqKvAWu/phA9OuzkV+CJWQkEH+MqGor10pJpRqUc=","Date":"Thu, 22 Apr 2021 05:59:55 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16475,"web_url":"https://patchwork.libcamera.org/comment/16475/","msgid":"","date":"2021-04-22T03:06:13","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent and Kieran,\n\nOn Thu, Apr 22, 2021 at 12:00 PM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Wed, Apr 21, 2021 at 11:27:09AM +0100, Kieran Bingham wrote:\n> > On 21/04/2021 10:14, Laurent Pinchart wrote:\n> > > On Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:\n> > >> On 21/04/2021 05:46, Hirokazu Honda wrote:\n> > >>> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:\n> > >>>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n> > >>>>> When the camera manager calls stop on a pipeline, it is expected that\n> > >>>>> the pipeline handler guarantees all requests are returned back to the\n> > >>>>> application before the camera has stopped.\n> > >>>>>\n> > >>>>> Ensure that this guarantee is met by providing an accessor on the\n> > >>>>> pipeline handler to validate that all pending requests are removed.\n> > >>>>>\n> > >>>>> Signed-off-by: Kieran Bingham \n> > >>>>> ---\n> > >>>>> include/libcamera/internal/pipeline_handler.h | 1 +\n> > >>>>> src/libcamera/camera.cpp | 2 ++\n> > >>>>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n> > >>>>> 3 files changed, 18 insertions(+)\n> > >>>>>\n> > >>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > >>>>> index c6454db6b2c4..27d45d8ffc20 100644\n> > >>>>> --- a/include/libcamera/internal/pipeline_handler.h\n> > >>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n> > >>>>> @@ -80,6 +80,7 @@ public:\n> > >>>>>\n> > >>>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n> > >>>>> virtual void stop(Camera *camera) = 0;\n> > >>>>> + bool active(const Camera *camera) const;\n> > >>>>>\n> > >>>>> void queueRequest(Request *request);\n> > >>>>>\n> > >>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > >>>>> index 763f3b9926fd..c3fc3dd91bd6 100644\n> > >>>>> --- a/src/libcamera/camera.cpp\n> > >>>>> +++ b/src/libcamera/camera.cpp\n> > >>>>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n> > >>>>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> > >>>>> this);\n> > >>>>>\n> > >>>>> + ASSERT(!d->pipe_->active(this));\n> > >>>>> +\n> > >>>>> d->setState(Private::CameraConfigured);\n> > >>>>>\n> > >>>>> return 0;\n> > >>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > >>>>> index 3b3150bdbbf7..8629e3578f02 100644\n> > >>>>> --- a/src/libcamera/pipeline_handler.cpp\n> > >>>>> +++ b/src/libcamera/pipeline_handler.cpp\n> > >>>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> > >>>>> * \\context This function is called from the CameraManager thread.\n> > >>>>> */\n> > >>>>>\n> > >>>>> +/**\n> > >>>>> + * \\brief Determine if the camera has any requests pending\n> > >>>>> + * \\param[in] camera The camera to check\n> > >>>>> + *\n> > >>>>> + * This method determines if there are any requests queued to the pipeline\n> > >>>>> + * awaiting processing.\n> > >>>>> + *\n> > >>>>> + * \\return True if there are pending requests, or false otherwise\n> > >>>>> + */\n> > >>>>> +bool PipelineHandler::active(const Camera *camera) const\n> > >>>>\n> > >>>> How about naming the function hasPendingRequests() to make it more\n> > >>>> explicit ?\n> > >>\n> > >> Yes, I can rename it.\n> > >>\n> > >>>> Reviewed-by: Laurent Pinchart \n> > >>>\n> > >>> Do you think to make this a virtual function?\n> > >>\n> > >> Yes, it might be sensible to be virtual so that the Pipeline handler can\n> > >> override it if there are more specific things to add.\n> > >\n> > > What use cases do you foresee for this ? With a function called\n> > > hasPendingRequests(), the semantics is clear, and the base\n> > > PipelineHandler class should have all the information it needs. If we\n> > > want to also take into account more information, the function name may\n> > > not be a good match anymore. I'd like to make a decision based on use\n> > > cases. If we don't have any yet, I'd keep this function non-virtual, and\n> > > possibly change it later once use cases arise.\n> >\n> > Have you read this below?\n> >\n> > >>> Since I introduce a pending requests queue in IPU3CameraData, I would\n> > >>> like to IPU3PipelineHandler checks data->queuedRequests_ and\n> > >>> data->pendingRequests_.empty(), or this should be a virtual function\n> > >>> of CameraData.\n> > >>> How do you think?\n> >\n> > This is already an extended use case. Hiro is suggesting he will / has\n> > added a queuedRequests in the IPU3CameraData (not CameraData).\n> >\n> > To validate that hasPendingRequests() is true, that queue should also be\n> > checked.\n> >\n> > As it is IPU3 specific, it needs to be overridden in the IPU3 pipeline\n> > handler.\n> >\n> > Of course, if this were to be a generic thing, where we track two lists,\n> > pending, and queued in the base CameraData(), then the\n> > hasPendingRequests() can check both there too.\n>\n> I may be missing something, but won't data->queuedRequests_ work in that\n> case too ? It lists all pending requests from a libcamera core point of\n> view, regardless of how the pipeline handler handles them.\n>\n\nAh, you're right. Yes, just checking data->queuedRequests_ should work.\nSorry for the wrong comment.\n-Hiro\n\n> > >> At the moment, I would think it's better that the Pipeline handler ...\n> > >> handles it. The PH has access to the pipeline specific camera data anyway.\n> > >>\n> > >> I'll make this\n> > >>\n> > >> virtual bool hasPendingRequests(const Camera *camera) const;\n> > >>\n> > >> in the next version.\n> > >>\n> > >>>>> +{\n> > >>>>> + const CameraData *data = cameraData(camera);\n> > >>>>> + return !data->queuedRequests_.empty();\n> > >>>>> +}\n> > >>>>> +\n> > >>>>> /**\n> > >>>>> * \\fn PipelineHandler::queueRequest()\n> > >>>>> * \\brief Queue a request\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 3E2E0BDB15\n\tfor ;\n\tThu, 22 Apr 2021 03:06:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F0E968847;\n\tThu, 22 Apr 2021 05:06:25 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9235960514\n\tfor ;\n\tThu, 22 Apr 2021 05:06:24 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id g17so50985953edm.6\n\tfor ;\n\tWed, 21 Apr 2021 20:06:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"X5o97ce9\"; dkim-atps=neutral","DKIM-Signature":"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=AND9g9Mx9wNl7oXcztOnvwXp52IuwJa44FhMenKhFIE=;\n\tb=X5o97ce9pIpFJ6wF8S89GJI0wGhd2RDYn1GLW/fJe2YRXp2G5OEMxNOD8OcSSZl39u\n\t8LQRaFqzZLTIaBCZKdDC1vPvIr1f7O3QHcEz3P00NbIh2KwZ3tEbY3tKLFRzVzVFEnWy\n\tGhb1mxms8PsvpH7eal7dLuNA/tacFsXMhrxV0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=AND9g9Mx9wNl7oXcztOnvwXp52IuwJa44FhMenKhFIE=;\n\tb=F6ztvdc7zUhmT8Ldb4Ow4NUt72uR6A4sFuSg28C9tOXNeWPWbz9A/0G9imHPSZnjnK\n\tqgYIB1dIdizhxw90kYaVz/UlGCVJXYj7Li9sYPX7eGxMdBfjZJ2PQ8slGZpaWVVWhQAQ\n\tceSuXxuHmcGDfwc2wemKPdg5JQEjGZF5Kn0cye7tBVe+QGQ1cXigbmHycUzcUNDGxdm5\n\tTjeXgvjsbewae0m5Hk9aH/2WoA+LFNqqqEJ6qUhkIsgSs1Hpf249yDNgrKxuWJg1+NKv\n\tOl0JjOFMlOOQUbT0CISJyO8xWHkZFjCR2RgOzrPNAfq+5PAegSKXBDD3S36/vjYvMgvI\n\tzm6A==","X-Gm-Message-State":"AOAM530cvI1nRcWXMzkla2Rv0PwzTNwGBL7ZOCNlEeRS7CasNxCR77Uo\n\tk7jzUjuY/KWnN+GIUH9WmBNhtISUPRGuXLy5kObrsgahAyE=","X-Google-Smtp-Source":"ABdhPJxT3ZDL+5mhIf1x3r6e07766Ff0o3s79xd6kVYuaGquhKoK6ypkS1yJy+30ubfimGTRLXUBEkTX8Ez2c73sIrI=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr1102300edv.233.1619060784154; \n\tWed, 21 Apr 2021 20:06:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 22 Apr 2021 12:06:13 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16510,"web_url":"https://patchwork.libcamera.org/comment/16510/","msgid":"<436ae1ff-a9c6-646e-e40e-0b378b27c5b8@ideasonboard.com>","date":"2021-04-22T10:05:52","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 22/04/2021 04:06, Hirokazu Honda wrote:\n> Hi Laurent and Kieran,\n> \n> On Thu, Apr 22, 2021 at 12:00 PM Laurent Pinchart\n> wrote:\n>>\n>> Hi Kieran,\n>>\n>> On Wed, Apr 21, 2021 at 11:27:09AM +0100, Kieran Bingham wrote:\n>>> On 21/04/2021 10:14, Laurent Pinchart wrote:\n>>>> On Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:\n>>>>> On 21/04/2021 05:46, Hirokazu Honda wrote:\n>>>>>> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:\n>>>>>>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:\n>>>>>>>> When the camera manager calls stop on a pipeline, it is expected that\n>>>>>>>> the pipeline handler guarantees all requests are returned back to the\n>>>>>>>> application before the camera has stopped.\n>>>>>>>>\n>>>>>>>> Ensure that this guarantee is met by providing an accessor on the\n>>>>>>>> pipeline handler to validate that all pending requests are removed.\n>>>>>>>>\n>>>>>>>> Signed-off-by: Kieran Bingham \n>>>>>>>> ---\n>>>>>>>> include/libcamera/internal/pipeline_handler.h | 1 +\n>>>>>>>> src/libcamera/camera.cpp | 2 ++\n>>>>>>>> src/libcamera/pipeline_handler.cpp | 15 +++++++++++++++\n>>>>>>>> 3 files changed, 18 insertions(+)\n>>>>>>>>\n>>>>>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>>>>>> index c6454db6b2c4..27d45d8ffc20 100644\n>>>>>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>>>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>>>>>> @@ -80,6 +80,7 @@ public:\n>>>>>>>>\n>>>>>>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n>>>>>>>> virtual void stop(Camera *camera) = 0;\n>>>>>>>> + bool active(const Camera *camera) const;\n>>>>>>>>\n>>>>>>>> void queueRequest(Request *request);\n>>>>>>>>\n>>>>>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>>>>>>> index 763f3b9926fd..c3fc3dd91bd6 100644\n>>>>>>>> --- a/src/libcamera/camera.cpp\n>>>>>>>> +++ b/src/libcamera/camera.cpp\n>>>>>>>> @@ -1084,6 +1084,8 @@ int Camera::stop()\n>>>>>>>> d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>>>>>>>> this);\n>>>>>>>>\n>>>>>>>> + ASSERT(!d->pipe_->active(this));\n>>>>>>>> +\n>>>>>>>> d->setState(Private::CameraConfigured);\n>>>>>>>>\n>>>>>>>> return 0;\n>>>>>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>>>>>> index 3b3150bdbbf7..8629e3578f02 100644\n>>>>>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>>>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>>>>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n>>>>>>>> * \\context This function is called from the CameraManager thread.\n>>>>>>>> */\n>>>>>>>>\n>>>>>>>> +/**\n>>>>>>>> + * \\brief Determine if the camera has any requests pending\n>>>>>>>> + * \\param[in] camera The camera to check\n>>>>>>>> + *\n>>>>>>>> + * This method determines if there are any requests queued to the pipeline\n>>>>>>>> + * awaiting processing.\n>>>>>>>> + *\n>>>>>>>> + * \\return True if there are pending requests, or false otherwise\n>>>>>>>> + */\n>>>>>>>> +bool PipelineHandler::active(const Camera *camera) const\n>>>>>>>\n>>>>>>> How about naming the function hasPendingRequests() to make it more\n>>>>>>> explicit ?\n>>>>>\n>>>>> Yes, I can rename it.\n>>>>>\n>>>>>>> Reviewed-by: Laurent Pinchart \n>>>>>>\n>>>>>> Do you think to make this a virtual function?\n>>>>>\n>>>>> Yes, it might be sensible to be virtual so that the Pipeline handler can\n>>>>> override it if there are more specific things to add.\n>>>>\n>>>> What use cases do you foresee for this ? With a function called\n>>>> hasPendingRequests(), the semantics is clear, and the base\n>>>> PipelineHandler class should have all the information it needs. If we\n>>>> want to also take into account more information, the function name may\n>>>> not be a good match anymore. I'd like to make a decision based on use\n>>>> cases. If we don't have any yet, I'd keep this function non-virtual, and\n>>>> possibly change it later once use cases arise.\n>>>\n>>> Have you read this below?\n>>>\n>>>>>> Since I introduce a pending requests queue in IPU3CameraData, I would\n>>>>>> like to IPU3PipelineHandler checks data->queuedRequests_ and\n>>>>>> data->pendingRequests_.empty(), or this should be a virtual function\n>>>>>> of CameraData.\n>>>>>> How do you think?\n>>>\n>>> This is already an extended use case. Hiro is suggesting he will / has\n>>> added a queuedRequests in the IPU3CameraData (not CameraData).\n>>>\n>>> To validate that hasPendingRequests() is true, that queue should also be\n>>> checked.\n>>>\n>>> As it is IPU3 specific, it needs to be overridden in the IPU3 pipeline\n>>> handler.\n>>>\n>>> Of course, if this were to be a generic thing, where we track two lists,\n>>> pending, and queued in the base CameraData(), then the\n>>> hasPendingRequests() can check both there too.\n>>\n>> I may be missing something, but won't data->queuedRequests_ work in that\n>> case too ? It lists all pending requests from a libcamera core point of\n>> view, regardless of how the pipeline handler handles them.\n>>\n> \n> Ah, you're right. Yes, just checking data->queuedRequests_ should work.\n> Sorry for the wrong comment.\n\nAyeee of course. I'll do the rename, but keep is as a non-virtual member.\n\n--\nKieran\n\n\n> -Hiro\n> \n>>>>> At the moment, I would think it's better that the Pipeline handler ...\n>>>>> handles it. The PH has access to the pipeline specific camera data anyway.\n>>>>>\n>>>>> I'll make this\n>>>>>\n>>>>> virtual bool hasPendingRequests(const Camera *camera) const;\n>>>>>\n>>>>> in the next version.\n>>>>>\n>>>>>>>> +{\n>>>>>>>> + const CameraData *data = cameraData(camera);\n>>>>>>>> + return !data->queuedRequests_.empty();\n>>>>>>>> +}\n>>>>>>>> +\n>>>>>>>> /**\n>>>>>>>> * \\fn PipelineHandler::queueRequest()\n>>>>>>>> * \\brief Queue a request\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart","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 30CDEBDB15\n\tfor ;\n\tThu, 22 Apr 2021 10:05:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A238060516;\n\tThu, 22 Apr 2021 12:05:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C71160514\n\tfor ;\n\tThu, 22 Apr 2021 12:05:55 +0200 (CEST)","from [192.168.0.20]\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 07B5B3EE;\n\tThu, 22 Apr 2021 12:05:54 +0200 (CEST)"],"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=\"WEo+eqEJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619085955;\n\tbh=oo+cN6dCRYz8a0V7U99L3DqK1WAuOGoF89KDOBLIku4=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WEo+eqEJqDhe4afJftqPkjsW8lxCCWrnvUenmKj+qZVA9g9KeWYqkVr+1JHQ2SePo\n\tdvpsMtp9LuhYfykRXJ+UPYRovZ+TmmhTJcOXzlS/noWQzZLcd9aNQUq3I+7EeYwxd0\n\t/rfu2uHUz5twA8Qc1VpXu26wIYmnz1RWW48MIhck=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-2-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<905392e0-43af-cc6b-8e5b-3847fa7433bd@ideasonboard.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<436ae1ff-a9c6-646e-e40e-0b378b27c5b8@ideasonboard.com>","Date":"Thu, 22 Apr 2021 11:05:52 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert\n\tpipelines complete all requests","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]