[{"id":24991,"web_url":"https://patchwork.libcamera.org/comment/24991/","msgid":"<166333510987.836189.12070813701569269687@Monstersaurus>","date":"2022-09-16T13:31:49","subject":"Re: [libcamera-devel] [PATCH v1 1/2] pipeline: raspberrypi: Add an\n\terror state","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nQuoting Naushir Patuck via libcamera-devel (2022-09-16 11:05:16)\n> Add an error state used internally in the Raspberry Pi pipeline handler.\n> Currently this state is never set, but will be in a subsequent commit when a\n> device timeout has been signalled.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 18 ++++++++++--------\n> 1 file changed, 10 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index b4094898ca6c..d429cb444d58 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -253,7 +253,7 @@ public:\n> * thread. So, we do not need to have any mutex to protect access to any\n> * of the variables below.\n> */\n> - enum class State { Stopped, Idle, Busy, IpaComplete };\n> + enum class State { Stopped, Idle, Busy, IpaComplete, Error };\n> State state_;\n> \n> struct BayerFrame {\n> @@ -1109,7 +1109,8 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> {\n> RPiCameraData *data = cameraData(camera);\n> \n> - if (data->state_ == RPiCameraData::State::Stopped)\n> + if (data->state_ == RPiCameraData::State::Stopped ||\n> + data->state_ == RPiCameraData::State::Error)\n\nThis is fine I think, but is it worth an 'isRunning()' state helper? \n\nDoes the state match the state machine of the Camera object? (I.e.\nshould we expose the states/helpers more conveniently to the pipeline\nhandlers so that you don't have to manage it separately?\n\n\nAs this updates the existing state machine though, I think this is\nfine...\n\n\nReviewed-by: Kieran Bingham \n\n> return -EINVAL;\n> \n> LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> @@ -1708,7 +1709,7 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> \n> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)\n> {\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> @@ -1744,7 +1745,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &\n> \n> void RPiCameraData::runIsp(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);\n> @@ -1759,7 +1760,7 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> \n> void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> {\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> @@ -1825,7 +1826,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> RPi::Stream *stream = nullptr;\n> int index;\n> \n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> for (RPi::Stream &s : unicam_) {\n> @@ -1864,7 +1865,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \n> void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n> {\n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> @@ -1881,7 +1882,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> RPi::Stream *stream = nullptr;\n> int index;\n> \n> - if (state_ == State::Stopped)\n> + if (state_ == State::Stopped || state_ == State::Error)\n> return;\n> \n> for (RPi::Stream &s : isp_) {\n> @@ -1991,6 +1992,7 @@ void RPiCameraData::handleState()\n> switch (state_) {\n> case State::Stopped:\n> case State::Busy:\n> + case State::Error:\n> break;\n> \n> case State::IpaComplete:\n> -- \n> 2.25.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 A8066C0DA4\n\tfor ;\n\tFri, 16 Sep 2022 13:31:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E724361FB7;\n\tFri, 16 Sep 2022 15:31:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5B2B61FAB\n\tfor ;\n\tFri, 16 Sep 2022 15:31:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40E96DD;\n\tFri, 16 Sep 2022 15:31:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663335114;\n\tbh=3LsSbk7VDximTRd32g5bCjA2W8Yo0D8AeuhEM2YcJkw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=vYtALO1LTGtZ78g5MmQRB2+gO51PCMWIQNHY7GtlWIaG32I65DY/irD6wZZfT9aTz\n\tHT4QxKGm/ipIM03wxm8aKEtAeTgB+n/vRpQRGtsVNoy68yNdEqqFP+zVfwvaWeMIVW\n\teh4P+hkmOjfrXgIrrBr7/IHrqHTDefeDevblHt5zoYQeOrUFiSdY4uJSRwmjWC2rxy\n\tZA2V+//X9+luUOTnXBqw87eGQGtJzofFvpookgxhDL0eGGzK6AXlIePqV//Btbxm6v\n\tSHKqL3laS4PGpf0lllMWdeFIeXgWwbEowU7vZkPQqNlrMQbb22QDPk79H1FqgNqRsK\n\tkIkbfwc9vgUsw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663335112;\n\tbh=3LsSbk7VDximTRd32g5bCjA2W8Yo0D8AeuhEM2YcJkw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=mLv1ALw7N5taow6uCREIllWJoCqeeoF8gJlihrH2XgZ0kkt4a/1XYtCtmIuucBfPe\n\tS5z/uaxSlhOvb7/ta/Jfa1qrfc4XEVejXEmY+ayh9KzDq+CGBJc5/wgqqjPd+onAZ2\n\t6GKCVwYYT5+cXLc2/rSSH+khVNGE2E9Srw54jsCA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mLv1ALw7\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220916100517.12446-2-naush@raspberrypi.com>","References":"<20220916100517.12446-1-naush@raspberrypi.com>\n\t<20220916100517.12446-2-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 16 Sep 2022 14:31:49 +0100","Message-ID":"<166333510987.836189.12070813701569269687@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 1/2] pipeline: raspberrypi: Add an\n\terror state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24993,"web_url":"https://patchwork.libcamera.org/comment/24993/","msgid":"","date":"2022-09-16T13:41:42","subject":"Re: [libcamera-devel] [PATCH v1 1/2] pipeline: raspberrypi: Add an\n\terror state","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThanks for the review!\n\nOn Fri, 16 Sept 2022 at 14:31, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Quoting Naushir Patuck via libcamera-devel (2022-09-16 11:05:16)\n> > Add an error state used internally in the Raspberry Pi pipeline handler.\n> > Currently this state is never set, but will be in a subsequent commit\n> when a\n> > device timeout has been signalled.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 18 ++++++++++--------\n> > 1 file changed, 10 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index b4094898ca6c..d429cb444d58 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -253,7 +253,7 @@ public:\n> > * thread. So, we do not need to have any mutex to protect\n> access to any\n> > * of the variables below.\n> > */\n> > - enum class State { Stopped, Idle, Busy, IpaComplete };\n> > + enum class State { Stopped, Idle, Busy, IpaComplete, Error };\n> > State state_;\n> >\n> > struct BayerFrame {\n> > @@ -1109,7 +1109,8 @@ int PipelineHandlerRPi::queueRequestDevice(Camera\n> *camera, Request *request)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> >\n> > - if (data->state_ == RPiCameraData::State::Stopped)\n> > + if (data->state_ == RPiCameraData::State::Stopped ||\n> > + data->state_ == RPiCameraData::State::Error)\n>\n> This is fine I think, but is it worth an 'isRunning()' state helper?\n\n\nI can do that for v2, it would probably look cleaner!\n\n\n>\n>\n> Does the state match the state machine of the Camera object? (I.e.\n> should we expose the states/helpers more conveniently to the pipeline\n> handlers so that you don't have to manage it separately?\n>\n\nThis state is internal the the rpi pipeline handler really, so not the same\nas the Camera object - although the common start/stop states would\nprobably match. As such, it's not going to help me here if the Camera object\nstate was exposed to us.\n\nRegards,\nNaush\n\n\n>\n> As this updates the existing state machine though, I think this is\n> fine...\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> > return -EINVAL;\n> >\n> > LOG(RPI, Debug) << \"queueRequestDevice: New request.\";\n> > @@ -1708,7 +1709,7 @@ void\n> RPiCameraData::enumerateVideoDevices(MediaLink *link)\n> >\n> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const\n> ControlList &controls)\n> > {\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > @@ -1744,7 +1745,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t\n> bufferId, const ControlList &\n> >\n> > void RPiCameraData::runIsp(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > FrameBuffer *buffer =\n> unicam_[Unicam::Image].getBuffers().at(bufferId);\n> > @@ -1759,7 +1760,7 @@ void RPiCameraData::runIsp(uint32_t bufferId)\n> >\n> > void RPiCameraData::embeddedComplete(uint32_t bufferId)\n> > {\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > FrameBuffer *buffer =\n> unicam_[Unicam::Embedded].getBuffers().at(bufferId);\n> > @@ -1825,7 +1826,7 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > RPi::Stream *stream = nullptr;\n> > int index;\n> >\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > for (RPi::Stream &s : unicam_) {\n> > @@ -1864,7 +1865,7 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> >\n> > void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n> > {\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> > @@ -1881,7 +1882,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer\n> *buffer)\n> > RPi::Stream *stream = nullptr;\n> > int index;\n> >\n> > - if (state_ == State::Stopped)\n> > + if (state_ == State::Stopped || state_ == State::Error)\n> > return;\n> >\n> > for (RPi::Stream &s : isp_) {\n> > @@ -1991,6 +1992,7 @@ void RPiCameraData::handleState()\n> > switch (state_) {\n> > case State::Stopped:\n> > case State::Busy:\n> > + case State::Error:\n> > break;\n> >\n> > case State::IpaComplete:\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 04377C3272\n\tfor ;\n\tFri, 16 Sep 2022 13:42:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 629F36203B;\n\tFri, 16 Sep 2022 15:42:01 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BB5A61FAB\n\tfor ;\n\tFri, 16 Sep 2022 15:41:59 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id o2so33475444lfc.10\n\tfor ;\n\tFri, 16 Sep 2022 06:41:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663335721;\n\tbh=XisEfhD78xP1p47rDHk8apc/AntMC+w4s1wSHizcpoM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=l/ro/SSKeW1K3AfCxKQdyEb83vEMJeeEsNJ9zXsqkg8+xPxe36YR9v/J/FUFZ79TK\n\tOyP1P0O1hTJpZdEpx4TEEOgiDsnWDpxIdgTdTiPTp2nsqLIWdF5yykylmU/aSovhiF\n\tYyAKMyVfZPiyeozqRkATmJW55BEniQhXGCDO60GXDiGkHvxthaL8GgyKHf9eTAObNN\n\tqD22Y5vBqy8c57L0G+EEhOtW5tgWDbr3JEEFL8/DolWVtyO70l96uX8GH2QXvLUAS2\n\tASZmccbeCWEyXPHOwNx0uSTVLXyQdGnbk07TQkYmSPeKRy+heRJaErFY0f89B+hkIM\n\t3vatXanar4xsg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=7wjl4KF2YtDyZrHGg5VV4yXMBh92JP0zF9o1nlLOb4Q=;\n\tb=B3+Ix4KE2jQGZGGMuAAaW15MWUkO37XAwFw1TXPuKbPLZlKMtjy0+nmjmr1kbqQaoF\n\tgbeWZHo2UutNjXSK6dJEhFLcgriqNW9FQ8NsaUL4yKT/o4FvPBGsO0JkIip3f9Y8zPDP\n\tvIxry8ub966UyPjF5tsbcp9hFburKg3Np2byPsCIHa3kOxwoorUiHqHYPk8bSktZ+tJA\n\tiZNOGQoqCGGNXOIX0uy+1ArbjNwU/a6PjrZX2IJEg68y1fLZOXODZskN1FA0kavtUlxV\n\tMTaCscNK7EY1MQ5OBc4beN7KM9a0EALpkcRTugC2QMLXXsZS4m1PHdGSWIFNH1W4ht0W\n\tUBmw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"B3+Ix4KE\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=7wjl4KF2YtDyZrHGg5VV4yXMBh92JP0zF9o1nlLOb4Q=;\n\tb=nCKc8NseyKsQV/Nvsf+UF3OhV84rHQbAE2hBMWwzprVjQpy3/1n0u6rac6ZwDnsxIW\n\tps4tftc3ZVGKJ5vItzEwc3nE0mo60m680KQib2Om+63iimtNxOGpMBWarYkCQa5GJUHs\n\t6ZW0lEShdsYW8/or9OVED0B73vSSmjTA5LgMeli0UidxdPeUV3qXBNbSucH/6LQDlCvT\n\tE96GHp516yBl1YlI1qFienzbpA4mNTmLtGXddGf9GH0YvkixHQ6w097PkNW8D/KjYSKA\n\t7HnDuIWiEfXU2RHinwHxlDnz7bg3yfg0DYoBi9T8zwM/paz56GCHxBR6FltRHOqCm04z\n\tXaPg==","X-Gm-Message-State":"ACrzQf0LkDDZ5dyd1Rc17578sxog8tE/D4FZmm/yD/5Mw+qbR5cq55qR\n\tiHpw0h5bjy4aMycpBLZ3BWCWvlTpRH1qc2iGYmPZEqXgQi8f9g==","X-Google-Smtp-Source":"AMsMyM6ATZMBHacfj0EpLdsPeLARM+dafe3G1G2RIfHT25Zl+ZKLCShJjP1sXKTHnpezFDiJpnU3nJ9TZAhCK3Eo3hE=","X-Received":"by 2002:a05:6512:234a:b0:49d:3155:45a7 with SMTP id\n\tp10-20020a056512234a00b0049d315545a7mr1584402lfu.245.1663335718663;\n\tFri, 16 Sep 2022 06:41:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20220916100517.12446-1-naush@raspberrypi.com>\n\t<20220916100517.12446-2-naush@raspberrypi.com>\n\t<166333510987.836189.12070813701569269687@Monstersaurus>","In-Reply-To":"<166333510987.836189.12070813701569269687@Monstersaurus>","Date":"Fri, 16 Sep 2022 14:41:42 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000a74e0705e8cb86aa\"","Subject":"Re: [libcamera-devel] [PATCH v1 1/2] pipeline: raspberrypi: Add an\n\terror state","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]