<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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 Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote:<br>
> On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote:<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>
<br>
What latency does this save, and won't we repay that later anyway ?<br></blockquote><div><br></div><div>I don't have an exact time value, but since the stats are generated at the front of</div><div>the pipeline, no other processing blocks are run if there is no output. Multiply that</div><div>by the number (5-8) of convergence frames, and we get a measurable (though</div><div>still very small) improvement.</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>
> > > + * 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>
> <br>
> You caught me :-)<br>
> <br>
> I would have done exactly this, only our FW interface does not work that way.<br>
> If the kernel device node is opened, we must provide it with an Output0 buffer<br>
> to function correctly.<br>
<br>
This patch addresses VIDIOC_STREAMON, do you then mean if the video<br>
device node is streaming, not if it is open ?<br></blockquote><div><br></div><div>Sorry, I did mean VIDIOC_STREAMON!</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>
> This is purely down to the FW and not the kernel driver.<br>
> Given the fragile nature of the FW, I don't want to make any large scale<br>
> changes to fix this in FW land.<br>
<br>
Sometimes I think it could make things easier if the firmware was<br>
open-source ;-)<br>
<br>
More seriously, it seems this could be addressed in kernelspace, by<br>
delaying the vchiq_mmal_port_enable() call until the first buffer is<br>
queued, which may be as easy as setting vb2_queue.min_buffers_needed to<br>
1. A quick look at videobuf2 doesn't show any immediate ill side<br>
effects, but I may have overlooked something. I also understand that<br>
changing the kernel would be more complex (as it would need to be synced<br>
with merging this series), and thinking more about it, it may be a bit<br>
of a hack as it would only work as expect for the startup frame drop.<br></blockquote><div><br></div><div>Indeed, I opted for the userland fix as it does simplify things quite a bit. </div><div> <br></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>
<br>
Maybe "We haven't enabled streaming yet" to make this clearer ?<br></blockquote><div><br></div><div>Ack.</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>
> > > + * 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>
I'd add a blank line here.<br>
<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>
Here too.<br>
<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>
This looks good.<br>
<br>
I think there's a potential issue though. Given that the output queues<br>
are not started in start(), but buffers are still queued in<br>
queueRequestDevice(), what will happen if the application calls stop()<br>
before the video nodes are started ? Won't the buffers stay queued ?<br></blockquote><div><br></div><div>PipelineHandlerRPi::stopDevice() unconditionally calls V4L2VideoDevice::streamOff()<br></div><div>for the ISP nodes even if they were not started. I was assuming this would unqueue</div><div>the buffers and clean up correctly. Do you think this might not be correct?</div><div><br></div><div>Regards,</div><div>Naush</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>
> > > }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>