[{"id":15831,"web_url":"https://patchwork.libcamera.org/comment/15831/","msgid":"","date":"2021-03-23T13:05:16","subject":"Re: [libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi:\n\tConditionally open the embedded data node","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Mar 23, 2021 at 12:27:35PM +0000, Naushir Patuck wrote:\n> Conditionally open the embedded data node in pipeline_handler::match()\n> based on whether the ipa::init() result reports if the sensor supports\n> embedded data or not.\n> \n> Signed-off-by: Naushir Patuck \n> Tested-by: David Plowman \n> Reviewed-by: Laurent Pinchart \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 57 +++++++------------\n> 1 file changed, 21 insertions(+), 36 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index ce1994186d66..4a3f7cbe3065 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -138,7 +138,7 @@ class RPiCameraData : public CameraData\n> {\n> public:\n> \tRPiCameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), embeddedNodeOpened_(false), state_(State::Stopped),\n> +\t\t: CameraData(pipe), state_(State::Stopped),\n> \t\t supportsFlips_(false), flipsAlterBayerOrder_(false),\n> \t\t updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)\n> \t{\n> @@ -183,7 +183,6 @@ public:\n> \n> \tstd::unique_ptr delayedCtrls_;\n> \tbool sensorMetadata_;\n> -\tbool embeddedNodeOpened_;\n> \n> \t/*\n> \t * All the functions in this class are called from a single calling\n> @@ -749,19 +748,13 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> \t/*\n> -\t * The IPA will set data->sensorMetadata_ to true if embedded data is\n> -\t * supported on this sensor. If so, open the Unicam embedded data\n> -\t * node and configure the output format.\n> +\t * Configure the Unicam embedded data output format only if the sensor\n> +\t * supports it.\n> \t */\n> \tif (data->sensorMetadata_) {\n> \t\tformat = {};\n> \t\tformat.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> \n> -\t\tif (!data->embeddedNodeOpened_) {\n> -\t\t\tdata->unicam_[Unicam::Embedded].dev()->open();\n> -\t\t\tdata->embeddedNodeOpened_ = true;\n> -\t\t}\n> -\n> \t\tLOG(RPI, Debug) << \"Setting embedded data format.\";\n> \t\tret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> \t\tif (ret) {\n> @@ -778,14 +771,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\t */\n> \t\tif (rawStream)\n> \t\t\tdata->unicam_[Unicam::Embedded].setExternal(true);\n> -\t} else {\n> -\t\t/*\n> -\t\t * No embedded data present, so we do not want to iterate over\n> -\t\t * the embedded data stream when starting and stopping.\n> -\t\t */\n> -\t\tdata->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(),\n> -\t\t\t\t\t\t &data->unicam_[Unicam::Embedded]),\n> -\t\t\t\t data->streams_.end());\n> \t}\n> \n> \t/*\n> @@ -989,24 +974,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> \tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> \n> -\t/* This is just for convenience so that we can easily iterate over all streams. */\n> -\tfor (auto &stream : data->unicam_)\n> -\t\tdata->streams_.push_back(&stream);\n> -\tfor (auto &stream : data->isp_)\n> -\t\tdata->streams_.push_back(&stream);\n> -\n> -\t/*\n> -\t * Open all Unicam and ISP streams. The exception is the embedded data\n> -\t * stream, which only gets opened if the IPA reports that the sensor\n> -\t * supports embedded data. This happens in RPiCameraData::configureIPA().\n> -\t */\n> -\tfor (auto const stream : data->streams_) {\n> -\t\tif (stream != &data->unicam_[Unicam::Embedded]) {\n> -\t\t\tif (stream->dev()->open())\n> -\t\t\t\treturn false;\n> -\t\t}\n> -\t}\n> -\n> \t/* Wire up all the buffer connections. */\n> \tdata->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);\n> \tdata->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);\n> @@ -1036,6 +1003,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n> \t}\n> \n> +\t/*\n> +\t * Open all Unicam and ISP streams. The exception is the embedded data\n> +\t * stream, which only gets opened below if the IPA reports that the sensor\n> +\t * supports embedded data.\n> +\t *\n> +\t * The below grouping is just for convenience so that we can easily\n> +\t * iterate over all streams in one go.\n> +\t */\n> +\tdata->streams_.push_back(&data->unicam_[Unicam::Image]);\n> +\tif (sensorConfig.sensorMetadata)\n> +\t\tdata->streams_.push_back(&data->unicam_[Unicam::Embedded]);\n> +\n> +\tfor (auto &stream : data->isp_) {\n> +\t\tdata->streams_.push_back(&stream);\n> +\t\tif (stream.dev()->open())\n> +\t\t\treturn false;\n> +\t}\n\nUnless I'm mistaken, you're not opening the unicam devices anymore.\nThat's why I had too loops in my proposal:\n\n\tfor (auto &stream : data->isp_)\n\t\tdata->streams_.push_back(&stream);\n\n\tfor (auto stream : data->streams_) {\n\t\tif (stream->dev()->open())\n\t\t\treturn false;\n\t}\n\nIf that's indeed the case, could you make sure to test the next version\n? :-)\n\n> +\n> \t/*\n> \t * Setup our delayed control writer with the sensor default\n> \t * gain and exposure delays. Mark VBLANK for priority write.","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 2A813C32E5\n\tfor ;\n\tTue, 23 Mar 2021 13:06:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8931068D63;\n\tTue, 23 Mar 2021 14:06:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62F82602D7\n\tfor ;\n\tTue, 23 Mar 2021 14:05:59 +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 86176B1D;\n\tTue, 23 Mar 2021 14:05:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nP7zlgGg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616504758;\n\tbh=rHQe1FevdA7KSU1w8lpGqhymWCWnbnit52rYUtuNNj8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nP7zlgGg6kkhm2sgyaQcz580UtVJLBoHZKWWpKq+w4SErWFFIW8JFdek3OImCTliT\n\tjT8qRZmPnlpySyFre0JJL35Pk5ZQBHoijPnNXvtwPrIJ8opMixiTzU3nkuJZ6niYLO\n\tLEBCXxkYYgz23lqv/wcjbXJwmERxvUjc08kPpvRE=","Date":"Tue, 23 Mar 2021 15:05:16 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210323122739.680560-1-naush@raspberrypi.com>\n\t<20210323122739.680560-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210323122739.680560-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi:\n\tConditionally open the embedded data node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15833,"web_url":"https://patchwork.libcamera.org/comment/15833/","msgid":"","date":"2021-03-23T13:31:32","subject":"Re: [libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi:\n\tConditionally open the embedded data node","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Tue, 23 Mar 2021 at 13:06, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Mar 23, 2021 at 12:27:35PM +0000, Naushir Patuck wrote:\n> > Conditionally open the embedded data node in pipeline_handler::match()\n> > based on whether the ipa::init() result reports if the sensor supports\n> > embedded data or not.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Tested-by: David Plowman \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 57 +++++++------------\n> > 1 file changed, 21 insertions(+), 36 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index ce1994186d66..4a3f7cbe3065 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -138,7 +138,7 @@ class RPiCameraData : public CameraData\n> > {\n> > public:\n> > RPiCameraData(PipelineHandler *pipe)\n> > - : CameraData(pipe), embeddedNodeOpened_(false),\n> state_(State::Stopped),\n> > + : CameraData(pipe), state_(State::Stopped),\n> > supportsFlips_(false), flipsAlterBayerOrder_(false),\n> > updateScalerCrop_(true), dropFrameCount_(0),\n> ispOutputCount_(0)\n> > {\n> > @@ -183,7 +183,6 @@ public:\n> >\n> > std::unique_ptr delayedCtrls_;\n> > bool sensorMetadata_;\n> > - bool embeddedNodeOpened_;\n> >\n> > /*\n> > * All the functions in this class are called from a single calling\n> > @@ -749,19 +748,13 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> >\n> > /*\n> > - * The IPA will set data->sensorMetadata_ to true if embedded data\n> is\n> > - * supported on this sensor. If so, open the Unicam embedded data\n> > - * node and configure the output format.\n> > + * Configure the Unicam embedded data output format only if the\n> sensor\n> > + * supports it.\n> > */\n> > if (data->sensorMetadata_) {\n> > format = {};\n> > format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);\n> >\n> > - if (!data->embeddedNodeOpened_) {\n> > - data->unicam_[Unicam::Embedded].dev()->open();\n> > - data->embeddedNodeOpened_ = true;\n> > - }\n> > -\n> > LOG(RPI, Debug) << \"Setting embedded data format.\";\n> > ret =\n> data->unicam_[Unicam::Embedded].dev()->setFormat(&format);\n> > if (ret) {\n> > @@ -778,14 +771,6 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > */\n> > if (rawStream)\n> > data->unicam_[Unicam::Embedded].setExternal(true);\n> > - } else {\n> > - /*\n> > - * No embedded data present, so we do not want to iterate\n> over\n> > - * the embedded data stream when starting and stopping.\n> > - */\n> > - data->streams_.erase(std::remove(data->streams_.begin(),\n> data->streams_.end(),\n> > -\n> &data->unicam_[Unicam::Embedded]),\n> > - data->streams_.end());\n> > }\n> >\n> > /*\n> > @@ -989,24 +974,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture2\"));\n> > data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\",\n> isp_->getEntityByName(\"bcm2835-isp0-capture3\"));\n> >\n> > - /* This is just for convenience so that we can easily iterate over\n> all streams. */\n> > - for (auto &stream : data->unicam_)\n> > - data->streams_.push_back(&stream);\n> > - for (auto &stream : data->isp_)\n> > - data->streams_.push_back(&stream);\n> > -\n> > - /*\n> > - * Open all Unicam and ISP streams. The exception is the embedded\n> data\n> > - * stream, which only gets opened if the IPA reports that the\n> sensor\n> > - * supports embedded data. This happens in\n> RPiCameraData::configureIPA().\n> > - */\n> > - for (auto const stream : data->streams_) {\n> > - if (stream != &data->unicam_[Unicam::Embedded]) {\n> > - if (stream->dev()->open())\n> > - return false;\n> > - }\n> > - }\n> > -\n> > /* Wire up all the buffer connections. */\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> > @@ -1036,6 +1003,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator\n> *enumerator)\n> > return false;\n> > }\n> >\n> > + /*\n> > + * Open all Unicam and ISP streams. The exception is the embedded\n> data\n> > + * stream, which only gets opened below if the IPA reports that\n> the sensor\n> > + * supports embedded data.\n> > + *\n> > + * The below grouping is just for convenience so that we can easily\n> > + * iterate over all streams in one go.\n> > + */\n> > + data->streams_.push_back(&data->unicam_[Unicam::Image]);\n> > + if (sensorConfig.sensorMetadata)\n> > + data->streams_.push_back(&data->unicam_[Unicam::Embedded]);\n> > +\n> > + for (auto &stream : data->isp_) {\n> > + data->streams_.push_back(&stream);\n> > + if (stream.dev()->open())\n> > + return false;\n> > + }\n>\n> Unless I'm mistaken, you're not opening the unicam devices anymore.\n> That's why I had too loops in my proposal:\n>\n> for (auto &stream : data->isp_)\n> data->streams_.push_back(&stream);\n>\n> for (auto stream : data->streams_) {\n> if (stream->dev()->open())\n> return false;\n> }\n>\n> If that's indeed the case, could you make sure to test the next version\n> ? :-)\n>\n\n\nSorry :-(\nI did hit this error and have the correct change locally but did not amend\nthe commit!\nWill submit an update shortly.\n\n\n\n\n\n>\n> > +\n> > /*\n> > * Setup our delayed control writer with the sensor default\n> > * gain and exposure delays. Mark VBLANK for priority write.\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 B089EC32E5\n\tfor ;\n\tTue, 23 Mar 2021 13:31:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EFAF68D63;\n\tTue, 23 Mar 2021 14:31:51 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00C17602D7\n\tfor ;\n\tTue, 23 Mar 2021 14:31:48 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id m12so26628178lfq.10\n\tfor ;\n\tTue, 23 Mar 2021 06:31:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"G2ErahaB\"; dkim-atps=neutral","DKIM-Signature":"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=IBZPkdMhpEQImlm3HmSS0kc9FCb3PaYOthaQSAaQ6Zo=;\n\tb=G2ErahaBZOC/Aq0QtZEh3svpwbxKlnVeDlGVHlE8r7HFXAwaM2p9q8rMkck8xYh93v\n\taannZ+1mC9m4dDpKW1VPPvtvUhuD8O/SQZt0wQZMQyBDfigsM8UW9xv1XT8w4aAa6ye7\n\t9E7npiSPBxBUX6ixCxVeqOnSSWZJyNZpLWpx7e2/UfJOcKt05Feb8k/Q/Rf60a37DTmG\n\tZ9BGH70kkSMj8sUz/0SD0Ufz+//mtgIvNvcuVpXN9ACAfQjfXQEIV0KVl9nasmpnShxW\n\tVxCCeHJxYSOwPD60xJDJu96T12EobWG58QMu5/hgKEQ/8swcGW6RhWdQHGAKNB7z/BRN\n\t+wjQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IBZPkdMhpEQImlm3HmSS0kc9FCb3PaYOthaQSAaQ6Zo=;\n\tb=Wj3NU9wxldOF277L02HkZNYGHcxGLCfHr1B/1swTAFn7ZyYJGevvUuhFRa/uACoQQf\n\t1UKU5HLkWAT1qS1flEL+z4IzcnD29zqKV5Xo0IWCtV7W1ufCgFjGzYjjSuf0vjr5l6CE\n\tkv7zhHRI7wJhGX5A69iZMbgqT+L/c3fN9j+SmEY0MFljqJtjV5tVQb/ocJ7tBZcMRnjJ\n\tELgkT6tAPZfo5r6xx80mMN13hhpx1hzNt/2PDzgKVjQr7DU9HRYpnA+MfzsMWsdpjhsT\n\tAa5UXYDtRzGHg82ye+hGw2QUUgx/y1DqYFZlDB7EFHH4lMpg8VDiUNIjapOFsd/dbYyB\n\twn+A==","X-Gm-Message-State":"AOAM530WcHWyz+2m/smGYaIaFLRLRUGMfKFol5lOyG4oEp9mHioGm0Hz\n\tJKvNqYDdO8qOOrkslJoG7OsJkTOxgBH0CWJtTKbZVw==","X-Google-Smtp-Source":"ABdhPJydYNfWnuUUYRvB1Zb8yyUODg6nYuu4E0Y39cnrG3BcsJJWrOUJzMRgjEAyPQyiN9/XqI7G+l97fUrsoeUwNNM=","X-Received":"by 2002:ac2:4205:: with SMTP id y5mr2609892lfh.375.1616506308289;\n\tTue, 23 Mar 2021 06:31:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323122739.680560-1-naush@raspberrypi.com>\n\t<20210323122739.680560-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 23 Mar 2021 13:31:32 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 3/7] pipeline: raspberrypi:\n\tConditionally open the embedded data node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2509972524095765822==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]