[{"id":22362,"web_url":"https://patchwork.libcamera.org/comment/22362/","msgid":"","date":"2022-03-22T21:51:09","subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and\n> connect the timeout signal to a slot in the pipeline handler. This slot will\n> log a fatal error message informing the user of a possible hardware stall.\n> \n> The timeout is calculated as 2x the maximum frame length possible for a given\n> mode, returned by the IPA.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++\n> 3 files changed, 18 insertions(+)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index acd3cafe6c91..5a228b75cd2f 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -41,6 +41,7 @@ struct IPAConfig {\n> struct StartConfig {\n> \tlibcamera.ControlList controls;\n> \tint32 dropFrameCount;\n> +\tuint32 maxSensorFrameLengthMs;\n\nWe really need duration types in mojo.\n\n> };\n> \n> interface IPARPiInterface {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index fd8fecb07f81..675f9ba1b350 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> \t}\n> \n> \tstartConfig->dropFrameCount = dropFrameCount_;\n> +\tconst Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> +\tstartConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get();\n> \n> \tfirstStart_ = false;\n> \tlastRunTimestamp_ = 0;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index c2230199fed7..50b39f1adf93 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -45,6 +45,8 @@\n> #include \"dma_heaps.h\"\n> #include \"rpi_stream.h\"\n> \n> +using namespace std::literals::chrono_literals;\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RPI)\n> @@ -202,6 +204,7 @@ public:\n> \tvoid setIspControls(const ControlList &controls);\n> \tvoid setDelayedControls(const ControlList &controls);\n> \tvoid setSensorControls(ControlList &controls);\n> +\tvoid unicamTimeout(V4L2VideoDevice *dev);\n> \n> \t/* bufferComplete signal handlers. */\n> \tvoid unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \t\t}\n> \t}\n> \n> +\t/* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> +\tutils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;\n> +\tdata->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);\n> +\tdata->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);\n> +\tLOG(RPI, Debug) << \"Setting sensor timeout to \" << duration;\n> +\n> \treturn 0;\n> }\n> \n> @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)\n> \tRPiCameraData *data = cameraData(camera);\n> \n> \tdata->state_ = RPiCameraData::State::Stopped;\n> +\tdata->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();\n\nNo need to connect and disconnect the signal when starting and stopping,\nyou can connect it at init time and keep it connected forever.\n\n> \n> \t/* Disable SOF event generation. */\n> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);\n> @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> \tsensor_->setControls(&controls);\n> }\n> \n> +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)\n> +{\n> +\tLOG(RPI, Fatal) << \"Unicam has timed out!\";\n\nThat's harsh, and is likely to trigger if we ever miss a frame due to\nhigh CPU load. If the goal is to detect a complete stall, I'd increase\nthe timeout to at least 10 frames.\n\nHow will this work with sensors that use an external trigger ?\n\n> +}\n> +\n> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> {\n> \tRPi::Stream *stream = nullptr;","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 5C4B1C0F1B\n\tfor ;\n\tTue, 22 Mar 2022 21:51:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9D6C604DB;\n\tTue, 22 Mar 2022 22:51:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26A52604C5\n\tfor ;\n\tTue, 22 Mar 2022 22:51:28 +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 98182DFA;\n\tTue, 22 Mar 2022 22:51:26 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647985889;\n\tbh=mNQLSHarEDm0xy6IZzTp5HCjb6SmYgRimUExD3pqOqM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=bnPfohG32eAMOZQgQOq02KAdq1v37yD/yhEh3jMevw0QqpYbmRQgCLdrlTHBT9m6c\n\tDK5I+WHJyvrsQjl89XaJ89tysq70DlTvT01GMXDkDN6wy+QkGxiNpUO9QC8MfniwfS\n\tgAmVeXJ1LG37fRRxy86dyEPWHTTktgAck9t9s3KKX/W0tfF+Mg3rMcrcjz4VmaVFz4\n\tMaHHgXMRG1sXnblyjMOJUzRV3ReKY+L9ksL/DhQsHp6ZLkQx6BCPZvoGwGHWTRNImL\n\tnueQoQ9kd6ucLb3XiIdPf+ugjloPlvKNsWgsJPzrTpRVfbt/iXHdtbURj3Yw8BEv+O\n\thnKCKLi9pI3mQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647985886;\n\tbh=mNQLSHarEDm0xy6IZzTp5HCjb6SmYgRimUExD3pqOqM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BXsG+/vqIIUF8PAbXoHDWZyHuB3l6B7r/IpUGKENtjCj2tgCXtu3buXocIlNKrL00\n\te1jFhdrNRJNizLwBynwSJQCm6V+XiP8ITidl1VNbt3UlA5N3UrsYEo0DypnvWb5ViT\n\tGcx4vwIypR21JsnIsY5oiPr9C/+YYdWYAvUdmk8E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BXsG+/vq\"; dkim-atps=neutral","Date":"Tue, 22 Mar 2022 23:51:09 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220322131635.2869526-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22388,"web_url":"https://patchwork.libcamera.org/comment/22388/","msgid":"","date":"2022-03-23T09:16:51","subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 22 Mar 2022 at 21:51, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and\n> > connect the timeout signal to a slot in the pipeline handler. This slot\n> will\n> > log a fatal error message informing the user of a possible hardware\n> stall.\n> >\n> > The timeout is calculated as 2x the maximum frame length possible for a\n> given\n> > mode, returned by the IPA.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++\n> > 3 files changed, 18 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index acd3cafe6c91..5a228b75cd2f 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -41,6 +41,7 @@ struct IPAConfig {\n> > struct StartConfig {\n> > libcamera.ControlList controls;\n> > int32 dropFrameCount;\n> > + uint32 maxSensorFrameLengthMs;\n>\n> We really need duration types in mojo.\n>\n\nYes please!! :-)\n\n\n>\n> > };\n> >\n> > interface IPARPiInterface {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index fd8fecb07f81..675f9ba1b350 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n> > }\n> >\n> > startConfig->dropFrameCount = dropFrameCount_;\n> > + const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n> > + startConfig->maxSensorFrameLengthMs =\n> maxSensorFrameDuration.get();\n> >\n> > firstStart_ = false;\n> > lastRunTimestamp_ = 0;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index c2230199fed7..50b39f1adf93 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -45,6 +45,8 @@\n> > #include \"dma_heaps.h\"\n> > #include \"rpi_stream.h\"\n> >\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(RPI)\n> > @@ -202,6 +204,7 @@ public:\n> > void setIspControls(const ControlList &controls);\n> > void setDelayedControls(const ControlList &controls);\n> > void setSensorControls(ControlList &controls);\n> > + void unicamTimeout(V4L2VideoDevice *dev);\n> >\n> > /* bufferComplete signal handlers. */\n> > void unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > }\n> > }\n> >\n> > + /* Set the dequeue timeout to 2x the maximum possible frame\n> duration. */\n> > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2\n> * 1ms;\n> > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);\n> > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data,\n> &RPiCameraData::unicamTimeout);\n> > + LOG(RPI, Debug) << \"Setting sensor timeout to \" << duration;\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)\n> > RPiCameraData *data = cameraData(camera);\n> >\n> > data->state_ = RPiCameraData::State::Stopped;\n> > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();\n>\n> No need to connect and disconnect the signal when starting and stopping,\n> you can connect it at init time and keep it connected forever.\n>\n> >\n> > /* Disable SOF event generation. */\n> > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);\n> > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList\n> &controls)\n> > sensor_->setControls(&controls);\n> > }\n> >\n> > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)\n> > +{\n> > + LOG(RPI, Fatal) << \"Unicam has timed out!\";\n>\n> That's harsh, and is likely to trigger if we ever miss a frame due to\n> high CPU load. If the goal is to detect a complete stall, I'd increase\n> the timeout to at least 10 frames.\n>\n\nYes, you are right.\n\nI actually meant to use Error here - Fatal was a bit leftover from my\ntesting\nto see the timeout hit as expected.\n\n\n>\n> How will this work with sensors that use an external trigger ?\n>\n\nShort answer is it won't and cannot right now. Given this is a purely\ndebug feature,\nthe only effect it will have is to dump a log message to the console which\nI think is fine.\n\nIn the longer term, perhaps we need to be informed (through the v4l2\ndriver?) that we\nare in \"bulb\" mode and we must disable timeouts. Or perhaps the application\ncan pass\na config flag?\n\nRegards,\nNaush\n\n\n>\n> > +}\n> > +\n> > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > {\n> > RPi::Stream *stream = nullptr;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 09511C0F1B\n\tfor ;\n\tWed, 23 Mar 2022 09:17:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 506B6604C6;\n\tWed, 23 Mar 2022 10:17:10 +0100 (CET)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 650D0601F8\n\tfor ;\n\tWed, 23 Mar 2022 10:17:08 +0100 (CET)","by mail-lj1-x22c.google.com with SMTP id 17so961786ljw.8\n\tfor ;\n\tWed, 23 Mar 2022 02:17:08 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648027030;\n\tbh=bKrqNEULGDov2jDKi7Y5Ch2LACl2v6T7qDa8+Fl2P+s=;\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=4I4UUu0tJ+koorIKBwUN8F/p2d/NzBx68ltiTLFBcpW2NPpDXdHw1uYwSfp/qFT3g\n\t1dsNT5bbrItd3AK+H3UQCOLeUkvKJFbe3PoxgR60YlMeICAY3mKQjmfPcwpRFCkqBr\n\tXZSghfWg8HpBgfxeciusvzE/EMNbzmGUtM5zx4s8M7tVJrVt5K4r+y0UOmE0zSDMvW\n\tWfbmKH/RVCe9KP5zM3zZIVqEyuTl9nxzBtdWhTM7UqtIh+dmmW41k6TdozoeQnh02j\n\t/E/k5J2ZhXvMdzv2/yRme5U9m+kYIHbDZOxlfxZzVcwF1OS1Mdiqzuyva8Qsq6WNPn\n\tG3TZY8cWbV/mg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4ZQkcRsfi/f2xFwv6+obcgJ0KFzlmmsnxXk6BZI1tsw=;\n\tb=KtVyqeK7k8ofHolsWF/kvaHnKEKPWq3I/dZKVQL+BCRkYV4NHDlOSJd9kwfMQ6Wxpi\n\tuIUUHb60cbBOfYbix1kZA4xWSjm73LIjJKT4sQYhMBFMqwXp6Q1qhvXQfoYnlm5+D4CW\n\t/yoqa4vsUOytCCi0t743hei3zQOdmyHuz7s3dOJAh7cHF4yfhcZllE1Si6ffP2f0MGT1\n\tWpiPgqR8CIhShcIsZxErBmHj4dN1LMNV/g8HNj3QB7zqznr576/7lZFG/i0zVRjHfPrS\n\tZQlqdesOBZ97B4sJY1XWtfQ4oxXfuVVtfih6eteL1gMhuNS1yySUrHsg8CE45wJrdAUs\n\thVLQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"KtVyqeK7\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4ZQkcRsfi/f2xFwv6+obcgJ0KFzlmmsnxXk6BZI1tsw=;\n\tb=ctIvmUDlpljHLEv3Q9LNvegAWXxFSqFL30YuS5Mg1wojxscIbWDtCWTi/1LDy4xIvm\n\tuGGr2WZErlz640XNtOo6L9NSWdx3cTHbl4W7ZkRWKhO2gy7Y6+b9cVhshwWbtQkPrimA\n\tP2ZsqAxTZFwbZkP+q0nfg++w3wBk7SAkJjH/++y11slmcomYUBiqGBqOfN0//hQpb247\n\tisvLJIyYN1gtTGWX7EG/wVUjdq1krIoitZRLDt2vQ4PposhaViTlhOTdwuOtQOH85m2y\n\tOh8df2BlG0H8hCRd0+kLcWazVP0JDIH01eyBUXR7/1ynUyf+dvN3YcFAwsIdUSLvL8x/\n\tb0Hw==","X-Gm-Message-State":"AOAM533rlDgalDIaP0ECTft1oE7HSA8WCUeFgeRMxCdPylxoRV9RZUTQ\n\t7E+uyB1/7skRwXcOpHMz1XvyXGx+2HNyJctR4jxqjqt9fnku0A==","X-Google-Smtp-Source":"ABdhPJzvleg6F6q7PsBS5IgSdHvGi+iH3ibRrQI0RxJ4ijQ1jDZRVtmLPJ86KXSZuppBxLYJpSucOBkHTP36YfdDnUg=","X-Received":"by 2002:a2e:a54d:0:b0:249:8dd1:9da1 with SMTP id\n\te13-20020a2ea54d000000b002498dd19da1mr6487459ljn.372.1648027027699;\n\tWed, 23 Mar 2022 02:17:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 23 Mar 2022 09:16:51 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000910db705dadf3198\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22401,"web_url":"https://patchwork.libcamera.org/comment/22401/","msgid":"","date":"2022-03-23T10:51:03","subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:\n> On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:\n> > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via\n> > libcamera-devel wrote:\n> > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and\n> > > connect the timeout signal to a slot in the pipeline handler. This slot will\n> > > log a fatal error message informing the user of a possible hardware stall.\n> > >\n> > > The timeout is calculated as 2x the maximum frame length possible for a given\n> > > mode, returned by the IPA.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++\n> > > 3 files changed, 18 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > index acd3cafe6c91..5a228b75cd2f 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -41,6 +41,7 @@ struct IPAConfig {\n> > > struct StartConfig {\n> > > libcamera.ControlList controls;\n> > > int32 dropFrameCount;\n> > > + uint32 maxSensorFrameLengthMs;\n> >\n> > We really need duration types in mojo.\n> \n> Yes please!! :-)\n> \n> > > };\n> > >\n> > > interface IPARPiInterface {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index fd8fecb07f81..675f9ba1b350 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> > > }\n> > >\n> > > startConfig->dropFrameCount = dropFrameCount_;\n> > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> > > + startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get();\n> > >\n> > > firstStart_ = false;\n> > > lastRunTimestamp_ = 0;\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index c2230199fed7..50b39f1adf93 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -45,6 +45,8 @@\n> > > #include \"dma_heaps.h\"\n> > > #include \"rpi_stream.h\"\n> > >\n> > > +using namespace std::literals::chrono_literals;\n> > > +\n> > > namespace libcamera {\n> > >\n> > > LOG_DEFINE_CATEGORY(RPI)\n> > > @@ -202,6 +204,7 @@ public:\n> > > void setIspControls(const ControlList &controls);\n> > > void setDelayedControls(const ControlList &controls);\n> > > void setSensorControls(ControlList &controls);\n> > > + void unicamTimeout(V4L2VideoDevice *dev);\n> > >\n> > > /* bufferComplete signal handlers. */\n> > > void unicamBufferDequeue(FrameBuffer *buffer);\n> > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > }\n> > > }\n> > >\n> > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;\n> > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);\n> > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);\n> > > + LOG(RPI, Debug) << \"Setting sensor timeout to \" << duration;\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)\n> > > RPiCameraData *data = cameraData(camera);\n> > >\n> > > data->state_ = RPiCameraData::State::Stopped;\n> > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();\n> >\n> > No need to connect and disconnect the signal when starting and stopping,\n> > you can connect it at init time and keep it connected forever.\n> >\n> > > /* Disable SOF event generation. */\n> > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);\n> > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > > sensor_->setControls(&controls);\n> > > }\n> > >\n> > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)\n> > > +{\n> > > + LOG(RPI, Fatal) << \"Unicam has timed out!\";\n> >\n> > That's harsh, and is likely to trigger if we ever miss a frame due to\n> > high CPU load. If the goal is to detect a complete stall, I'd increase\n> > the timeout to at least 10 frames.\n> >\n> \n> Yes, you are right.\n> \n> I actually meant to use Error here - Fatal was a bit leftover from my testing\n> to see the timeout hit as expected.\n\nMaybe the error message in the V4L2VideoDevice class is enough then ?\n\n> > How will this work with sensors that use an external trigger ?\n> \n> Short answer is it won't and cannot right now. Given this is a purely debug feature,\n> the only effect it will have is to dump a log message to the console which\n> I think is fine.\n\nWon't the message be repeated at regular intervals though ? That may not\nbe very nice.\n\n> In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we\n> are in \"bulb\" mode and we must disable timeouts. Or perhaps the application can pass\n> a config flag?\n\nOr perhaps libcamera should handle the internal/external sync\nconfiguration itself, I wouldn't be surprised if the IPA module could\nbenefit from more knowledge of how the sensor is triggered. We'll of\ncourse need a trigger mode control in that case.\n\n> > > +}\n> > > +\n> > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > {\n> > > RPi::Stream *stream = nullptr;","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 9A705BD80A\n\tfor ;\n\tWed, 23 Mar 2022 10:51:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDCD3604E6;\n\tWed, 23 Mar 2022 11:51:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93921601F8\n\tfor ;\n\tWed, 23 Mar 2022 11:51:21 +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 1AAC59DE;\n\tWed, 23 Mar 2022 11:51:21 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648032682;\n\tbh=Xxdar3fl39zSkcv6eZStzumbr5RFizVG9bg917fefG4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=IXe5NBo8mvV9eG3Hu4xZ8wLwbyin1gdB70Y0wDQoYgKtncwrQM2aQwrjw3/0RIiFS\n\tLqlo4FVNgDA47g52LIGL34+SonqahAgxbkJg7Avda3VdEuGtlrsb/5W5RRe8BaFgwO\n\tmJIGovFxkWBYkmgoSmOiTHSzmOnD7pICF/2z+hkGRyMmDUcwjk2iepuKjmlYZPSJoE\n\tsKtZJQ62B4blN0yJpsV4PPTIAPWRdGs3VEZKdJ2Bz67it/k+iUK0ckQ6u704jryGsJ\n\tkZmjeM0GGTpXTPkA3LYX8KHY7V8DchhJdqpS02CM9BZYu/IIIRDdTlNFZsJu3Ovm7N\n\t9ZJJy9+2VMp6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648032681;\n\tbh=Xxdar3fl39zSkcv6eZStzumbr5RFizVG9bg917fefG4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T0WGLPaGuiczaEzz8wCVgp0kI4BxOdgOLLt8l7VIuY3Esd0t4GN46GBFajeb7mAvu\n\tiSdIVXMM2czdMgXbpi8M/66u1TuNZfLj6aYOlqEQCBKTfJV2Vs7u13bMJGegTLv4k6\n\tmfd2Pk4qmcj0dguAmQcUHOc8nrjxKAr1AWmmrpOI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T0WGLPaG\"; dkim-atps=neutral","Date":"Wed, 23 Mar 2022 12:51:03 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-4-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22413,"web_url":"https://patchwork.libcamera.org/comment/22413/","msgid":"","date":"2022-03-23T15:10:00","subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Wed, 23 Mar 2022 at 10:51, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:\n> > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:\n> > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via\n> > > libcamera-devel wrote:\n> > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image\n> node, and\n> > > > connect the timeout signal to a slot in the pipeline handler. This\n> slot will\n> > > > log a fatal error message informing the user of a possible hardware\n> stall.\n> > > >\n> > > > The timeout is calculated as 2x the maximum frame length possible\n> for a given\n> > > > mode, returned by the IPA.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15\n> +++++++++++++++\n> > > > 3 files changed, 18 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > > > index acd3cafe6c91..5a228b75cd2f 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > @@ -41,6 +41,7 @@ struct IPAConfig {\n> > > > struct StartConfig {\n> > > > libcamera.ControlList controls;\n> > > > int32 dropFrameCount;\n> > > > + uint32 maxSensorFrameLengthMs;\n> > >\n> > > We really need duration types in mojo.\n> >\n> > Yes please!! :-)\n> >\n> > > > };\n> > > >\n> > > > interface IPARPiInterface {\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index fd8fecb07f81..675f9ba1b350 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls,\n> ipa::RPi::StartConfig *startConf\n> > > > }\n> > > >\n> > > > startConfig->dropFrameCount = dropFrameCount_;\n> > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length\n> * mode_.line_length;\n> > > > + startConfig->maxSensorFrameLengthMs =\n> maxSensorFrameDuration.get();\n> > > >\n> > > > firstStart_ = false;\n> > > > lastRunTimestamp_ = 0;\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index c2230199fed7..50b39f1adf93 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -45,6 +45,8 @@\n> > > > #include \"dma_heaps.h\"\n> > > > #include \"rpi_stream.h\"\n> > > >\n> > > > +using namespace std::literals::chrono_literals;\n> > > > +\n> > > > namespace libcamera {\n> > > >\n> > > > LOG_DEFINE_CATEGORY(RPI)\n> > > > @@ -202,6 +204,7 @@ public:\n> > > > void setIspControls(const ControlList &controls);\n> > > > void setDelayedControls(const ControlList &controls);\n> > > > void setSensorControls(ControlList &controls);\n> > > > + void unicamTimeout(V4L2VideoDevice *dev);\n> > > >\n> > > > /* bufferComplete signal handlers. */\n> > > > void unicamBufferDequeue(FrameBuffer *buffer);\n> > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera,\n> const ControlList *controls)\n> > > > }\n> > > > }\n> > > >\n> > > > + /* Set the dequeue timeout to 2x the maximum possible frame\n> duration. */\n> > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs\n> * 2 * 1ms;\n> > > > +\n> data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);\n> > > > +\n> data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data,\n> &RPiCameraData::unicamTimeout);\n> > > > + LOG(RPI, Debug) << \"Setting sensor timeout to \" << duration;\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera\n> *camera)\n> > > > RPiCameraData *data = cameraData(camera);\n> > > >\n> > > > data->state_ = RPiCameraData::State::Stopped;\n> > > > +\n> data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();\n> > >\n> > > No need to connect and disconnect the signal when starting and\n> stopping,\n> > > you can connect it at init time and keep it connected forever.\n> > >\n> > > > /* Disable SOF event generation. */\n> > > >\n> data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);\n> > > > @@ -1757,6 +1767,11 @@ void\n> RPiCameraData::setSensorControls(ControlList &controls)\n> > > > sensor_->setControls(&controls);\n> > > > }\n> > > >\n> > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice\n> *dev)\n> > > > +{\n> > > > + LOG(RPI, Fatal) << \"Unicam has timed out!\";\n> > >\n> > > That's harsh, and is likely to trigger if we ever miss a frame due to\n> > > high CPU load. If the goal is to detect a complete stall, I'd increase\n> > > the timeout to at least 10 frames.\n> > >\n> >\n> > Yes, you are right.\n> >\n> > I actually meant to use Error here - Fatal was a bit leftover from my\n> testing\n> > to see the timeout hit as expected.\n>\n> Maybe the error message in the V4L2VideoDevice class is enough then ?\n>\n> > > How will this work with sensors that use an external trigger ?\n> >\n> > Short answer is it won't and cannot right now. Given this is a purely\n> debug feature,\n> > the only effect it will have is to dump a log message to the console\n> which\n> > I think is fine.\n>\n> Won't the message be repeated at regular intervals though ? That may not\n> be very nice.\n>\n\nYes they will. However, for now, I feel the advantages of having the log\nmessage for\nusers with malfunctioning sensors outweigh the (very rare) cases where\nusers will be\ndriving the sensor in bulb mode. Also given none of our sensors have this\nsupport\n(well, imx477 does, but the kernel driver does not have the swith to do it\nyet), we may\nnot encounter it at all :-)\n\n\n>\n> > In the longer term, perhaps we need to be informed (through the v4l2\n> driver?) that we\n> > are in \"bulb\" mode and we must disable timeouts. Or perhaps the\n> application can pass\n> > a config flag?\n>\n> Or perhaps libcamera should handle the internal/external sync\n> configuration itself, I wouldn't be surprised if the IPA module could\n> benefit from more knowledge of how the sensor is triggered. We'll of\n> course need a trigger mode control in that case.\n>\n\nI think this makes sense. This probably wants to have a dedicated v4l2\ncontrol behind it\nto control from userland.\n\nNaush\n\n\n> > > > +}\n> > > > +\n> > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > {\n> > > > RPi::Stream *stream = nullptr;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 6E315BD80A\n\tfor ;\n\tWed, 23 Mar 2022 15:10:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B03B5604DA;\n\tWed, 23 Mar 2022 16:10:18 +0100 (CET)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EE36604C4\n\tfor ;\n\tWed, 23 Mar 2022 16:10:17 +0100 (CET)","by mail-lf1-x136.google.com with SMTP id bu29so3286716lfb.0\n\tfor ;\n\tWed, 23 Mar 2022 08:10:17 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648048218;\n\tbh=XCtWIYOj0Lzy8t5gz9fGFBKferWK1uD6C9Bec/z1WUU=;\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=ycg8apdnIEhA3jIiDykiAuCnLic31BAuw4gVrPg3prvZItGqAR2FKvC/I9M4RV+vU\n\t3ZLkNXH/c4abNV3JUDLX3Uq0uCNTDigyb95GbPqVjXE1GzpXJi6/KUh4o3NP9Ud/rJ\n\tdtIWH4ujjCY3TbnAWJ5XrytlrtAL6vd0EtVy18G96MJKkGPI7Lrv/b7MtPwK6ZVqjo\n\tw0yjQvo8kpIQnEJi6l1f4k0iTI961cRvKejXykxFu4GZa/tKYvqlzu5zq5lUonAboM\n\tWoedFdPbkR+xB9ea04DjdC9V8Z9169+uarF6qrw3S5clC5mwZroG3r8AOXvT2Zgv9n\n\tCJjwhJ8x4+x4g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=irPGKeoRyq4ovpLqU9GNB72eQRyskrcnjmwN3pykSME=;\n\tb=En4KLmdAOnWKTa50WK8b9ade5apFBghYoBJTZ7DBZwECIPj8gNRW4Aou/G8jkLM7OC\n\t4sUx1a19e71eO/n5U77WdsuZ3J9lsgl+jlXpzqpmpYbo1q7vjPxhEYyDT9NEkeqCGs7e\n\teLYIcvcm8pTwp2KOXeHjpIHrap5lbA0TzNxBIDaoy2eZ45DGjMtVX12pcl1Z7OwDb8Wk\n\tSBZh+5yH6H2Bz9E1++y5x9wjFn2wB8HrFiLOCvadi/HL3rXCgUKHls5bxoNKuJ+NKLFC\n\tzFMI0gfR3aiodL9lVzHRdQNVoXjANQpitB5KqMITxpo69zvoOfUdeME9MMv9Sc0lBW0H\n\tsfbA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"En4KLmdA\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=irPGKeoRyq4ovpLqU9GNB72eQRyskrcnjmwN3pykSME=;\n\tb=H9SpLqQO90qS4ZrykKyvv0iMAY+2O+Ha7JUS3JozL+c0Z50dDj5fMWundV4wKymZOE\n\tr5ugH5uId+eTRrpd9BzYLm4uRphDRhbT1Ct/ENagufXyA7zzHjuxWtedAM2rhn1A8ZLF\n\tRw4f4gIfLveDBvY7r/QqcpPBwWW/PW4OBQ2Ljvh2QKYwbtRA3Hwg0Q7myMQXj5qvgmGL\n\tTPv8SxmQFjULku5TaX/XBEEVvgAqV6MxZF1G78BPiVdkiKDrYIaWWBNBQioNYb+kn5P5\n\tBcXzzANWJDuRWC628N67MKv0oC+yR7+SIQl01Bt4CATXa21lAwMrsCZiv8Vi/T/ny4oe\n\tcY5w==","X-Gm-Message-State":"AOAM53115rWDY+uRSp1AZmGiS+gQ51qNVDEanTAF69up05QXnmJHxjHD\n\tj8rWm4vP6HsRDpNey1kXfJVVG2p2DZSV9LfCOCCKRyJfmto=","X-Google-Smtp-Source":"ABdhPJxs4HJVfclRlZapUBqbmR4g0Zim8HAsb4R08Ha8j2AOgbZDQiWYUA4nbsE8AWsHffAQpF4JDhb1CdzPM2t5H14=","X-Received":"by 2002:ac2:4203:0:b0:448:8053:d402 with SMTP id\n\ty3-20020ac24203000000b004488053d402mr218726lfh.687.1648048216389;\n\tWed, 23 Mar 2022 08:10:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-4-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","Date":"Wed, 23 Mar 2022 15:10:00 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000082cef305dae420fb\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22415,"web_url":"https://patchwork.libcamera.org/comment/22415/","msgid":"","date":"2022-03-23T20:44:25","subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Mar 23, 2022 at 03:10:00PM +0000, Naushir Patuck wrote:\n> On Wed, 23 Mar 2022 at 10:51, Laurent Pinchart wrote:\n> > On Wed, Mar 23, 2022 at 09:16:51AM +0000, Naushir Patuck wrote:\n> > > On Tue, 22 Mar 2022 at 21:51, Laurent Pinchart wrote:\n> > > > On Tue, Mar 22, 2022 at 01:16:35PM +0000, Naushir Patuck via\n> > > > libcamera-devel wrote:\n> > > > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node, and\n> > > > > connect the timeout signal to a slot in the pipeline handler. This slot will\n> > > > > log a fatal error message informing the user of a possible hardware stall.\n> > > > >\n> > > > > The timeout is calculated as 2x the maximum frame length possible for a given\n> > > > > mode, returned by the IPA.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++++++++++++\n> > > > > 3 files changed, 18 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > index acd3cafe6c91..5a228b75cd2f 100644\n> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > > > @@ -41,6 +41,7 @@ struct IPAConfig {\n> > > > > struct StartConfig {\n> > > > > libcamera.ControlList controls;\n> > > > > int32 dropFrameCount;\n> > > > > + uint32 maxSensorFrameLengthMs;\n> > > >\n> > > > We really need duration types in mojo.\n> > >\n> > > Yes please!! :-)\n> > >\n> > > > > };\n> > > > >\n> > > > > interface IPARPiInterface {\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index fd8fecb07f81..675f9ba1b350 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -280,6 +280,8 @@ void IPARPi::start(const ControlList &controls, ipa::RPi::StartConfig *startConf\n> > > > > }\n> > > > >\n> > > > > startConfig->dropFrameCount = dropFrameCount_;\n> > > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> > > > > + startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get();\n> > > > >\n> > > > > firstStart_ = false;\n> > > > > lastRunTimestamp_ = 0;\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index c2230199fed7..50b39f1adf93 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -45,6 +45,8 @@\n> > > > > #include \"dma_heaps.h\"\n> > > > > #include \"rpi_stream.h\"\n> > > > >\n> > > > > +using namespace std::literals::chrono_literals;\n> > > > > +\n> > > > > namespace libcamera {\n> > > > >\n> > > > > LOG_DEFINE_CATEGORY(RPI)\n> > > > > @@ -202,6 +204,7 @@ public:\n> > > > > void setIspControls(const ControlList &controls);\n> > > > > void setDelayedControls(const ControlList &controls);\n> > > > > void setSensorControls(ControlList &controls);\n> > > > > + void unicamTimeout(V4L2VideoDevice *dev);\n> > > > >\n> > > > > /* bufferComplete signal handlers. */\n> > > > > void unicamBufferDequeue(FrameBuffer *buffer);\n> > > > > @@ -1032,6 +1035,12 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > + /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> > > > > + utils::Duration duration = startConfig.maxSensorFrameLengthMs * 2 * 1ms;\n> > > > > + data->unicam_[Unicam::Image].dev()->setDequeueTimeout(duration);\n> > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data, &RPiCameraData::unicamTimeout);\n> > > > > + LOG(RPI, Debug) << \"Setting sensor timeout to \" << duration;\n> > > > > +\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > @@ -1040,6 +1049,7 @@ void PipelineHandlerRPi::stopDevice(Camera *camera)\n> > > > > RPiCameraData *data = cameraData(camera);\n> > > > >\n> > > > > data->state_ = RPiCameraData::State::Stopped;\n> > > > > + data->unicam_[Unicam::Image].dev()->dequeueTimeout.disconnect();\n> > > >\n> > > > No need to connect and disconnect the signal when starting and stopping,\n> > > > you can connect it at init time and keep it connected forever.\n> > > >\n> > > > > /* Disable SOF event generation. */\n> > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);\n> > > > > @@ -1757,6 +1767,11 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > > > > sensor_->setControls(&controls);\n> > > > > }\n> > > > >\n> > > > > +void RPiCameraData::unicamTimeout([[maybe_unused]] V4L2VideoDevice *dev)\n> > > > > +{\n> > > > > + LOG(RPI, Fatal) << \"Unicam has timed out!\";\n> > > >\n> > > > That's harsh, and is likely to trigger if we ever miss a frame due to\n> > > > high CPU load. If the goal is to detect a complete stall, I'd increase\n> > > > the timeout to at least 10 frames.\n> > >\n> > > Yes, you are right.\n> > >\n> > > I actually meant to use Error here - Fatal was a bit leftover from my testing\n> > > to see the timeout hit as expected.\n> >\n> > Maybe the error message in the V4L2VideoDevice class is enough then ?\n> >\n> > > > How will this work with sensors that use an external trigger ?\n> > >\n> > > Short answer is it won't and cannot right now. Given this is a purely debug feature,\n> > > the only effect it will have is to dump a log message to the console which\n> > > I think is fine.\n> >\n> > Won't the message be repeated at regular intervals though ? That may not\n> > be very nice.\n> \n> Yes they will. However, for now, I feel the advantages of having the log message for\n> users with malfunctioning sensors outweigh the (very rare) cases where users will be\n> driving the sensor in bulb mode. Also given none of our sensors have this support\n> (well, imx477 does, but the kernel driver does not have the swith to do it yet), we may\n> not encounter it at all :-)\n\nWe can address it when/if needed, that's fine with me, I'll forward bug\nreports your way ;-)\n\n> > > In the longer term, perhaps we need to be informed (through the v4l2 driver?) that we\n> > > are in \"bulb\" mode and we must disable timeouts. Or perhaps the application can pass\n> > > a config flag?\n> >\n> > Or perhaps libcamera should handle the internal/external sync\n> > configuration itself, I wouldn't be surprised if the IPA module could\n> > benefit from more knowledge of how the sensor is triggered. We'll of\n> > course need a trigger mode control in that case.\n> \n> I think this makes sense. This probably wants to have a dedicated v4l2 control behind it\n> to control from userland.\n> \n> > > > > +}\n> > > > > +\n> > > > > void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > > {\n> > > > > RPi::Stream *stream = nullptr;","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 BE012BD80A\n\tfor ;\n\tWed, 23 Mar 2022 20:44:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDC80604DA;\n\tWed, 23 Mar 2022 21:44:28 +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 54603604C4\n\tfor ;\n\tWed, 23 Mar 2022 21:44:27 +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 BD0D69DE;\n\tWed, 23 Mar 2022 21:44:26 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648068269;\n\tbh=WhA2w6kXT5O031IvyncUGVdnbuQgE1w1PZFtC3/0kd0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1AezqZi54Fu+QRgj/nQ64JeeGo9CdG/wsMtXowCae1doJijJ/1+zJDs7xCkRg4HjA\n\tFpPYUZJ418ItQaxWCgtW868xWXlvmEfqek/f8hgXKULsmXlIDJzaZWLy/ixE4TDXbD\n\tdekD2X28sW2NvMQHSH0lPBdLy5fVJu/lKUcxOfG34PlMx8MwYNzUt/xOAn7HhBY0Xs\n\ttJaXtWq3LqXPiGwXXo8oABBdcUr9gmihhX5w87cQphyPzoBGWPymZyOEdyI+xMkrvU\n\tB3uE0XBc39d1yXpCvaZ9AfvLNwK/mNOTFeV070A0ODlAgxWYumSZmIWLjKAkSy4GAS\n\t487ekFwpvY8Uw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648068267;\n\tbh=WhA2w6kXT5O031IvyncUGVdnbuQgE1w1PZFtC3/0kd0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rclVdumQ0eBdQs0+Hv43BoXwhAHlaGgAFf/EyHX9At58XNBegiuiGCCT4nLetfWIh\n\tWO/wKOYPlTpXy6kLwZYU/RsOoSmFkOAg1euryTubagecKvsXeKj5qBrfyzMtDGGEdh\n\ttdK36zQ3eKdQE4f53XkFowP19fQZETUNs2ngzhOI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rclVdumQ\"; dkim-atps=neutral","Date":"Wed, 23 Mar 2022 22:44:25 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20220322131635.2869526-1-naush@raspberrypi.com>\n\t<20220322131635.2869526-4-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]