<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 23 Mar 2021 at 13:06, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Tue, Mar 23, 2021 at 12:27:35PM +0000, Naushir Patuck wrote:<br>
> Conditionally open the embedded data node in pipeline_handler::match()<br>
> based on whether the ipa::init() result reports if the sensor supports<br>
> embedded data or not.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 57 +++++++------------<br>
> 1 file changed, 21 insertions(+), 36 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index ce1994186d66..4a3f7cbe3065 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -138,7 +138,7 @@ class RPiCameraData : public CameraData<br>
> {<br>
> public:<br>
> RPiCameraData(PipelineHandler *pipe)<br>
> - : CameraData(pipe), embeddedNodeOpened_(false), state_(State::Stopped),<br>
> + : CameraData(pipe), state_(State::Stopped),<br>
> supportsFlips_(false), flipsAlterBayerOrder_(false),<br>
> updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)<br>
> {<br>
> @@ -183,7 +183,6 @@ public:<br>
> <br>
> std::unique_ptr<DelayedControls> delayedCtrls_;<br>
> bool sensorMetadata_;<br>
> - bool embeddedNodeOpened_;<br>
> <br>
> /*<br>
> * All the functions in this class are called from a single calling<br>
> @@ -749,19 +748,13 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> LOG(RPI, Error) << "Failed to configure the IPA: " << ret;<br>
> <br>
> /*<br>
> - * The IPA will set data->sensorMetadata_ to true if embedded data is<br>
> - * supported on this sensor. If so, open the Unicam embedded data<br>
> - * node and configure the output format.<br>
> + * Configure the Unicam embedded data output format only if the sensor<br>
> + * supports it.<br>
> */<br>
> if (data->sensorMetadata_) {<br>
> format = {};<br>
> format.fourcc = V4L2PixelFormat(V4L2_META_FMT_SENSOR_DATA);<br>
> <br>
> - if (!data->embeddedNodeOpened_) {<br>
> - data->unicam_[Unicam::Embedded].dev()->open();<br>
> - data->embeddedNodeOpened_ = true;<br>
> - }<br>
> -<br>
> LOG(RPI, Debug) << "Setting embedded data format.";<br>
> ret = data->unicam_[Unicam::Embedded].dev()->setFormat(&format);<br>
> if (ret) {<br>
> @@ -778,14 +771,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> */<br>
> if (rawStream)<br>
> data->unicam_[Unicam::Embedded].setExternal(true);<br>
> - } else {<br>
> - /*<br>
> - * No embedded data present, so we do not want to iterate over<br>
> - * the embedded data stream when starting and stopping.<br>
> - */<br>
> - data->streams_.erase(std::remove(data->streams_.begin(), data->streams_.end(),<br>
> - &data->unicam_[Unicam::Embedded]),<br>
> - data->streams_.end());<br>
> }<br>
> <br>
> /*<br>
> @@ -989,24 +974,6 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
> data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", isp_->getEntityByName("bcm2835-isp0-capture2"));<br>
> data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", isp_->getEntityByName("bcm2835-isp0-capture3"));<br>
> <br>
> - /* This is just for convenience so that we can easily iterate over all streams. */<br>
> - for (auto &stream : data->unicam_)<br>
> - data->streams_.push_back(&stream);<br>
> - for (auto &stream : data->isp_)<br>
> - data->streams_.push_back(&stream);<br>
> -<br>
> - /*<br>
> - * Open all Unicam and ISP streams. The exception is the embedded data<br>
> - * stream, which only gets opened if the IPA reports that the sensor<br>
> - * supports embedded data. This happens in RPiCameraData::configureIPA().<br>
> - */<br>
> - for (auto const stream : data->streams_) {<br>
> - if (stream != &data->unicam_[Unicam::Embedded]) {<br>
> - if (stream->dev()->open())<br>
> - return false;<br>
> - }<br>
> - }<br>
> -<br>
> /* Wire up all the buffer connections. */<br>
> data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), &RPiCameraData::frameStarted);<br>
> data->unicam_[Unicam::Image].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue);<br>
> @@ -1036,6 +1003,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
> return false;<br>
> }<br>
> <br>
> + /*<br>
> + * Open all Unicam and ISP streams. The exception is the embedded data<br>
> + * stream, which only gets opened below if the IPA reports that the sensor<br>
> + * supports embedded data.<br>
> + *<br>
> + * The below grouping is just for convenience so that we can easily<br>
> + * iterate over all streams in one go.<br>
> + */<br>
> + data->streams_.push_back(&data->unicam_[Unicam::Image]);<br>
> + if (sensorConfig.sensorMetadata)<br>
> + data->streams_.push_back(&data->unicam_[Unicam::Embedded]);<br>
> +<br>
> + for (auto &stream : data->isp_) {<br>
> + data->streams_.push_back(&stream);<br>
> + if (<a href="http://stream.dev" rel="noreferrer" target="_blank">stream.dev</a>()->open())<br>
> + return false;<br>
> + }<br>
<br>
Unless I'm mistaken, you're not opening the unicam devices anymore.<br>
That's why I had too loops in my proposal:<br>
<br>
for (auto &stream : data->isp_)<br>
data->streams_.push_back(&stream);<br>
<br>
for (auto stream : data->streams_) {<br>
if (stream->dev()->open())<br>
return false;<br>
}<br>
<br>
If that's indeed the case, could you make sure to test the next version<br>
? :-)<br></blockquote><div><br></div><div><br></div><div>Sorry :-(</div><div>I did hit this error and have the correct change locally but did not amend the commit!</div><div>Will submit an update shortly.</div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> /*<br>
> * Setup our delayed control writer with the sensor default<br>
> * gain and exposure delays. Mark VBLANK for priority write.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>