<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Dec 2022 at 13:44, 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>
Thank you for the patch.<br>
<br>
On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> If the pipeline handler is required to drop startup frames by the IPA, do not<br>
> call StreamOn on the ISP Output0 and Output1 device nodes from<br>
> PipelineHandlerRPi::start. This stops the ISP from generating output on those<br>
> channels giving improving latency, and more crucially does not require internal<br>
> buffers to be allocated to deal with this condition.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/data/default.json | 2 +-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---<br>
> 2 files changed, 31 insertions(+), 5 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> index d709e31850af..a7ea735c87f4 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json<br>
> @@ -14,7 +14,7 @@<br>
> # min_total_unicam_buffers - external buffer count)<br>
> "min_total_unicam_buffers": 4,<br>
> <br>
> - # The number of internal buffers used for ISP Output0. This must be set to 1.<br>
> + # The number of internal buffers used for ISP Output0.<br>
<br>
Can you explain this change in the commit message ?<br>
<br>
> "num_output0_buffers": 1<br>
> }<br>
> }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index ce411453db0a..86eb43a3a3c5 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> <br>
> data->state_ = RPiCameraData::State::Idle;<br>
> <br>
> - /* Start all streams. */<br>
> + /*<br>
> + * Start all streams with the exception of ISP Output0 and Output1 if<br>
> + * we are dropping some start-up frames. This saves a tiny bit of latency<br>
> + * and avoids the need for allocating an Output0 buffer only to handle<br>
> + * startup drop frame conditions.<br>
> + */<br>
<br>
I'm not sure to follow you here. Wouldn't it be sufficient to drop the<br>
buffers at unicam's output without pushing them to the ISP ? Why does<br>
delaying streamOn() help ?<br></blockquote><div><br></div><div>You caught me :-)</div><div><br></div><div>I would have done exactly this, only our FW interface does not work that way.</div><div>If the kernel device node is opened, we must provide it with an Output0 buffer</div><div>to function correctly. This is purely down to the FW and not the kernel driver.</div><div>Given the fragile nature of the FW, I don't want to make any large scale</div><div>changes to fix this in FW land.</div><div><br></div><div>Naush</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>
> for (auto const stream : data->streams_) {<br>
> + if (data->dropFrameCount_ &&<br>
> + (stream == &data->isp_[Isp::Output0] ||<br>
> + stream == &data->isp_[Isp::Output1]))<br>
> + continue;<br>
> +<br>
> ret = stream->dev()->streamOn();<br>
> if (ret) {<br>
> stop(camera);<br>
> @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)<br>
> if (ret < 0)<br>
> return ret;<br>
> } else {<br>
> + /*<br>
> + * We don't enable streaming for Output0 and Output1 for<br>
> + * startup frame drops, so don't queue any buffers.<br>
> + */<br>
> + if (stream == &data->isp_[Isp::Output0] ||<br>
> + stream == &data->isp_[Isp::Output1])<br>
> + continue;<br>
> /*<br>
> * For external streams, we must queue up a set of internal<br>
> * buffers to handle the number of drop frames requested by<br>
> @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()<br>
> }<br>
> <br>
> /*<br>
> - * Make sure we have three outputs completed in the case of a dropped<br>
> - * frame.<br>
> + * Only the ISP statistics output is generated when we are dropping<br>
> + * frames on startup.<br>
> */<br>
> if (state_ == State::IpaComplete &&<br>
> - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {<br>
> + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {<br>
> state_ = State::Idle;<br>
> if (dropFrameCount_) {<br>
> dropFrameCount_--;<br>
> LOG(RPI, Debug) << "Dropping frame at the request of the IPA ("<br>
> << dropFrameCount_ << " left)";<br>
> + /*<br>
> + * If we have finished dropping startup frames, start<br>
> + * streaming on the ISP Output0 and Output1 nodes for<br>
> + * normal operation.<br>
> + */<br>
> + if (!dropFrameCount_) {<br>
> + isp_[Isp::Output0].dev()->streamOn();<br>
> + isp_[Isp::Output1].dev()->streamOn();<br>
> + }<br>
> }<br>
> }<br>
> }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>