[{"id":20759,"web_url":"https://patchwork.libcamera.org/comment/20759/","msgid":"<163646938276.1606134.18156731669439313658@Monstersaurus>","date":"2021-11-09T14:49:42","subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-28 12:15:18)\n> Since a queue of waiting Requests has been introduced, not all Requests\n> queued to the PipelineHandler are immediately queued to the device.\n> \n> As a Camera can be stopped at any time, it is required to complete the\n> waiting requests after the ones queued to the device had been completed.\n> \n> Introduce a pure virtual PipelineHandler::stopDevice() function to be\n> implemented by pipeline handlers and make the PipelineHandler::stop()\n> function call it before completing pending requests.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/pipeline_handler.h | 3 ++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--\n> src/libcamera/pipeline/simple/simple.cpp | 4 +--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 +--\n> src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--\n> 8 files changed, 39 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index afb7990ea21b..5c3c0bc5a8b3 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -56,7 +56,7 @@ public:\n> std::vector> *buffers) = 0;\n> \n> virtual int start(Camera *camera, const ControlList *controls) = 0;\n> - virtual void stop(Camera *camera) = 0;\n> + void stop(Camera *camera);\n> bool hasPendingRequests(const Camera *camera) const;\n> \n> void queueRequest(Request *request);\n> @@ -71,6 +71,7 @@ protected:\n> void hotplugMediaDevice(MediaDevice *media);\n> \n> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> + virtual void stopDevice(Camera *camera) = 0;\n> \n> CameraManager *manager_;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index eb714aa61099..2b2d7904f275 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -135,7 +135,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> @@ -791,7 +791,7 @@ error:\n> return ret;\n> }\n> \n> -void PipelineHandlerIPU3::stop(Camera *camera)\n> +void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> {\n> IPU3CameraData *data = cameraData(camera);\n> int ret = 0;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 1634ca98f481..b5b7a240d5ed 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -255,7 +255,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> return 0;\n> }\n> \n> -void PipelineHandlerRPi::stop(Camera *camera)\n> +void PipelineHandlerRPi::stopDevice(Camera *camera)\n> {\n> RPiCameraData *data = cameraData(camera);\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 980088628523..9860fa84a5ae 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -146,7 +146,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> return ret;\n> }\n> \n> -void PipelineHandlerRkISP1::stop(Camera *camera)\n> +void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> {\n> RkISP1CameraData *data = cameraData(camera);\n> int ret;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 701fb4be0b71..fdff4ebd5134 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -279,7 +279,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> bool match(DeviceEnumerator *enumerator) override;\n> \n> @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> return 0;\n> }\n> \n> -void SimplePipelineHandler::stop(Camera *camera)\n> +void SimplePipelineHandler::stopDevice(Camera *camera)\n> {\n> SimpleCameraData *data = cameraData(camera);\n> V4L2VideoDevice *video = data->video_;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 264f5370cf32..40654a0bc40c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -74,7 +74,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> return 0;\n> }\n> \n> -void PipelineHandlerUVC::stop(Camera *camera)\n> +void PipelineHandlerUVC::stopDevice(Camera *camera)\n> {\n> UVCCameraData *data = cameraData(camera);\n> data->video_->streamOff();\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index e453091da4b2..29960fe3f038 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -91,7 +91,7 @@ public:\n> std::vector> *buffers) override;\n> \n> int start(Camera *camera, const ControlList *controls) override;\n> - void stop(Camera *camera) override;\n> + void stopDevice(Camera *camera) override;\n> \n> int queueRequestDevice(Camera *camera, Request *request) override;\n> \n> @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> return 0;\n> }\n> \n> -void PipelineHandlerVimc::stop(Camera *camera)\n> +void PipelineHandlerVimc::stopDevice(Camera *camera)\n> {\n> VimcCameraData *data = cameraData(camera);\n> data->video_->streamOff();\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 38edd00cebad..0f9fec1b618f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -268,8 +268,7 @@ void PipelineHandler::unlock()\n> */\n> \n> /**\n> - * \\fn PipelineHandler::stop()\n> - * \\brief Stop capturing from all running streams\n> + * \\brief Stop capturing from all running streams and cancel pending requests\n> * \\param[in] camera The camera to stop\n> *\n> * This function stops capturing and processing requests immediately. All\n> @@ -277,6 +276,30 @@ void PipelineHandler::unlock()\n> *\n> * \\context This function is called from the CameraManager thread.\n> */\n> +void PipelineHandler::stop(Camera *camera)\n> +{\n> + /* Stop the pipeline handler and let the queued requests complete. */\n> + stopDevice(camera);\n> +\n\nI /think/ it's fine - but are we certain that nothing on the\nwaitingRequests_ will get queued between stopDevice above and clearing\nbelow?\n\n\n\n> + /* Cancel and signal as complete all requests waiting for fences. */\n> + MutexLocker lock(waitingRequestsMutex_);\n> +\n> + for (auto &waitingRequest : waitingRequests_) {\n> + waitingRequest->cancel();\n> + completeRequest(waitingRequest);\n> + }\n> +\n> + waitingRequests_.clear();\n\nI'd be very happy to see:\n\n\tCamera::Private *data = camera->_d();\n\tASSERT(data->queuedRequests_.empty());\n\nas an enforcement to be sure that all requests are completed by the end\nof this function.\n\nBut I think I can get away with this on this patch:\n\n\nReviewed-by: Kieran Bingham \n\n\n> +}\n> +\n> +/**\n> + * \\fn PipelineHandler::stopDevice()\n> + * \\brief Stop capturing from all running streams\n> + * \\param[in] camera The camera to stop\n> + *\n> + * This function stops capturing and processing requests immediately. All\n> + * pending requests are cancelled and complete immediately in an error state.\n> + */\n> \n> /**\n> * \\brief Determine if the camera has any requests pending\n> -- \n> 2.33.1\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 77CEDBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 14:49:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D76346034E;\n\tTue, 9 Nov 2021 15:49:46 +0100 (CET)","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 58969600BF\n\tfor ;\n\tTue, 9 Nov 2021 15:49:45 +0100 (CET)","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 E174ADEE;\n\tTue, 9 Nov 2021 15:49:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bSV9l32x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636469385;\n\tbh=ODHsWp0Zm68p0ZWB3UH3+mthWUmICxbp771I/+Go9as=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=bSV9l32xI2ISdXBbUrj6mePdV9D4Cr3SXbSPQMhTp/6nuo8RyrnPafiY6Stvn1z0z\n\t+cqlZz6IyYKP99TyM25QkiLQRKENSVICHIS2stwz+an/Kwf2AxRKmQnsW+7829UDMy\n\tfdZFGnGeRSBt1y4kCTJLaZWGqAIQ5F+8gbMybcqE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-9-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-9-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 14:49:42 +0000","Message-ID":"<163646938276.1606134.18156731669439313658@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20771,"web_url":"https://patchwork.libcamera.org/comment/20771/","msgid":"<20211109174726.bpf2axhenqlirxcx@uno.localdomain>","date":"2021-11-09T17:47:26","subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Tue, Nov 09, 2021 at 02:49:42PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:18)\n> > Since a queue of waiting Requests has been introduced, not all Requests\n> > queued to the PipelineHandler are immediately queued to the device.\n> >\n> > As a Camera can be stopped at any time, it is required to complete the\n> > waiting requests after the ones queued to the device had been completed.\n> >\n> > Introduce a pure virtual PipelineHandler::stopDevice() function to be\n> > implemented by pipeline handlers and make the PipelineHandler::stop()\n> > function call it before completing pending requests.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 3 ++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--\n> > src/libcamera/pipeline/simple/simple.cpp | 4 +--\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--\n> > src/libcamera/pipeline/vimc/vimc.cpp | 4 +--\n> > src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--\n> > 8 files changed, 39 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index afb7990ea21b..5c3c0bc5a8b3 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -56,7 +56,7 @@ public:\n> > std::vector> *buffers) = 0;\n> >\n> > virtual int start(Camera *camera, const ControlList *controls) = 0;\n> > - virtual void stop(Camera *camera) = 0;\n> > + void stop(Camera *camera);\n> > bool hasPendingRequests(const Camera *camera) const;\n> >\n> > void queueRequest(Request *request);\n> > @@ -71,6 +71,7 @@ protected:\n> > void hotplugMediaDevice(MediaDevice *media);\n> >\n> > virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > + virtual void stopDevice(Camera *camera) = 0;\n> >\n> > CameraManager *manager_;\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index eb714aa61099..2b2d7904f275 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -135,7 +135,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > int queueRequestDevice(Camera *camera, Request *request) override;\n> >\n> > @@ -791,7 +791,7 @@ error:\n> > return ret;\n> > }\n> >\n> > -void PipelineHandlerIPU3::stop(Camera *camera)\n> > +void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> > {\n> > IPU3CameraData *data = cameraData(camera);\n> > int ret = 0;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 1634ca98f481..b5b7a240d5ed 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -255,7 +255,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > int queueRequestDevice(Camera *camera, Request *request) override;\n> >\n> > @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > return 0;\n> > }\n> >\n> > -void PipelineHandlerRPi::stop(Camera *camera)\n> > +void PipelineHandlerRPi::stopDevice(Camera *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 980088628523..9860fa84a5ae 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -146,7 +146,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > int queueRequestDevice(Camera *camera, Request *request) override;\n> >\n> > @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > return ret;\n> > }\n> >\n> > -void PipelineHandlerRkISP1::stop(Camera *camera)\n> > +void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > {\n> > RkISP1CameraData *data = cameraData(camera);\n> > int ret;\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 701fb4be0b71..fdff4ebd5134 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -279,7 +279,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > bool match(DeviceEnumerator *enumerator) override;\n> >\n> > @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > return 0;\n> > }\n> >\n> > -void SimplePipelineHandler::stop(Camera *camera)\n> > +void SimplePipelineHandler::stopDevice(Camera *camera)\n> > {\n> > SimpleCameraData *data = cameraData(camera);\n> > V4L2VideoDevice *video = data->video_;\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 264f5370cf32..40654a0bc40c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -74,7 +74,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > int queueRequestDevice(Camera *camera, Request *request) override;\n> >\n> > @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> > return 0;\n> > }\n> >\n> > -void PipelineHandlerUVC::stop(Camera *camera)\n> > +void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > {\n> > UVCCameraData *data = cameraData(camera);\n> > data->video_->streamOff();\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index e453091da4b2..29960fe3f038 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -91,7 +91,7 @@ public:\n> > std::vector> *buffers) override;\n> >\n> > int start(Camera *camera, const ControlList *controls) override;\n> > - void stop(Camera *camera) override;\n> > + void stopDevice(Camera *camera) override;\n> >\n> > int queueRequestDevice(Camera *camera, Request *request) override;\n> >\n> > @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> > return 0;\n> > }\n> >\n> > -void PipelineHandlerVimc::stop(Camera *camera)\n> > +void PipelineHandlerVimc::stopDevice(Camera *camera)\n> > {\n> > VimcCameraData *data = cameraData(camera);\n> > data->video_->streamOff();\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 38edd00cebad..0f9fec1b618f 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -268,8 +268,7 @@ void PipelineHandler::unlock()\n> > */\n> >\n> > /**\n> > - * \\fn PipelineHandler::stop()\n> > - * \\brief Stop capturing from all running streams\n> > + * \\brief Stop capturing from all running streams and cancel pending requests\n> > * \\param[in] camera The camera to stop\n> > *\n> > * This function stops capturing and processing requests immediately. All\n> > @@ -277,6 +276,30 @@ void PipelineHandler::unlock()\n> > *\n> > * \\context This function is called from the CameraManager thread.\n> > */\n> > +void PipelineHandler::stop(Camera *camera)\n> > +{\n> > + /* Stop the pipeline handler and let the queued requests complete. */\n> > + stopDevice(camera);\n> > +\n>\n> I /think/ it's fine - but are we certain that nothing on the\n> waitingRequests_ will get queued between stopDevice above and clearing\n> below?\n\nThe camera state machine shouldn't allow new Requests to be queued if\nthe camera is stopped and the Camera::stop() documentations says\n\n * \\context This function may be called in any camera state as defined in \\ref\n * camera_operation, and shall be synchronized by the caller with other\n * functions that affect the camera state. If called when the camera isn't\n * running, it is a no-op.\n\n>\n>\n>\n> > + /* Cancel and signal as complete all requests waiting for fences. */\n> > + MutexLocker lock(waitingRequestsMutex_);\n> > +\n> > + for (auto &waitingRequest : waitingRequests_) {\n> > + waitingRequest->cancel();\n> > + completeRequest(waitingRequest);\n> > + }\n> > +\n> > + waitingRequests_.clear();\n>\n> I'd be very happy to see:\n>\n> \tCamera::Private *data = camera->_d();\n> \tASSERT(data->queuedRequests_.empty());\n>\n> as an enforcement to be sure that all requests are completed by the end\n> of this function.\n\nAfaict stopDevice() is synchronous, it should complete all requests in\nflight in error state before returning\n>\n> But I think I can get away with this on this patch:\n>\n\nMight be a useful validation to ASSERT though\n\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > +}\n> > +\n> > +/**\n> > + * \\fn PipelineHandler::stopDevice()\n> > + * \\brief Stop capturing from all running streams\n> > + * \\param[in] camera The camera to stop\n> > + *\n> > + * This function stops capturing and processing requests immediately. All\n> > + * pending requests are cancelled and complete immediately in an error state.\n> > + */\n> >\n> > /**\n> > * \\brief Determine if the camera has any requests pending\n> > --\n> > 2.33.1\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 3CAC9BF415\n\tfor ;\n\tTue, 9 Nov 2021 17:46:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63C046034E;\n\tTue, 9 Nov 2021 18:46:34 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 037D5600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:46:32 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 82096FF808;\n\tTue, 9 Nov 2021 17:46:32 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:47:26 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109174726.bpf2axhenqlirxcx@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-9-jacopo@jmondi.org>\n\t<163646938276.1606134.18156731669439313658@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646938276.1606134.18156731669439313658@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20772,"web_url":"https://patchwork.libcamera.org/comment/20772/","msgid":"<163648045291.1716192.13980894585165525849@Monstersaurus>","date":"2021-11-09T17:54:12","subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-09 17:47:26)\n> Hi Kieran\n> \n> On Tue, Nov 09, 2021 at 02:49:42PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:18)\n> > > Since a queue of waiting Requests has been introduced, not all Requests\n> > > queued to the PipelineHandler are immediately queued to the device.\n> > >\n> > > As a Camera can be stopped at any time, it is required to complete the\n> > > waiting requests after the ones queued to the device had been completed.\n> > >\n> > > Introduce a pure virtual PipelineHandler::stopDevice() function to be\n> > > implemented by pipeline handlers and make the PipelineHandler::stop()\n> > > function call it before completing pending requests.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 3 ++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--\n> > > src/libcamera/pipeline/simple/simple.cpp | 4 +--\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +--\n> > > src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--\n> > > 8 files changed, 39 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index afb7990ea21b..5c3c0bc5a8b3 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -56,7 +56,7 @@ public:\n> > > std::vector> *buffers) = 0;\n> > >\n> > > virtual int start(Camera *camera, const ControlList *controls) = 0;\n> > > - virtual void stop(Camera *camera) = 0;\n> > > + void stop(Camera *camera);\n> > > bool hasPendingRequests(const Camera *camera) const;\n> > >\n> > > void queueRequest(Request *request);\n> > > @@ -71,6 +71,7 @@ protected:\n> > > void hotplugMediaDevice(MediaDevice *media);\n> > >\n> > > virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> > > + virtual void stopDevice(Camera *camera) = 0;\n> > >\n> > > CameraManager *manager_;\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index eb714aa61099..2b2d7904f275 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -135,7 +135,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > int queueRequestDevice(Camera *camera, Request *request) override;\n> > >\n> > > @@ -791,7 +791,7 @@ error:\n> > > return ret;\n> > > }\n> > >\n> > > -void PipelineHandlerIPU3::stop(Camera *camera)\n> > > +void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> > > {\n> > > IPU3CameraData *data = cameraData(camera);\n> > > int ret = 0;\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 1634ca98f481..b5b7a240d5ed 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -255,7 +255,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > int queueRequestDevice(Camera *camera, Request *request) override;\n> > >\n> > > @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > return 0;\n> > > }\n> > >\n> > > -void PipelineHandlerRPi::stop(Camera *camera)\n> > > +void PipelineHandlerRPi::stopDevice(Camera *camera)\n> > > {\n> > > RPiCameraData *data = cameraData(camera);\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 980088628523..9860fa84a5ae 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -146,7 +146,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > int queueRequestDevice(Camera *camera, Request *request) override;\n> > >\n> > > @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > return ret;\n> > > }\n> > >\n> > > -void PipelineHandlerRkISP1::stop(Camera *camera)\n> > > +void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> > > {\n> > > RkISP1CameraData *data = cameraData(camera);\n> > > int ret;\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 701fb4be0b71..fdff4ebd5134 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -279,7 +279,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > bool match(DeviceEnumerator *enumerator) override;\n> > >\n> > > @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > return 0;\n> > > }\n> > >\n> > > -void SimplePipelineHandler::stop(Camera *camera)\n> > > +void SimplePipelineHandler::stopDevice(Camera *camera)\n> > > {\n> > > SimpleCameraData *data = cameraData(camera);\n> > > V4L2VideoDevice *video = data->video_;\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 264f5370cf32..40654a0bc40c 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -74,7 +74,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > int queueRequestDevice(Camera *camera, Request *request) override;\n> > >\n> > > @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> > > return 0;\n> > > }\n> > >\n> > > -void PipelineHandlerUVC::stop(Camera *camera)\n> > > +void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > > {\n> > > UVCCameraData *data = cameraData(camera);\n> > > data->video_->streamOff();\n> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > index e453091da4b2..29960fe3f038 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -91,7 +91,7 @@ public:\n> > > std::vector> *buffers) override;\n> > >\n> > > int start(Camera *camera, const ControlList *controls) override;\n> > > - void stop(Camera *camera) override;\n> > > + void stopDevice(Camera *camera) override;\n> > >\n> > > int queueRequestDevice(Camera *camera, Request *request) override;\n> > >\n> > > @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> > > return 0;\n> > > }\n> > >\n> > > -void PipelineHandlerVimc::stop(Camera *camera)\n> > > +void PipelineHandlerVimc::stopDevice(Camera *camera)\n> > > {\n> > > VimcCameraData *data = cameraData(camera);\n> > > data->video_->streamOff();\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index 38edd00cebad..0f9fec1b618f 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -268,8 +268,7 @@ void PipelineHandler::unlock()\n> > > */\n> > >\n> > > /**\n> > > - * \\fn PipelineHandler::stop()\n> > > - * \\brief Stop capturing from all running streams\n> > > + * \\brief Stop capturing from all running streams and cancel pending requests\n> > > * \\param[in] camera The camera to stop\n> > > *\n> > > * This function stops capturing and processing requests immediately. All\n> > > @@ -277,6 +276,30 @@ void PipelineHandler::unlock()\n> > > *\n> > > * \\context This function is called from the CameraManager thread.\n> > > */\n> > > +void PipelineHandler::stop(Camera *camera)\n> > > +{\n> > > + /* Stop the pipeline handler and let the queued requests complete. */\n> > > + stopDevice(camera);\n> > > +\n> >\n> > I /think/ it's fine - but are we certain that nothing on the\n> > waitingRequests_ will get queued between stopDevice above and clearing\n> > below?\n> \n> The camera state machine shouldn't allow new Requests to be queued if\n> the camera is stopped and the Camera::stop() documentations says\n> \n> * \\context This function may be called in any camera state as defined in \\ref\n> * camera_operation, and shall be synchronized by the caller with other\n> * functions that affect the camera state. If called when the camera isn't\n> * running, it is a no-op.\n\nMy concern wasn't so much from external requests being queued, but from\nany potential event triggering (such as a fence completion) that might\nmove a request from the waitingRequests_ to the data->queuedRequests_()\nat this point, before we clear it down...\n\n\n\n> \n> >\n> >\n> >\n> > > + /* Cancel and signal as complete all requests waiting for fences. */\n> > > + MutexLocker lock(waitingRequestsMutex_);\n\nPerhaps taking this lock before calling stopDevice() (if allowed) would\nprevent that possibility?\n\n\n> > > +\n> > > + for (auto &waitingRequest : waitingRequests_) {\n> > > + waitingRequest->cancel();\n> > > + completeRequest(waitingRequest);\n> > > + }\n> > > +\n> > > + waitingRequests_.clear();\n> >\n> > I'd be very happy to see:\n> >\n> > Camera::Private *data = camera->_d();\n> > ASSERT(data->queuedRequests_.empty());\n> >\n> > as an enforcement to be sure that all requests are completed by the end\n> > of this function.\n> \n> Afaict stopDevice() is synchronous, it should complete all requests in\n> flight in error state before returning\n> >\n> > But I think I can get away with this on this patch:\n> >\n> \n> Might be a useful validation to ASSERT though\n> \n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn PipelineHandler::stopDevice()\n> > > + * \\brief Stop capturing from all running streams\n> > > + * \\param[in] camera The camera to stop\n> > > + *\n> > > + * This function stops capturing and processing requests immediately. All\n> > > + * pending requests are cancelled and complete immediately in an error state.\n> > > + */\n> > >\n> > > /**\n> > > * \\brief Determine if the camera has any requests pending\n> > > --\n> > > 2.33.1\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 390E9BDB1C\n\tfor ;\n\tTue, 9 Nov 2021 17:54:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86F656034E;\n\tTue, 9 Nov 2021 18:54:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95C8A600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:54:15 +0100 (CET)","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 2BEECDEE;\n\tTue, 9 Nov 2021 18:54:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cNdX+u7C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636480455;\n\tbh=Y+CKB3rOwbm+PkuV1d2FGdVAJII3QWZBHk+9acYs2hM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=cNdX+u7CkDZmn4a0HITCSJn5POf0kAOrGYdlZPoNLv/vuDvz/WHkm5Ghr4YVqnucj\n\t+z5nygQ3fW8C79yaROljTrNFnbJ5t7juyrS8pNHEZyoOVmKoZuZVB9vmOXG+hJzdRY\n\toXkjIsWeh2/UqL/bhvJRAgFcKgw0egOJ77mXmZd8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109174726.bpf2axhenqlirxcx@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-9-jacopo@jmondi.org>\n\t<163646938276.1606134.18156731669439313658@Monstersaurus>\n\t<20211109174726.bpf2axhenqlirxcx@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Tue, 09 Nov 2021 17:54:12 +0000","Message-ID":"<163648045291.1716192.13980894585165525849@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20802,"web_url":"https://patchwork.libcamera.org/comment/20802/","msgid":"","date":"2021-11-10T13:01:09","subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello,\n\nThe patch looks fine to me as such,\n\nOn 11/9/21 11:24 PM, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-11-09 17:47:26)\n>> Hi Kieran\n>>\n>> On Tue, Nov 09, 2021 at 02:49:42PM +0000, Kieran Bingham wrote:\n>>> Quoting Jacopo Mondi (2021-10-28 12:15:18)\n>>>> Since a queue of waiting Requests has been introduced, not all Requests\n>>>> queued to the PipelineHandler are immediately queued to the device.\n>>>>\n>>>> As a Camera can be stopped at any time, it is required to complete the\n>>>> waiting requests after the ones queued to the device had been completed.\n>>>>\n>>>> Introduce a pure virtual PipelineHandler::stopDevice() function to be\n>>>> implemented by pipeline handlers and make the PipelineHandler::stop()\n>>>> function call it before completing pending requests.\n>>>>\n>>>> Signed-off-by: Jacopo Mondi \n>>>> ---\n>>>> include/libcamera/internal/pipeline_handler.h | 3 ++-\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--\n>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--\n>>>> src/libcamera/pipeline/simple/simple.cpp | 4 +--\n>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--\n>>>> src/libcamera/pipeline/vimc/vimc.cpp | 4 +--\n>>>> src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--\n>>>> 8 files changed, 39 insertions(+), 15 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>> index afb7990ea21b..5c3c0bc5a8b3 100644\n>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>> @@ -56,7 +56,7 @@ public:\n>>>> std::vector> *buffers) = 0;\n>>>>\n>>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n>>>> - virtual void stop(Camera *camera) = 0;\n>>>> + void stop(Camera *camera);\n>>>> bool hasPendingRequests(const Camera *camera) const;\n>>>>\n>>>> void queueRequest(Request *request);\n>>>> @@ -71,6 +71,7 @@ protected:\n>>>> void hotplugMediaDevice(MediaDevice *media);\n>>>>\n>>>> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n>>>> + virtual void stopDevice(Camera *camera) = 0;\n>>>>\n>>>> CameraManager *manager_;\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index eb714aa61099..2b2d7904f275 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -135,7 +135,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> int queueRequestDevice(Camera *camera, Request *request) override;\n>>>>\n>>>> @@ -791,7 +791,7 @@ error:\n>>>> return ret;\n>>>> }\n>>>>\n>>>> -void PipelineHandlerIPU3::stop(Camera *camera)\n>>>> +void PipelineHandlerIPU3::stopDevice(Camera *camera)\n>>>> {\n>>>> IPU3CameraData *data = cameraData(camera);\n>>>> int ret = 0;\n>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> index 1634ca98f481..b5b7a240d5ed 100644\n>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> @@ -255,7 +255,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> int queueRequestDevice(Camera *camera, Request *request) override;\n>>>>\n>>>> @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>>>> return 0;\n>>>> }\n>>>>\n>>>> -void PipelineHandlerRPi::stop(Camera *camera)\n>>>> +void PipelineHandlerRPi::stopDevice(Camera *camera)\n>>>> {\n>>>> RPiCameraData *data = cameraData(camera);\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index 980088628523..9860fa84a5ae 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -146,7 +146,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> int queueRequestDevice(Camera *camera, Request *request) override;\n>>>>\n>>>> @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n>>>> return ret;\n>>>> }\n>>>>\n>>>> -void PipelineHandlerRkISP1::stop(Camera *camera)\n>>>> +void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n>>>> {\n>>>> RkISP1CameraData *data = cameraData(camera);\n>>>> int ret;\n>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>>> index 701fb4be0b71..fdff4ebd5134 100644\n>>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>>> @@ -279,7 +279,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> bool match(DeviceEnumerator *enumerator) override;\n>>>>\n>>>> @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n>>>> return 0;\n>>>> }\n>>>>\n>>>> -void SimplePipelineHandler::stop(Camera *camera)\n>>>> +void SimplePipelineHandler::stopDevice(Camera *camera)\n>>>> {\n>>>> SimpleCameraData *data = cameraData(camera);\n>>>> V4L2VideoDevice *video = data->video_;\n>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> index 264f5370cf32..40654a0bc40c 100644\n>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> @@ -74,7 +74,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> int queueRequestDevice(Camera *camera, Request *request) override;\n>>>>\n>>>> @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n>>>> return 0;\n>>>> }\n>>>>\n>>>> -void PipelineHandlerUVC::stop(Camera *camera)\n>>>> +void PipelineHandlerUVC::stopDevice(Camera *camera)\n>>>> {\n>>>> UVCCameraData *data = cameraData(camera);\n>>>> data->video_->streamOff();\n>>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>>>> index e453091da4b2..29960fe3f038 100644\n>>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>>>> @@ -91,7 +91,7 @@ public:\n>>>> std::vector> *buffers) override;\n>>>>\n>>>> int start(Camera *camera, const ControlList *controls) override;\n>>>> - void stop(Camera *camera) override;\n>>>> + void stopDevice(Camera *camera) override;\n>>>>\n>>>> int queueRequestDevice(Camera *camera, Request *request) override;\n>>>>\n>>>> @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n>>>> return 0;\n>>>> }\n>>>>\n>>>> -void PipelineHandlerVimc::stop(Camera *camera)\n>>>> +void PipelineHandlerVimc::stopDevice(Camera *camera)\n>>>> {\n>>>> VimcCameraData *data = cameraData(camera);\n>>>> data->video_->streamOff();\n>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>> index 38edd00cebad..0f9fec1b618f 100644\n>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>> @@ -268,8 +268,7 @@ void PipelineHandler::unlock()\n>>>> */\n>>>>\n>>>> /**\n>>>> - * \\fn PipelineHandler::stop()\n>>>> - * \\brief Stop capturing from all running streams\n>>>> + * \\brief Stop capturing from all running streams and cancel pending requests\n>>>> * \\param[in] camera The camera to stop\n>>>> *\n>>>> * This function stops capturing and processing requests immediately. All\n>>>> @@ -277,6 +276,30 @@ void PipelineHandler::unlock()\n>>>> *\n>>>> * \\context This function is called from the CameraManager thread.\n>>>> */\n>>>> +void PipelineHandler::stop(Camera *camera)\n>>>> +{\n>>>> + /* Stop the pipeline handler and let the queued requests complete. */\n>>>> + stopDevice(camera);\n>>>> +\n>>> I /think/ it's fine - but are we certain that nothing on the\n>>> waitingRequests_ will get queued between stopDevice above and clearing\n>>> below?\n>> The camera state machine shouldn't allow new Requests to be queued if\n>> the camera is stopped and the Camera::stop() documentations says\n>>\n>> * \\context This function may be called in any camera state as defined in \\ref\n>> * camera_operation, and shall be synchronized by the caller with other\n>> * functions that affect the camera state. If called when the camera isn't\n>> * running, it is a no-op.\n> My concern wasn't so much from external requests being queued, but from\n> any potential event triggering (such as a fence completion) that might\n> move a request from the waitingRequests_ to the data->queuedRequests_()\n> at this point, before we clear it down...\n>\n>\n>\n>>>\n>>>\n>>>> + /* Cancel and signal as complete all requests waiting for fences. */\n>>>> + MutexLocker lock(waitingRequestsMutex_);\n> Perhaps taking this lock before calling stopDevice() (if allowed) would\n> prevent that possibility?\n\n\nAs mentioned in previous patch, I am still un-sure why do we need this \nlock or I don't understand if any other thread is trying to access \nwaitingRequests_ (either reading or queuing to it).\n\n>\n>\n>>>> +\n>>>> + for (auto &waitingRequest : waitingRequests_) {\n>>>> + waitingRequest->cancel();\n>>>> + completeRequest(waitingRequest);\n>>>> + }\n>>>> +\n>>>> + waitingRequests_.clear();\n>>> I'd be very happy to see:\n>>>\n>>> Camera::Private *data = camera->_d();\n>>> ASSERT(data->queuedRequests_.empty());\n>>>\n>>> as an enforcement to be sure that all requests are completed by the end\n>>> of this function.\n>> Afaict stopDevice() is synchronous, it should complete all requests in\n>> flight in error state before returning\n\n\nyes, stopDevice() (earlier's stop()) will complete all request, so it's \nsafe to assume all in flight requests are completed.\n\n>>> But I think I can get away with this on this patch:\n>>>\n>> Might be a useful validation to ASSERT though\n\n\nAgreed, I see no harm (and more tightening)\n\n>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn PipelineHandler::stopDevice()\n>>>> + * \\brief Stop capturing from all running streams\n>>>> + * \\param[in] camera The camera to stop\n>>>> + *\n>>>> + * This function stops capturing and processing requests immediately. All\n>>>> + * pending requests are cancelled and complete immediately in an error state.\n>>>> + */\n>>>>\n>>>> /**\n>>>> * \\brief Determine if the camera has any requests pending\n>>>> --\n>>>> 2.33.1\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 22749BF415\n\tfor ;\n\tWed, 10 Nov 2021 13:01:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A3636035D;\n\tWed, 10 Nov 2021 14:01:16 +0100 (CET)","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 9114E6033C\n\tfor ;\n\tWed, 10 Nov 2021 14:01:14 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D376556;\n\tWed, 10 Nov 2021 14:01:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j3dVbgGk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636549274;\n\tbh=FGpa6e3noK0o8K/2hPYBSC/KtvjXG9Xs+YUOaLDa8go=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=j3dVbgGkM5x2ddFIDVNzFEEF+5XCZNPtf3Hhu0TS6Ll3HEkLIdy1q5AaL5OA2O+rF\n\tnOan2RQt75eBfw9DTGQ70TGB6Pgv+YA/rMZcpT0F94zQCxn5vGix2F3MIPbDQfgHiZ\n\tBtmXi1SWFlgh/hoDi5olJDijZBIJcAiySj3yyF4s=","To":"Kieran Bingham ,\n\tJacopo Mondi ","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-9-jacopo@jmondi.org>\n\t<163646938276.1606134.18156731669439313658@Monstersaurus>\n\t<20211109174726.bpf2axhenqlirxcx@uno.localdomain>\n\t<163648045291.1716192.13980894585165525849@Monstersaurus>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 18:31:09 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<163648045291.1716192.13980894585165525849@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20922,"web_url":"https://patchwork.libcamera.org/comment/20922/","msgid":"","date":"2021-11-12T15:55:56","subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Nov 10, 2021 at 06:31:09PM +0530, Umang Jain wrote:\n> On 11/9/21 11:24 PM, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-11-09 17:47:26)\n> >> On Tue, Nov 09, 2021 at 02:49:42PM +0000, Kieran Bingham wrote:\n> >>> Quoting Jacopo Mondi (2021-10-28 12:15:18)\n> >>>> Since a queue of waiting Requests has been introduced, not all Requests\n> >>>> queued to the PipelineHandler are immediately queued to the device.\n> >>>>\n> >>>> As a Camera can be stopped at any time, it is required to complete the\n> >>>> waiting requests after the ones queued to the device had been completed.\n> >>>>\n> >>>> Introduce a pure virtual PipelineHandler::stopDevice() function to be\n> >>>> implemented by pipeline handlers and make the PipelineHandler::stop()\n> >>>> function call it before completing pending requests.\n> >>>>\n> >>>> Signed-off-by: Jacopo Mondi \n> >>>> ---\n> >>>> include/libcamera/internal/pipeline_handler.h | 3 ++-\n> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--\n> >>>> .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--\n> >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--\n> >>>> src/libcamera/pipeline/simple/simple.cpp | 4 +--\n> >>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--\n> >>>> src/libcamera/pipeline/vimc/vimc.cpp | 4 +--\n> >>>> src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--\n> >>>> 8 files changed, 39 insertions(+), 15 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >>>> index afb7990ea21b..5c3c0bc5a8b3 100644\n> >>>> --- a/include/libcamera/internal/pipeline_handler.h\n> >>>> +++ b/include/libcamera/internal/pipeline_handler.h\n> >>>> @@ -56,7 +56,7 @@ public:\n> >>>> std::vector> *buffers) = 0;\n> >>>>\n> >>>> virtual int start(Camera *camera, const ControlList *controls) = 0;\n> >>>> - virtual void stop(Camera *camera) = 0;\n> >>>> + void stop(Camera *camera);\n> >>>> bool hasPendingRequests(const Camera *camera) const;\n> >>>>\n> >>>> void queueRequest(Request *request);\n> >>>> @@ -71,6 +71,7 @@ protected:\n> >>>> void hotplugMediaDevice(MediaDevice *media);\n> >>>>\n> >>>> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;\n> >>>> + virtual void stopDevice(Camera *camera) = 0;\n> >>>>\n> >>>> CameraManager *manager_;\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> index eb714aa61099..2b2d7904f275 100644\n> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>> @@ -135,7 +135,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> int queueRequestDevice(Camera *camera, Request *request) override;\n> >>>>\n> >>>> @@ -791,7 +791,7 @@ error:\n> >>>> return ret;\n> >>>> }\n> >>>>\n> >>>> -void PipelineHandlerIPU3::stop(Camera *camera)\n> >>>> +void PipelineHandlerIPU3::stopDevice(Camera *camera)\n> >>>> {\n> >>>> IPU3CameraData *data = cameraData(camera);\n> >>>> int ret = 0;\n> >>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> index 1634ca98f481..b5b7a240d5ed 100644\n> >>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>> @@ -255,7 +255,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> int queueRequestDevice(Camera *camera, Request *request) override;\n> >>>>\n> >>>> @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> -void PipelineHandlerRPi::stop(Camera *camera)\n> >>>> +void PipelineHandlerRPi::stopDevice(Camera *camera)\n> >>>> {\n> >>>> RPiCameraData *data = cameraData(camera);\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> index 980088628523..9860fa84a5ae 100644\n> >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>>> @@ -146,7 +146,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> int queueRequestDevice(Camera *camera, Request *request) override;\n> >>>>\n> >>>> @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> >>>> return ret;\n> >>>> }\n> >>>>\n> >>>> -void PipelineHandlerRkISP1::stop(Camera *camera)\n> >>>> +void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> >>>> {\n> >>>> RkISP1CameraData *data = cameraData(camera);\n> >>>> int ret;\n> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >>>> index 701fb4be0b71..fdff4ebd5134 100644\n> >>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >>>> @@ -279,7 +279,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> bool match(DeviceEnumerator *enumerator) override;\n> >>>>\n> >>>> @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> -void SimplePipelineHandler::stop(Camera *camera)\n> >>>> +void SimplePipelineHandler::stopDevice(Camera *camera)\n> >>>> {\n> >>>> SimpleCameraData *data = cameraData(camera);\n> >>>> V4L2VideoDevice *video = data->video_;\n> >>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>>> index 264f5370cf32..40654a0bc40c 100644\n> >>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>>> @@ -74,7 +74,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> int queueRequestDevice(Camera *camera, Request *request) override;\n> >>>>\n> >>>> @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> -void PipelineHandlerUVC::stop(Camera *camera)\n> >>>> +void PipelineHandlerUVC::stopDevice(Camera *camera)\n> >>>> {\n> >>>> UVCCameraData *data = cameraData(camera);\n> >>>> data->video_->streamOff();\n> >>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>>> index e453091da4b2..29960fe3f038 100644\n> >>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>>> @@ -91,7 +91,7 @@ public:\n> >>>> std::vector> *buffers) override;\n> >>>>\n> >>>> int start(Camera *camera, const ControlList *controls) override;\n> >>>> - void stop(Camera *camera) override;\n> >>>> + void stopDevice(Camera *camera) override;\n> >>>>\n> >>>> int queueRequestDevice(Camera *camera, Request *request) override;\n> >>>>\n> >>>> @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> -void PipelineHandlerVimc::stop(Camera *camera)\n> >>>> +void PipelineHandlerVimc::stopDevice(Camera *camera)\n> >>>> {\n> >>>> VimcCameraData *data = cameraData(camera);\n> >>>> data->video_->streamOff();\n> >>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >>>> index 38edd00cebad..0f9fec1b618f 100644\n> >>>> --- a/src/libcamera/pipeline_handler.cpp\n> >>>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>>> @@ -268,8 +268,7 @@ void PipelineHandler::unlock()\n> >>>> */\n> >>>>\n> >>>> /**\n> >>>> - * \\fn PipelineHandler::stop()\n> >>>> - * \\brief Stop capturing from all running streams\n> >>>> + * \\brief Stop capturing from all running streams and cancel pending requests\n> >>>> * \\param[in] camera The camera to stop\n> >>>> *\n> >>>> * This function stops capturing and processing requests immediately. All\n> >>>> @@ -277,6 +276,30 @@ void PipelineHandler::unlock()\n> >>>> *\n> >>>> * \\context This function is called from the CameraManager thread.\n> >>>> */\n> >>>> +void PipelineHandler::stop(Camera *camera)\n> >>>> +{\n> >>>> + /* Stop the pipeline handler and let the queued requests complete. */\n> >>>> + stopDevice(camera);\n> >>>> +\n> >>>\n> >>> I /think/ it's fine - but are we certain that nothing on the\n> >>> waitingRequests_ will get queued between stopDevice above and clearing\n> >>> below?\n> >>\n> >> The camera state machine shouldn't allow new Requests to be queued if\n> >> the camera is stopped and the Camera::stop() documentations says\n> >>\n> >> * \\context This function may be called in any camera state as defined in \\ref\n> >> * camera_operation, and shall be synchronized by the caller with other\n> >> * functions that affect the camera state. If called when the camera isn't\n> >> * running, it is a no-op.\n> >\n> > My concern wasn't so much from external requests being queued, but from\n> > any potential event triggering (such as a fence completion) that might\n> > move a request from the waitingRequests_ to the data->queuedRequests_()\n> > at this point, before we clear it down...\n\nWe're single threaded, so as long as we make sure to really stop\neverything in stop() to prevent any further event from being generated\nonce control returns to the event loop, it should be fine.\n\n> >>>> + /* Cancel and signal as complete all requests waiting for fences. */\n> >>>> + MutexLocker lock(waitingRequestsMutex_);\n> >\n> > Perhaps taking this lock before calling stopDevice() (if allowed) would\n> > prevent that possibility?\n> \n> As mentioned in previous patch, I am still un-sure why do we need this \n> lock or I don't understand if any other thread is trying to access \n> waitingRequests_ (either reading or queuing to it).\n\nI don't think we need the lock.\n\n> >>>> +\n> >>>> + for (auto &waitingRequest : waitingRequests_) {\n> >>>> + waitingRequest->cancel();\n> >>>> + completeRequest(waitingRequest);\n> >>>> + }\n> >>>> +\n> >>>> + waitingRequests_.clear();\n> >>>\n> >>> I'd be very happy to see:\n> >>>\n> >>> Camera::Private *data = camera->_d();\n> >>> ASSERT(data->queuedRequests_.empty());\n> >>>\n> >>> as an enforcement to be sure that all requests are completed by the end\n> >>> of this function.\n> >>\n> >> Afaict stopDevice() is synchronous, it should complete all requests in\n> >> flight in error state before returning\n> \n> yes, stopDevice() (earlier's stop()) will complete all request, so it's \n> safe to assume all in flight requests are completed.\n> \n> >>> But I think I can get away with this on this patch:\n> >>\n> >> Might be a useful validation to ASSERT though\n> \n> Agreed, I see no harm (and more tightening)\n> \n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\fn PipelineHandler::stopDevice()\n> >>>> + * \\brief Stop capturing from all running streams\n> >>>> + * \\param[in] camera The camera to stop\n> >>>> + *\n> >>>> + * This function stops capturing and processing requests immediately. All\n> >>>> + * pending requests are cancelled and complete immediately in an error state.\n> >>>> + */\n> >>>>\n> >>>> /**\n> >>>> * \\brief Determine if the camera has any requests pending","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 8C610BF415\n\tfor ;\n\tFri, 12 Nov 2021 15:56:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2FB86036B;\n\tFri, 12 Nov 2021 16:56:20 +0100 (CET)","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 430046032C\n\tfor ;\n\tFri, 12 Nov 2021 16:56:19 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3A17E7;\n\tFri, 12 Nov 2021 16:56:18 +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=\"wlDMuSPG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636732578;\n\tbh=rc7uP24zKDCx6C4/3lFYzZRkw2iaLsVL6UmuirFrflY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wlDMuSPGQoqgDQ49gvIgHFeKHMqSjzT9UNs8/OsSvE+QaMf8OlYg9tIGeUJvTiymD\n\tIZ4PFneZjgmTFxv78VNOXPOTaxQpvrhGUZiGgsmqBRnxfu07VcwtugEbghKaOKbvnN\n\ttRCcJ+2MVwxj961+RmJ5V8XqHTYowEZe7RbTGzw4=","Date":"Fri, 12 Nov 2021 17:55:56 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-9-jacopo@jmondi.org>\n\t<163646938276.1606134.18156731669439313658@Monstersaurus>\n\t<20211109174726.bpf2axhenqlirxcx@uno.localdomain>\n\t<163648045291.1716192.13980894585165525849@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce\n\tstopDevice()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]