[{"id":22534,"web_url":"https://patchwork.libcamera.org/comment/22534/","msgid":"<164873375714.15275.18433735012277391669@Monstersaurus>","date":"2022-03-31T13:35:57","subject":"Re: [libcamera-devel] [PATCH v3 2/3] pipeline: raspberrypi: Add a\n\tUnicam dequeue timeout","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)\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\nNo longer a fatal error message.\n\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 <naush@raspberrypi.com>\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>  \n>  interface IPARPiInterface {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 1bf4e270b062..89767a9db471 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<std::milli>();\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 8fd79be67988..93931aaf3b55 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -208,6 +208,7 @@ public:\n>         void setIspControls(const ControlList &controls);\n>         void setDelayedControls(const ControlList &controls);\n>         void setSensorControls(ControlList &controls);\n> +       void unicamTimeout();\n>  \n>         /* bufferComplete signal handlers. */\n>         void unicamBufferDequeue(FrameBuffer *buffer);\n> @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>                 }\n>         }\n>  \n> +       /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> +       std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);\n> +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);\n> +       LOG(RPI, Debug) << \"Setting Unicam timeout to \" << msec << \" ms.\";\n> +\n>         return 0;\n>  }\n>  \n> @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>         data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n>  \n>         /* Wire up all the buffer connections. */\n> +       data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);\n>         data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n>         data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);\n>         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);\n> @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n>         sensor_->setControls(&controls);\n>  }\n>  \n> +void RPiCameraData::unicamTimeout()\n> +{\n> +       LOG(RPI, Error)\n> +               << \"Unicam has timed out!\\n\"\n> +                  \"Please check that your camera sensor connector is attached securely.\\n\"\n\nThe '\\n' should probably be std::endl ... but I don't think that makes a\nbig difference here?\n\n\nBoth of those could be fixed while applying, there's nothing complex\nthere.\n\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> +                  \"Alternatively, try another cable and/or sensor.\";\n> +}\n> +\n>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n>  {\n>         RPi::Stream *stream = nullptr;\n> -- \n> 2.25.1\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 200F6C3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 31 Mar 2022 13:36:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6608B65633;\n\tThu, 31 Mar 2022 15:36:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E35DF6559A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 31 Mar 2022 15:35:59 +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 8CDF659D;\n\tThu, 31 Mar 2022 15:35:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648733761;\n\tbh=wj2KEkA2N6KL27DDOx68GmWc5j0j9ZLceMIG+0WDe24=;\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=o4MRbobv4q33olZbQSKRJDfKltQrtmq4/VxATERRjHiOXJN0rlkK7eT920P6ZTF+2\n\tUdImV9j6wJYZj1348//lBXhcLinGFcX17MY/ettFIsBYpsz8dtesT/ZY5dtDPTydw4\n\tftg7y3T79Q6uvtPGRgzlXbVVpxQibbwha9QDUKge7Yw0yckJb5p93UDQc9dC8tBtZl\n\tYGD07uxWxit3702LiqAIYGLAZ7CGgYyMlpNEQIA6EvL8O0/sqL76gayZrfAuZ01y0Q\n\tDKsFSBK4IalkQF6rTrS9MfFZmAAbKrJbFDUVAp0N+n+ZOubVWPSFKkD/UArxnXmhvc\n\tRrRPjJKde4M0A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648733759;\n\tbh=wj2KEkA2N6KL27DDOx68GmWc5j0j9ZLceMIG+0WDe24=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=tIJqdz11WO7u4yQ7w8zVSA5NjjDrOOjPVw5GsS8n7ey7sWoGu5BdjxxqjbXxAOZkX\n\t1jqA2ExMKec0dlS5NE6e4I+wXvzkgcWuqL3sdB3vNDwGq43rjkgtKxPKlI0t6YidDL\n\tgLgdXQ7NgMfJeqEwei4icgtRiXQ0x3Xj21bnhemw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tIJqdz11\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220329112929.465434-2-naush@raspberrypi.com>","References":"<20220329112929.465434-1-naush@raspberrypi.com>\n\t<20220329112929.465434-2-naush@raspberrypi.com>","To":"Naushir Patuck <naush@raspberrypi.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 31 Mar 2022 14:35:57 +0100","Message-ID":"<164873375714.15275.18433735012277391669@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"<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>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22574,"web_url":"https://patchwork.libcamera.org/comment/22574/","msgid":"<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","date":"2022-04-05T15:34:21","subject":"Re: [libcamera-devel] [PATCH v3 2/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":"On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)\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> No longer a fatal error message.\n> \n> > The timeout is calculated as 2x the maximum frame length possible for a given\n> > mode, returned by the IPA.\n\nI mentioned in the review of v1 that we should probably increase the\ntimeout (to 10 times the frame duration). That was based on the error\nmessage being fatal though, but maybe it's still good to avoid a short\nwatchdog timeout ?\n\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\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> >  \n> >  interface IPARPiInterface {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 1bf4e270b062..89767a9db471 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<std::milli>();\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 8fd79be67988..93931aaf3b55 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -208,6 +208,7 @@ public:\n> >         void setIspControls(const ControlList &controls);\n> >         void setDelayedControls(const ControlList &controls);\n> >         void setSensorControls(ControlList &controls);\n> > +       void unicamTimeout();\n> >  \n> >         /* bufferComplete signal handlers. */\n> >         void unicamBufferDequeue(FrameBuffer *buffer);\n> > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> >                 }\n> >         }\n> >  \n> > +       /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> > +       std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);\n\ns/msec/timeout/\n\n> > +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);\n> > +       LOG(RPI, Debug) << \"Setting Unicam timeout to \" << msec << \" ms.\";\n\nI may have dropped this message, I'm not sure it helps much.\n\n> > +\n> >         return 0;\n> >  }\n> >  \n> > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >         data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> >  \n> >         /* Wire up all the buffer connections. */\n> > +       data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);\n> >         data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> >         data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);\n> >         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);\n> > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> >         sensor_->setControls(&controls);\n> >  }\n> >  \n> > +void RPiCameraData::unicamTimeout()\n> > +{\n> > +       LOG(RPI, Error)\n> > +               << \"Unicam has timed out!\\n\"\n> > +                  \"Please check that your camera sensor connector is attached securely.\\n\"\n> \n> The '\\n' should probably be std::endl ... but I don't think that makes a\n> big difference here?\n\nIt would still be good, for consistency.\n\n> Both of those could be fixed while applying, there's nothing complex\n> there.\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > +                  \"Alternatively, try another cable and/or sensor.\";\n> > +}\n> > +\n> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> >  {\n> >         RPi::Stream *stream = nullptr;","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 CE3A5C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  5 Apr 2022 15:34:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4024965642;\n\tTue,  5 Apr 2022 17:34:27 +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 7FC86604BB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  5 Apr 2022 17:34:25 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08E5F5D;\n\tTue,  5 Apr 2022 17:34:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649172867;\n\tbh=NaH+Am/wl8lbJxqjdF61EtNjuCIYehsSgBBkenXJcsA=;\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=IWk5WxIkD+TELojv+RJuQceu3mzWKHnzpU1onNRnVwQ5h6Gp85qScksKIL/3ofDHH\n\tC+xv1y15GsU8FveGv104NYBCctcUtbW+9k/ROviTDcXijDRazI1FcnluAvVXHZn+Er\n\tc5CFicdFAuawD2C2qZPXPSuPQmGAheMGE5JJ4/VtWJfA+RhcpaQvIt3uNHk8XLWAWs\n\tlkOIaapMENOw+62FhPZQY71Osi3skx8u28fAjwWB423uxb/wmi94YAAV16kvvKB0P3\n\tYbaoR4H77xr3kRQR2dLHJfJiy2wPTQRDVGZiq5pZMTrXU487LmSuMxxS+YSN94/nW4\n\tkzgNfWRj6YzxA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649172865;\n\tbh=NaH+Am/wl8lbJxqjdF61EtNjuCIYehsSgBBkenXJcsA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cAoJnGHUzQTdC5KY2opuq8ziv/FMYCBCA0bEE0xNdzTxlrpvGuVckFKMBpvQfn+S9\n\tzB0ZRCQb5m0E7Xx2DODcrX5yRZPQFxfP1ZjSRXiNh89YS3Vfbe0iwcp4WCl77p12Th\n\tc5VIUO6ODaie3fMz7JKYSJZyYX72NtcdzU4BHCFQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cAoJnGHU\"; dkim-atps=neutral","Date":"Tue, 5 Apr 2022 18:34:21 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","References":"<20220329112929.465434-1-naush@raspberrypi.com>\n\t<20220329112929.465434-2-naush@raspberrypi.com>\n\t<164873375714.15275.18433735012277391669@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164873375714.15275.18433735012277391669@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"<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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22576,"web_url":"https://patchwork.libcamera.org/comment/22576/","msgid":"<YkxoOep/LhbkrJAU@pendragon.ideasonboard.com>","date":"2022-04-05T16:03:05","subject":"Re: [libcamera-devel] [PATCH v3 2/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":"On Tue, Apr 05, 2022 at 06:34:21PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)\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> > No longer a fatal error message.\n> > \n> > > The timeout is calculated as 2x the maximum frame length possible for a given\n> > > mode, returned by the IPA.\n> \n> I mentioned in the review of v1 that we should probably increase the\n> timeout (to 10 times the frame duration). That was based on the error\n> message being fatal though, but maybe it's still good to avoid a short\n> watchdog timeout ?\n> \n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\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> > >  \n> > >  interface IPARPiInterface {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 1bf4e270b062..89767a9db471 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<std::milli>();\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 8fd79be67988..93931aaf3b55 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -208,6 +208,7 @@ public:\n> > >         void setIspControls(const ControlList &controls);\n> > >         void setDelayedControls(const ControlList &controls);\n> > >         void setSensorControls(ControlList &controls);\n> > > +       void unicamTimeout();\n> > >  \n> > >         /* bufferComplete signal handlers. */\n> > >         void unicamBufferDequeue(FrameBuffer *buffer);\n> > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > >                 }\n> > >         }\n> > >  \n> > > +       /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> > > +       std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);\n> \n> s/msec/timeout/\n> \n> > > +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);\n> > > +       LOG(RPI, Debug) << \"Setting Unicam timeout to \" << msec << \" ms.\";\n> \n> I may have dropped this message, I'm not sure it helps much.\n> \n> > > +\n> > >         return 0;\n> > >  }\n> > >  \n> > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >         data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > >  \n> > >         /* Wire up all the buffer connections. */\n> > > +       data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);\n> > >         data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > >         data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);\n> > >         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);\n> > > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > >         sensor_->setControls(&controls);\n> > >  }\n> > >  \n> > > +void RPiCameraData::unicamTimeout()\n> > > +{\n> > > +       LOG(RPI, Error)\n> > > +               << \"Unicam has timed out!\\n\"\n> > > +                  \"Please check that your camera sensor connector is attached securely.\\n\"\n> > \n> > The '\\n' should probably be std::endl ... but I don't think that makes a\n> > big difference here?\n> \n> It would still be good, for consistency.\n\nAlso, a \\n in the middle of the log may be a bit messy, as the next\nlines will not have the right prefixes. We could add some sort of\nutils::logEndl that would do the right thing (but that's out of scope\nfor this series), or maybe shorten the message to one line here, or even\nprint multiple messages ?\n\n> > Both of those could be fixed while applying, there's nothing complex\n> > there.\n> > \n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> > > +                  \"Alternatively, try another cable and/or sensor.\";\n> > > +}\n> > > +\n> > >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > >  {\n> > >         RPi::Stream *stream = nullptr;\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 24643C3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  5 Apr 2022 16:03:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 814AD6563F;\n\tTue,  5 Apr 2022 18:03:10 +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 B8D9F604BB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  5 Apr 2022 18:03:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4B9D75D;\n\tTue,  5 Apr 2022 18:03:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649174590;\n\tbh=MAfhuuxDt6ItFf8ru9bivLwUn8heuhG6pDmOLNZZ+68=;\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:\n\tFrom;\n\tb=PdonsyP73MaDiAC061UwGpILcBoVAz5W+C8Uwa2ASXxiH5YknGr4sXouKVAbUSWyt\n\tC0iO620rg/uKBFVSV9MuEgVpRp+Grd61IT0DU4a9vPFMVEg9/5IaZxXp0CTLdEBF23\n\tfMGqB4mkKGm7eY5ohU2QczdZTjEe/hMX90GiMnahH0MImz1V2QAFH72Fs4XZtN9NQ8\n\tvqb47a93WrlroetPqOImeGpJzBpVzKDYP6pe0kcpHysh6+1+1SEJrHAFE60keQvZdV\n\tZyk1XsvuGu0+mqZzbdYzf0ZqJrx81a8qUZ+xouaFqbFwscV4aa1KtWbPRFVx9wxE9H\n\tIQXg7Ohjs8Siw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649174588;\n\tbh=MAfhuuxDt6ItFf8ru9bivLwUn8heuhG6pDmOLNZZ+68=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=OmKvHQwi+7fL4fEKrMgqAX5avOvG8m/uZddSzwGw3MAqpJ+vGn6dbnzvUsgtDDTCk\n\tib8ZIfZSFnfUI7YMmQmiRgQBzvcbbRPmrlrgsEv2aPzWh/KPxgyJjgNP1aPaWrFUVI\n\tV1eT827FW0DZ8uXlbf5oU+q6V8nTnhjo6yUxcF7U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OmKvHQwi\"; dkim-atps=neutral","Date":"Tue, 5 Apr 2022 19:03:05 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<YkxoOep/LhbkrJAU@pendragon.ideasonboard.com>","References":"<20220329112929.465434-1-naush@raspberrypi.com>\n\t<20220329112929.465434-2-naush@raspberrypi.com>\n\t<164873375714.15275.18433735012277391669@Monstersaurus>\n\t<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"<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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22608,"web_url":"https://patchwork.libcamera.org/comment/22608/","msgid":"<CAEmqJPqf5iXOHSPrHQGDMXiUPJyON+FOuP3t+tR0WkM=XRZd3w@mail.gmail.com>","date":"2022-04-06T07:12:26","subject":"Re: [libcamera-devel] [PATCH v3 2/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, 5 Apr 2022 at 16:34, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via\n> libcamera-devel wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)\n> > > Enable the V4L2VideoDevice dequeue timeout for the Unicam Image node,\n> 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> > No longer a fatal error message.\n> >\n> > > The timeout is calculated as 2x the maximum frame length possible for\n> a given\n> > > mode, returned by the IPA.\n>\n> I mentioned in the review of v1 that we should probably increase the\n> timeout (to 10 times the frame duration). That was based on the error\n> message being fatal though, but maybe it's still good to avoid a short\n> watchdog timeout ?\n>\n\nI did wonder what to do here for the timeout limit.  Having 10x frame\nduration is going to\nbe very long for imx477 that has a 200s frame length.  I think given this\nis only a log message\nI'm inclined to leave it short to catch the obvious failure case. If I get\ncomplaints of too many\nfalse positives we can bump this up a bit :-)\n\nI'll fix up all the suggested changes and post a new revision shortly.\n\nRegards,\nNaush\n\n\n>\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\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> > >\n> > >  interface IPARPiInterface {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 1bf4e270b062..89767a9db471 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<std::milli>();\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 8fd79be67988..93931aaf3b55 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -208,6 +208,7 @@ public:\n> > >         void setIspControls(const ControlList &controls);\n> > >         void setDelayedControls(const ControlList &controls);\n> > >         void setSensorControls(ControlList &controls);\n> > > +       void unicamTimeout();\n> > >\n> > >         /* bufferComplete signal handlers. */\n> > >         void unicamBufferDequeue(FrameBuffer *buffer);\n> > > @@ -1050,6 +1051,11 @@ 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> > > +       std::chrono::milliseconds\n> msec(startConfig.maxSensorFrameLengthMs * 2);\n>\n> s/msec/timeout/\n>\n> > > +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);\n> > > +       LOG(RPI, Debug) << \"Setting Unicam timeout to \" << msec << \"\n> ms.\";\n>\n> I may have dropped this message, I'm not sure it helps much.\n>\n> > > +\n> > >         return 0;\n> > >  }\n> > >\n> > > @@ -1192,6 +1198,7 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >         data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > >\n> > >         /* Wire up all the buffer connections. */\n> > > +\n>  data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(),\n> &RPiCameraData::unicamTimeout);\n> > >\n>  data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(),\n> &RPiCameraData::frameStarted);\n> > >\n>  data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::unicamBufferDequeue);\n> > >         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(),\n> &RPiCameraData::ispInputDequeue);\n> > > @@ -1773,6 +1780,14 @@ void\n> RPiCameraData::setSensorControls(ControlList &controls)\n> > >         sensor_->setControls(&controls);\n> > >  }\n> > >\n> > > +void RPiCameraData::unicamTimeout()\n> > > +{\n> > > +       LOG(RPI, Error)\n> > > +               << \"Unicam has timed out!\\n\"\n> > > +                  \"Please check that your camera sensor connector is\n> attached securely.\\n\"\n> >\n> > The '\\n' should probably be std::endl ... but I don't think that makes a\n> > big difference here?\n>\n> It would still be good, for consistency.\n>\n> > Both of those could be fixed while applying, there's nothing complex\n> > there.\n> >\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n> > > +                  \"Alternatively, try another cable and/or sensor.\";\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":"<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 8D650C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  6 Apr 2022 07:12:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0198F65642;\n\tWed,  6 Apr 2022 09:12:45 +0200 (CEST)","from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BE32604B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Apr 2022 09:12:43 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id a30so1936297ljq.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 06 Apr 2022 00:12:43 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649229165;\n\tbh=VhSeftGjnLunP8G0MqVrZ2+I0BsLNT+vkkujR+u2Ubw=;\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=Cp1ceRw2RcMDWH9PiSh4qisGdsTu+xWytiW8nN9UTo8npRFUhxMd3DS5x75vdXf3q\n\tbrJXBSBDxq1IkdLLVykBFnyGzXB7pm6lBj1Pl/TYE3fqm75R3058XMZnggt0Hovb3l\n\twDWwBk/C5xek+DNud+Mul1KcYHmxYDgNZjtttr+OrP5k9GLK+r4b4HPvgqtgozF0rX\n\t17o7KT2m+Al3m7RJj4CeRtIs8IFUAKc47VT1oOmNfAWX8/6WMHrz4e8OvJFAI9YwZK\n\tc8ilJBJqMvH+7HVg4OjjJnAolcuhllm4dHYGR9NyI9eS+AzW55PivlHq7Sjs2ZUiy2\n\tDzQJNfCcf1xRQ==","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=JGu3UA03dWTxXLPOP4vflTbQ9jaEpCNspB2JzrQk0Ow=;\n\tb=qqc794eczGo0D3jB+CJphYPdIvNd9LrD/tXAuZmM94NeRQh0ZGzOwjvqLsAD56FLwT\n\tptgkF3KtW8YnM/CboG9YOwpGRYIRyq0u99+WCzUbIflGldwcR7jS4IWGw2Vrfa6mnBVL\n\t7DDpWn3BAlRDVALx2UOTpnUD+FpfuQQ9qZOGEK9rVuhlIEsRmJCfo4UImpqjSqYHNN5e\n\t0e3QCDQG46hch1q3bdMEb5Fjw9Bo1IigAguo/rZ6rC5nvx2iLa8ouQXJZyUgHiZ213VQ\n\t2fK/XYVYbGewpoBGZvT3VBRwvj9T8jZGApsZ0VTnS6vQl09GwJ9OxTJqdXK18+8BvsAx\n\tqpvQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"qqc794ec\"; 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=JGu3UA03dWTxXLPOP4vflTbQ9jaEpCNspB2JzrQk0Ow=;\n\tb=6Yt3xLkQJmX0f6H0yZX0cNW+L1bquzrAKh18ApU4ai2URwPsuBpv0ZlqoYvYOtbV3r\n\t7nbCaYSUZ+TfNwJp1YsFTPkorgpxBgCmQV6UQYbzrmPKkFa+zbr0SHirVWLTJhiF4U7t\n\t5Z5lukG4j6yLlaoQtk83Prq3zqMSXTgVkqmEB8e8KyIVU2kofu/cohn+jNC2zOPISW+d\n\tulnVD+/Bel1BYpxLsCJJiJvEmMMbz1Bbs3Eg0Se+14MSHpGXAhdh2c7EgLvDQF5xUi2l\n\tk4VNM7iNEqjYV3bjr68qVoRwMvw0LjlR4dL/y1FmiM5dVWSe4SmjxCb9DrZoWk6vDn4z\n\t60nw==","X-Gm-Message-State":"AOAM533uiCinrCQR8tN01AxJubnfpTBG60ONbLdimATGQuone9Qh96dI\n\t3cq6hU4sVrVxx0qu+9oj6H9oDT8ZJRXIqA4yORA+dg==","X-Google-Smtp-Source":"ABdhPJxCtekTXCnGvqgDqfvI4teRVx4UsdkSOJXN03SrfkH2cogGtK2KcRBrj/HKfIdMyScSJMUmlnQMU+yqtdl9v0w=","X-Received":"by 2002:a2e:b054:0:b0:24b:108d:3792 with SMTP id\n\td20-20020a2eb054000000b0024b108d3792mr4438392ljl.444.1649229162739;\n\tWed, 06 Apr 2022 00:12:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20220329112929.465434-1-naush@raspberrypi.com>\n\t<20220329112929.465434-2-naush@raspberrypi.com>\n\t<164873375714.15275.18433735012277391669@Monstersaurus>\n\t<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","In-Reply-To":"<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>","Date":"Wed, 6 Apr 2022 08:12:26 +0100","Message-ID":"<CAEmqJPqf5iXOHSPrHQGDMXiUPJyON+FOuP3t+tR0WkM=XRZd3w@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000066158b05dbf71619\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"<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>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":22613,"web_url":"https://patchwork.libcamera.org/comment/22613/","msgid":"<Yk1sXFLIQXHN9cZP@pendragon.ideasonboard.com>","date":"2022-04-06T10:33:00","subject":"Re: [libcamera-devel] [PATCH v3 2/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, Apr 06, 2022 at 08:12:26AM +0100, Naushir Patuck wrote:\n> On Tue, 5 Apr 2022 at 16:34, Laurent Pinchart wrote:\n> > On Thu, Mar 31, 2022 at 02:35:57PM +0100, Kieran Bingham via\n> > libcamera-devel wrote:\n> > > Quoting Naushir Patuck via libcamera-devel (2022-03-29 12:29:28)\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> > > No longer a fatal error message.\n> > >\n> > > > The timeout is calculated as 2x the maximum frame length possible for a given\n> > > > mode, returned by the IPA.\n> >\n> > I mentioned in the review of v1 that we should probably increase the\n> > timeout (to 10 times the frame duration). That was based on the error\n> > message being fatal though, but maybe it's still good to avoid a short\n> > watchdog timeout ?\n> \n> I did wonder what to do here for the timeout limit.  Having 10x frame duration is going to\n> be very long for imx477 that has a 200s frame length.  I think given this is only a log message\n> I'm inclined to leave it short to catch the obvious failure case. If I get complaints of too many\n> false positives we can bump this up a bit :-)\n\nAh right it's the maximum frame duration, not the current frame\nduration. Well, in that case I wonder how useful it will be with the\nIMX477, with a 400s timeout :-) That's fine, there's no need to worry\nabout it.\n\nOn the other hand, for sensors that have a small maximum frame duration,\nwould it make sense to set the timeout to, for instance, std::max(1s,\nmax frame duration * 2) ?\n\n> I'll fix up all the suggested changes and post a new revision shortly.\n> \n> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\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> > > >\n> > > >  interface IPARPiInterface {\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 1bf4e270b062..89767a9db471 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<std::milli>();\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 8fd79be67988..93931aaf3b55 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -208,6 +208,7 @@ public:\n> > > >         void setIspControls(const ControlList &controls);\n> > > >         void setDelayedControls(const ControlList &controls);\n> > > >         void setSensorControls(ControlList &controls);\n> > > > +       void unicamTimeout();\n> > > >\n> > > >         /* bufferComplete signal handlers. */\n> > > >         void unicamBufferDequeue(FrameBuffer *buffer);\n> > > > @@ -1050,6 +1051,11 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > >                 }\n> > > >         }\n> > > >\n> > > > +       /* Set the dequeue timeout to 2x the maximum possible frame duration. */\n> > > > +       std::chrono::milliseconds msec(startConfig.maxSensorFrameLengthMs * 2);\n> >\n> > s/msec/timeout/\n> >\n> > > > +       data->unicam_[Unicam::Image].dev()->setDequeueTimeout(msec);\n> > > > +       LOG(RPI, Debug) << \"Setting Unicam timeout to \" << msec << \" ms.\";\n> >\n> > I may have dropped this message, I'm not sure it helps much.\n> >\n> > > > +\n> > > >         return 0;\n> > > >  }\n> > > >\n> > > > @@ -1192,6 +1198,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > >         data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> > > >\n> > > >         /* Wire up all the buffer connections. */\n> > > > +       data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);\n> > > >         data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> > > >         data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);\n> > > >         data->isp_[Isp::Input].dev()->bufferReady.connect(data.get(), &RPiCameraData::ispInputDequeue);\n> > > > @@ -1773,6 +1780,14 @@ void RPiCameraData::setSensorControls(ControlList &controls)\n> > > >         sensor_->setControls(&controls);\n> > > >  }\n> > > >\n> > > > +void RPiCameraData::unicamTimeout()\n> > > > +{\n> > > > +       LOG(RPI, Error)\n> > > > +               << \"Unicam has timed out!\\n\"\n> > > > +                  \"Please check that your camera sensor connector is attached securely.\\n\"\n> > >\n> > > The '\\n' should probably be std::endl ... but I don't think that makes a\n> > > big difference here?\n> >\n> > It would still be good, for consistency.\n> >\n> > > Both of those could be fixed while applying, there's nothing complex\n> > > there.\n> > >\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >\n> > > > +                  \"Alternatively, try another cable and/or sensor.\";\n> > > > +}\n> > > > +\n> > > >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > >  {\n> > > >         RPi::Stream *stream = nullptr;","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 6FB21C3256\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  6 Apr 2022 10:33:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC2CB65642;\n\tWed,  6 Apr 2022 12:33:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78700633A4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  6 Apr 2022 12:33:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F39CA482;\n\tWed,  6 Apr 2022 12:33:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649241186;\n\tbh=jmUR1Ya4+vIrbIeZRyXB0iPAz6tyAYvo86B9Rws2epk=;\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=VU7VRMzcpZlWQMJhct615ym/mHrJRvkyoKsp9gBNJvigDjLBDQx6pqId0SGhjQWDR\n\tYXIIkk0st/2v7RTcZZ1LjGKYnYsRgrqZqNsAW65+Bf7DuTtDmOecRuSDPbux12FS2P\n\tnfVQdbRTplm36Xt0PamoOudqSvKc1/s/GmNe4lGhWL96LIelGU6Q8Ffje79vnsq4gf\n\tBzLCRWUNpiWUEwISJDT/tBXjQV4l3kF1pJZ5/OGNWkB+2AdOuJWL4rXrHNsLCqqq1W\n\tF0QQOoxCMVdecdiLe5O3ieWCnzZtv6TwOlq8HDvHwVCNOEr1+7A08mLzVBqso/hEtN\n\tFfTxX9NvkK3Kw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649241184;\n\tbh=jmUR1Ya4+vIrbIeZRyXB0iPAz6tyAYvo86B9Rws2epk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dcjSvq13eGH/FTQyGp2Gw6wgLBl+ma+VCPMBXkGDSa/l5h/UskMnrtWXjcvykrK7S\n\t4t3nDlXGWBezWTxdTk37pkW7mrTbg9BdQrM9q6BGpHPNsksNnCJ9mdGe1iDobx/kd0\n\tdgMdDmSuKiNLLY7oqSJufUcPyWpfa+GmeFOK9UCU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dcjSvq13\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 13:33:00 +0300","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<Yk1sXFLIQXHN9cZP@pendragon.ideasonboard.com>","References":"<20220329112929.465434-1-naush@raspberrypi.com>\n\t<20220329112929.465434-2-naush@raspberrypi.com>\n\t<164873375714.15275.18433735012277391669@Monstersaurus>\n\t<YkxhfXtZOfrzGrzu@pendragon.ideasonboard.com>\n\t<CAEmqJPqf5iXOHSPrHQGDMXiUPJyON+FOuP3t+tR0WkM=XRZd3w@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPqf5iXOHSPrHQGDMXiUPJyON+FOuP3t+tR0WkM=XRZd3w@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/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":"<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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]