[{"id":15637,"web_url":"https://patchwork.libcamera.org/comment/15637/","msgid":"<YEv4iXI45vvx0qMh@bismarck.dyn.berto.se>","date":"2021-03-12T23:26:01","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nOn 2021-03-12 05:47:23 +0000, Kieran Bingham wrote:\n> All requests must have completed before the Camera has fully stopped.\n> \n> Requests completing when the camera is not running represent an internal\n> pipeline handler bug.\n> \n> Trap this event with a fatal error.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nI like this change too, but sure this must break some of our pipelines?  \nEven so they should then be fixed, for this change,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/camera.cpp | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 84edbb8f494d..19fb9eb415b0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1062,11 +1062,11 @@ int Camera::stop()\n>  \n>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>  \n> -\td->setState(Private::CameraConfigured);\n> -\n>  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>  \t\t\t       this);\n>  \n> +\td->setState(Private::CameraConfigured);\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -1079,6 +1079,12 @@ int Camera::stop()\n>   */\n>  void Camera::requestComplete(Request *request)\n>  {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> +\tif (ret < 0)\n> +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n> +\n>  \trequestCompleted.emit(request);\n>  }\n>  \n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 204F2BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Mar 2021 23:26:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FFBA68C68;\n\tSat, 13 Mar 2021 00:26:04 +0100 (CET)","from mail-wr1-x431.google.com (mail-wr1-x431.google.com\n\t[IPv6:2a00:1450:4864:20::431])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA321602E8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Mar 2021 00:26:02 +0100 (CET)","by mail-wr1-x431.google.com with SMTP id f12so5660428wrx.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Mar 2021 15:26:02 -0800 (PST)","from localhost (p54ac5521.dip0.t-ipconnect.de. [84.172.85.33])\n\tby smtp.gmail.com with ESMTPSA id 1sm4035551wmj.2.2021.03.12.15.26.01\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 12 Mar 2021 15:26:02 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"TWrBKBog\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=gtEe3OCfe/0lXiT3Lm7DFbVZiWSoou0EGIcJlFrMHlg=;\n\tb=TWrBKBogR25q0+n4HLzomCYChhtG9JaTS57DTHWL2WGnCdlBWulxzL06LqJc5f/EuM\n\t1ESnd7lZslxatU2sDhr+Df/XlA/2R4avH5tSNBNDxFnXrUnZ/0XXsdsUVh7KBybVpcwb\n\tKnV5bAlO1zbGPCcp7JRKSPp70oZH+Dksj+T1BbqrlACyIj321nkGAk4EuZ2G5jN2W4in\n\tABO3Ax/wa4W9ciBxT9RRGIZReG8q2YkCo5pRE+RmgWJ2ZIMEVAa2KzE7GNwCH1YkTQCX\n\t0UuHWAsZO0Fu/fuqqJO9tBopsVIdE71LbFcFSzgRJ+ZNx1Ll4a0WmAiq9TNgEz8GTrq4\n\t5wzg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=gtEe3OCfe/0lXiT3Lm7DFbVZiWSoou0EGIcJlFrMHlg=;\n\tb=WwytVufZfJF6giL2YafXTgeFeqhAHu9mb3rXywQvGb2pLWpxm0lmRyeMHatvwSpCXs\n\t5r3XzNweg84jgTBt2jRz7Y760fAMEhtJGGtFFARPwgN/c8/Ro85Fvg+SEAkvajMuiqKq\n\thi3+Mc+NDRQxd63r13rvAs7zEi5areGiqrsu5aWUieDAda4oebEZipmfQdzkQD4wHnOo\n\tycp6125S8b7CRCcyN7JTWHtAjlzIub3IzjrJP0W+ax905YgrJP697Tz++76v66RxbkQM\n\tUapdMZo3U2HofMvypDLd1GT4a9F3tqGBZz6h5cUguAYzfCdvYoosSRjSQ2xC2903u9Rp\n\tdiJg==","X-Gm-Message-State":"AOAM5332Pv89GPR9JXxWhuxoS8oF74AxfQAUsx3MYSX1jvsw1Hv1f47a\n\tC38TXdYAMHA6WNK73eiTlTCRFF+uAdvWK/Xk","X-Google-Smtp-Source":"ABdhPJwFta0UZrnuAnsb5qQU3p2X5GKuGSldQ2cQbze9W12AvWc/jmRTQs3Bhc+OanhCI4quSNF1eQ==","X-Received":"by 2002:adf:9bca:: with SMTP id\n\te10mr16341811wrc.364.1615591562406; \n\tFri, 12 Mar 2021 15:26:02 -0800 (PST)","Date":"Sat, 13 Mar 2021 00:26:01 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YEv4iXI45vvx0qMh@bismarck.dyn.berto.se>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210312054727.852622-5-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15647,"web_url":"https://patchwork.libcamera.org/comment/15647/","msgid":"<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>","date":"2021-03-13T09:35:49","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:\n> All requests must have completed before the Camera has fully stopped.\n>\n> Requests completing when the camera is not running represent an internal\n> pipeline handler bug.\n>\n> Trap this event with a fatal error.\n>\n\nAm I wrong or this is actually the other way around ? Marking the\nCamera as Configured -after- the pipeline's stop() has been invoked\nmakes sure the requests that complete with error due to stop() are\nrefused.\n\nIOW stop() is invoked on the pipeline thread, which might complete\nrequests with error asynchronously from the camera state being\nmoved to 'CameraConfigured'. After the Camera state has changed,\nall the requests completed with error will trigger a Fatal.\nAm I missing something obvious ?\n\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/libcamera/camera.cpp | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 84edbb8f494d..19fb9eb415b0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1062,11 +1062,11 @@ int Camera::stop()\n>\n>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>\n> -\td->setState(Private::CameraConfigured);\n> -\n>  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>  \t\t\t       this);\n>\n> +\td->setState(Private::CameraConfigured);\n> +\n>  \treturn 0;\n>  }\n>\n> @@ -1079,6 +1079,12 @@ int Camera::stop()\n>   */\n>  void Camera::requestComplete(Request *request)\n>  {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> +\tif (ret < 0)\n> +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n> +\n>  \trequestCompleted.emit(request);\n>  }\n>\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 26CA6BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 13 Mar 2021 09:35:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67D6868C67;\n\tSat, 13 Mar 2021 10:35:20 +0100 (CET)","from relay13.mail.gandi.net (relay13.mail.gandi.net\n\t[217.70.178.233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32E72602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Mar 2021 10:35:19 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay13.mail.gandi.net (Postfix) with ESMTPSA id 664738000C;\n\tSat, 13 Mar 2021 09:35:18 +0000 (UTC)"],"Date":"Sat, 13 Mar 2021 10:35:49 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210312054727.852622-5-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15660,"web_url":"https://patchwork.libcamera.org/comment/15660/","msgid":"<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","date":"2021-03-13T23:00:04","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","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 Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:\n> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:\n> > All requests must have completed before the Camera has fully stopped.\n> >\n> > Requests completing when the camera is not running represent an internal\n> > pipeline handler bug.\n> >\n> > Trap this event with a fatal error.\n> \n> Am I wrong or this is actually the other way around ? Marking the\n> Camera as Configured -after- the pipeline's stop() has been invoked\n> makes sure the requests that complete with error due to stop() are\n> refused.\n> \n> IOW stop() is invoked on the pipeline thread, which might complete\n> requests with error asynchronously from the camera state being\n> moved to 'CameraConfigured'. After the Camera state has changed,\n> all the requests completed with error will trigger a Fatal.\n> Am I missing something obvious ?\n\nThe call to PipelineHandler::stop() is blocking, which means that the\nstate will only be set back to CameraConfigured once the pipeline\nhandler's stop() function returns. Pipeline handlers are required to\ncomplete all pending requests before returning from stop(), so the\nassertion shouldn't be triggered. Am I missing something ? :-)\n\n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  src/libcamera/camera.cpp | 10 ++++++++--\n> >  1 file changed, 8 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 84edbb8f494d..19fb9eb415b0 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1062,11 +1062,11 @@ int Camera::stop()\n> >\n> >  \tLOG(Camera, Debug) << \"Stopping capture\";\n> >\n> > -\td->setState(Private::CameraConfigured);\n> > -\n> >  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> >  \t\t\t       this);\n> >\n> > +\td->setState(Private::CameraConfigured);\n> > +\n> >  \treturn 0;\n> >  }\n> >\n> > @@ -1079,6 +1079,12 @@ int Camera::stop()\n> >   */\n> >  void Camera::requestComplete(Request *request)\n> >  {\n> > +\tPrivate *const d = LIBCAMERA_D_PTR();\n> > +\n> > +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > +\tif (ret < 0)\n> > +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n\nNitpicking, as there's no Stopped state, I'd write \"stopped\".\n\nAnd you could also write\n\n\tif (d->isAccessAllowed(Private::CameraRunning) < 0)\n\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n\nI also wonder what then happens when a camera is unplugged. Could you\ntry unplugging a UVC camera while streaming ?\n\n> > +\n> >  \trequestCompleted.emit(request);\n> >  }\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AF4F4BD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 13 Mar 2021 23:00:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CAD2C60850;\n\tSun, 14 Mar 2021 00:00:42 +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 EC7086084D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Mar 2021 00:00:40 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 696249AA;\n\tSun, 14 Mar 2021 00:00:39 +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=\"ltaxhxRM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615676439;\n\tbh=D7AHY+SKSyCbzajrxqF+6oCwV0uyo5aGIskG+uMCsk0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ltaxhxRMSvluTB2IpaNXNetwMZajcjxvjYbgbs9nXcFbWl4425i5dbJYJrCJGMEkr\n\tCySUfvSv8qknu5jD1cvU8xWdEJYjhHRbvR73fVPzSeNNQ4SPZjv1Kn+NZVBCq99/dI\n\tOjKNiOqQYZNGIHuYWx+cRpIXr37SVLRdYXohwALo=","Date":"Sun, 14 Mar 2021 01:00:04 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>\n\t<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15673,"web_url":"https://patchwork.libcamera.org/comment/15673/","msgid":"<20210314091037.3gzvod5ji57kipna@uno.localdomain>","date":"2021-03-14T09:10:37","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Mar 14, 2021 at 01:00:04AM +0200, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:\n> > On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:\n> > > All requests must have completed before the Camera has fully stopped.\n> > >\n> > > Requests completing when the camera is not running represent an internal\n> > > pipeline handler bug.\n> > >\n> > > Trap this event with a fatal error.\n> >\n> > Am I wrong or this is actually the other way around ? Marking the\n> > Camera as Configured -after- the pipeline's stop() has been invoked\n> > makes sure the requests that complete with error due to stop() are\n> > refused.\n> >\n> > IOW stop() is invoked on the pipeline thread, which might complete\n> > requests with error asynchronously from the camera state being\n> > moved to 'CameraConfigured'. After the Camera state has changed,\n> > all the requests completed with error will trigger a Fatal.\n> > Am I missing something obvious ?\n>\n> The call to PipelineHandler::stop() is blocking, which means that the\n> state will only be set back to CameraConfigured once the pipeline\n> handler's stop() function returns. Pipeline handlers are required to\n> complete all pending requests before returning from stop(), so the\n> assertion shouldn't be triggered. Am I missing something ? :-)\n>\n\nI was missing the (obvious) fact that stop() is blocking it seems :)\n\nThanks for clearing this out!\n\n> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > ---\n> > >  src/libcamera/camera.cpp | 10 ++++++++--\n> > >  1 file changed, 8 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 84edbb8f494d..19fb9eb415b0 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1062,11 +1062,11 @@ int Camera::stop()\n> > >\n> > >  \tLOG(Camera, Debug) << \"Stopping capture\";\n> > >\n> > > -\td->setState(Private::CameraConfigured);\n> > > -\n> > >  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> > >  \t\t\t       this);\n> > >\n> > > +\td->setState(Private::CameraConfigured);\n> > > +\n> > >  \treturn 0;\n> > >  }\n> > >\n> > > @@ -1079,6 +1079,12 @@ int Camera::stop()\n> > >   */\n> > >  void Camera::requestComplete(Request *request)\n> > >  {\n> > > +\tPrivate *const d = LIBCAMERA_D_PTR();\n> > > +\n> > > +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> > > +\tif (ret < 0)\n> > > +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n>\n> Nitpicking, as there's no Stopped state, I'd write \"stopped\".\n>\n> And you could also write\n>\n> \tif (d->isAccessAllowed(Private::CameraRunning) < 0)\n> \t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n>\n> I also wonder what then happens when a camera is unplugged. Could you\n> try unplugging a UVC camera while streaming ?\n>\n> > > +\n> > >  \trequestCompleted.emit(request);\n> > >  }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 494BEBD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 14 Mar 2021 09:10:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7585860850;\n\tSun, 14 Mar 2021 10:10:09 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CCC6605B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Mar 2021 10:10:07 +0100 (CET)","from uno.localdomain (host-79-22-58-175.retail.telecomitalia.it\n\t[79.22.58.175]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 60AB0C0004;\n\tSun, 14 Mar 2021 09:10:06 +0000 (UTC)"],"X-Originating-IP":"79.22.58.175","Date":"Sun, 14 Mar 2021 10:10:37 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210314091037.3gzvod5ji57kipna@uno.localdomain>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>\n\t<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>\n\t<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15862,"web_url":"https://patchwork.libcamera.org/comment/15862/","msgid":"<8bca6aff-b009-e84e-21b4-8a195a940066@ideasonboard.com>","date":"2021-03-24T12:49:38","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 13/03/2021 23:00, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:\n>> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:\n>>> All requests must have completed before the Camera has fully stopped.\n>>>\n>>> Requests completing when the camera is not running represent an internal\n>>> pipeline handler bug.\n>>>\n>>> Trap this event with a fatal error.\n>>\n>> Am I wrong or this is actually the other way around ? Marking the\n>> Camera as Configured -after- the pipeline's stop() has been invoked\n>> makes sure the requests that complete with error due to stop() are\n>> refused.\n>>\n>> IOW stop() is invoked on the pipeline thread, which might complete\n>> requests with error asynchronously from the camera state being\n>> moved to 'CameraConfigured'. After the Camera state has changed,\n>> all the requests completed with error will trigger a Fatal.\n>> Am I missing something obvious ?\n> \n> The call to PipelineHandler::stop() is blocking, which means that the\n> state will only be set back to CameraConfigured once the pipeline\n> handler's stop() function returns. Pipeline handlers are required to\n> complete all pending requests before returning from stop(), so the\n> assertion shouldn't be triggered. Am I missing something ? :-)\n> \n>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>> ---\n>>>  src/libcamera/camera.cpp | 10 ++++++++--\n>>>  1 file changed, 8 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>> index 84edbb8f494d..19fb9eb415b0 100644\n>>> --- a/src/libcamera/camera.cpp\n>>> +++ b/src/libcamera/camera.cpp\n>>> @@ -1062,11 +1062,11 @@ int Camera::stop()\n>>>\n>>>  \tLOG(Camera, Debug) << \"Stopping capture\";\n>>>\n>>> -\td->setState(Private::CameraConfigured);\n>>> -\n>>>  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n>>>  \t\t\t       this);\n>>>\n>>> +\td->setState(Private::CameraConfigured);\n>>> +\n>>>  \treturn 0;\n>>>  }\n>>>\n>>> @@ -1079,6 +1079,12 @@ int Camera::stop()\n>>>   */\n>>>  void Camera::requestComplete(Request *request)\n>>>  {\n>>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n>>> +\n>>> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n>>> +\tif (ret < 0)\n>>> +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n> \n> Nitpicking, as there's no Stopped state, I'd write \"stopped\".\n> \n> And you could also write\n> \n> \tif (d->isAccessAllowed(Private::CameraRunning) < 0)\n> \t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n\nAs we don't otherwise need this ret value I agree, until ...\n\n> I also wonder what then happens when a camera is unplugged. Could you\n> try unplugging a UVC camera while streaming ?\n\nHrm ... indeed this is a race probably.\n\nWhen a camera is disconnected, it should set disconnected_, which will\nthen causes isAccessAllowed() to return -ENODEV, which would be entirely\npermittable here to still complete the request and pass it back to the\napplication.\n\nHow about:\n\n/* Disconnected cameras are still able to complete requests. */\nret = d->isAccessAllowed(Private::CameraRunning);\nif (ret < 0 && ret != -ENODEV)\n   LOG(Camera, Fatal) << \"Trying to complete a request when stopped\";\n\n\n>>> +\n>>>  \trequestCompleted.emit(request);\n>>>  }\n>>>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0708DC32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 12:49:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D39268D63;\n\tWed, 24 Mar 2021 13:49:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CA62602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 13:49:42 +0100 (CET)","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 D7347580;\n\tWed, 24 Mar 2021 13:49:41 +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=\"sP6KqPOq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616590182;\n\tbh=tEgvAXTaJL9RiftfhFn+9ZR6ZWLv9MG7Lrnu2H8SRLo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=sP6KqPOq0mpOcXaNSSPAcNVbVBowZN/Zh1+xFITiQ2TVOYpBHcBwsYT1nms9kMk26\n\tw6ITNmU4QSOch2quu/LuZNZqxf0Lk2qCxLI+Z//x3ckegLc9wkaD6njLjQ/9y1gUGe\n\tZ/DIGlQTZpaCRwos6b2TNAuIDdEQxgGKj05LNcTg=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>\n\t<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>\n\t<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<8bca6aff-b009-e84e-21b4-8a195a940066@ideasonboard.com>","Date":"Wed, 24 Mar 2021 12:49:38 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15875,"web_url":"https://patchwork.libcamera.org/comment/15875/","msgid":"<YFuGYNimjXLGJ9oX@pendragon.ideasonboard.com>","date":"2021-03-24T18:35:12","subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","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, Mar 24, 2021 at 12:49:38PM +0000, Kieran Bingham wrote:\n> On 13/03/2021 23:00, Laurent Pinchart wrote:\n> > On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:\n> >> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:\n> >>> All requests must have completed before the Camera has fully stopped.\n> >>>\n> >>> Requests completing when the camera is not running represent an internal\n> >>> pipeline handler bug.\n> >>>\n> >>> Trap this event with a fatal error.\n> >>\n> >> Am I wrong or this is actually the other way around ? Marking the\n> >> Camera as Configured -after- the pipeline's stop() has been invoked\n> >> makes sure the requests that complete with error due to stop() are\n> >> refused.\n> >>\n> >> IOW stop() is invoked on the pipeline thread, which might complete\n> >> requests with error asynchronously from the camera state being\n> >> moved to 'CameraConfigured'. After the Camera state has changed,\n> >> all the requests completed with error will trigger a Fatal.\n> >> Am I missing something obvious ?\n> > \n> > The call to PipelineHandler::stop() is blocking, which means that the\n> > state will only be set back to CameraConfigured once the pipeline\n> > handler's stop() function returns. Pipeline handlers are required to\n> > complete all pending requests before returning from stop(), so the\n> > assertion shouldn't be triggered. Am I missing something ? :-)\n> > \n> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>> ---\n> >>>  src/libcamera/camera.cpp | 10 ++++++++--\n> >>>  1 file changed, 8 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>> index 84edbb8f494d..19fb9eb415b0 100644\n> >>> --- a/src/libcamera/camera.cpp\n> >>> +++ b/src/libcamera/camera.cpp\n> >>> @@ -1062,11 +1062,11 @@ int Camera::stop()\n> >>>\n> >>>  \tLOG(Camera, Debug) << \"Stopping capture\";\n> >>>\n> >>> -\td->setState(Private::CameraConfigured);\n> >>> -\n> >>>  \td->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,\n> >>>  \t\t\t       this);\n> >>>\n> >>> +\td->setState(Private::CameraConfigured);\n> >>> +\n> >>>  \treturn 0;\n> >>>  }\n> >>>\n> >>> @@ -1079,6 +1079,12 @@ int Camera::stop()\n> >>>   */\n> >>>  void Camera::requestComplete(Request *request)\n> >>>  {\n> >>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> >>> +\n> >>> +\tint ret = d->isAccessAllowed(Private::CameraRunning);\n> >>> +\tif (ret < 0)\n> >>> +\t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n> > \n> > Nitpicking, as there's no Stopped state, I'd write \"stopped\".\n> > \n> > And you could also write\n> > \n> > \tif (d->isAccessAllowed(Private::CameraRunning) < 0)\n> > \t\tLOG(Camera, Fatal) << \"Trying to complete a request when Stopped\";\n> \n> As we don't otherwise need this ret value I agree, until ...\n> \n> > I also wonder what then happens when a camera is unplugged. Could you\n> > try unplugging a UVC camera while streaming ?\n> \n> Hrm ... indeed this is a race probably.\n> \n> When a camera is disconnected, it should set disconnected_, which will\n> then causes isAccessAllowed() to return -ENODEV, which would be entirely\n> permittable here to still complete the request and pass it back to the\n> application.\n> \n> How about:\n> \n> /* Disconnected cameras are still able to complete requests. */\n> ret = d->isAccessAllowed(Private::CameraRunning);\n> if (ret < 0 && ret != -ENODEV)\n>    LOG(Camera, Fatal) << \"Trying to complete a request when stopped\";\n\nSeems better to me. Another option would be to pas true as the second\nargument of isAccessAllowed().\n\n> >>> +\n> >>>  \trequestCompleted.emit(request);\n> >>>  }\n> >>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 400CFC32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 18:35:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8422068D69;\n\tWed, 24 Mar 2021 19:35:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48B6E602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 19:35:56 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A0833580;\n\tWed, 24 Mar 2021 19:35:55 +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=\"CVBCn0ru\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616610955;\n\tbh=2KyoLdLpl/NIL7gVkC6/WF7EEGbvWCuYuBhp0h3pTNU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CVBCn0ru8cB+mveIZPF4/PK/xrf5JJGVyW6PL9gzHpLCBRGmm2QDMRCk+wLGFej5v\n\t9i79nQh453m2vNW7MQKZf+WfZqDJ8FdxF+zj27WoHbIyWjImzgKAA/mCzT4iSr345Z\n\tPHglxBj31B/BtYIrCXULN2Rlbb4fiMvIhCIVFUow=","Date":"Wed, 24 Mar 2021 20:35:12 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YFuGYNimjXLGJ9oX@pendragon.ideasonboard.com>","References":"<20210312054727.852622-1-kieran.bingham@ideasonboard.com>\n\t<20210312054727.852622-5-kieran.bingham@ideasonboard.com>\n\t<20210313093549.dp7nzyz2xlappxnm@uno.localdomain>\n\t<YE1D9EKZTLTSSiB6@pendragon.ideasonboard.com>\n\t<8bca6aff-b009-e84e-21b4-8a195a940066@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<8bca6aff-b009-e84e-21b4-8a195a940066@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate\n\trequests are completed in Running state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]