[libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 5 13:31:16 CET 2022
Hi Naush,
On Mon, Dec 05, 2022 at 11:11:27AM +0000, Naushir Patuck wrote:
> On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart wrote:
> > On Fri, Dec 02, 2022 at 01:52:08PM +0000, Naushir Patuck wrote:
> > > On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart wrote:
> > > > On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > > > If the pipeline handler is required to drop startup frames by the IPA, do not
> > > > > call StreamOn on the ISP Output0 and Output1 device nodes from
> > > > > PipelineHandlerRPi::start. This stops the ISP from generating output on those
> > > > > channels giving improving latency, and more crucially does not require internal
> > > > > buffers to be allocated to deal with this condition.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > ---
> > > > > .../pipeline/raspberrypi/data/default.json | 2 +-
> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 34 ++++++++++++++++---
> > > > > 2 files changed, 31 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json
> > > > > index d709e31850af..a7ea735c87f4 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/data/default.json
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/data/default.json
> > > > > @@ -14,7 +14,7 @@
> > > > > # min_total_unicam_buffers - external buffer count)
> > > > > "min_total_unicam_buffers": 4,
> > > > >
> > > > > - # The number of internal buffers used for ISP Output0. This must be set to 1.
> > > > > + # The number of internal buffers used for ISP Output0.
> > > >
> > > > Can you explain this change in the commit message ?
> > > >
> > > > > "num_output0_buffers": 1
> > > > > }
> > > > > }
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index ce411453db0a..86eb43a3a3c5 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > > >
> > > > > data->state_ = RPiCameraData::State::Idle;
> > > > >
> > > > > - /* Start all streams. */
> > > > > + /*
> > > > > + * Start all streams with the exception of ISP Output0 and Output1 if
> > > > > + * we are dropping some start-up frames. This saves a tiny bit of latency
> >
> > What latency does this save, and won't we repay that later anyway ?
>
> I don't have an exact time value, but since the stats are generated at the front of
> the pipeline, no other processing blocks are run if there is no output. Multiply that
> by the number (5-8) of convergence frames, and we get a measurable (though
> still very small) improvement.
Ah, I was considering the start() latency, but you're talking about the
processing latency. Makes sense. Could you mention that here ? Something
along the lines of "a tiny bit of ISP processing latency".
> > > > > + * and avoids the need for allocating an Output0 buffer only to handle
> > > > > + * startup drop frame conditions.
> > > > > + */
> > > >
> > > > I'm not sure to follow you here. Wouldn't it be sufficient to drop the
> > > > buffers at unicam's output without pushing them to the ISP ? Why does
> > > > delaying streamOn() help ?
> > >
> > > You caught me :-)
> > >
> > > I would have done exactly this, only our FW interface does not work that way.
> > > If the kernel device node is opened, we must provide it with an Output0 buffer
> > > to function correctly.
> >
> > This patch addresses VIDIOC_STREAMON, do you then mean if the video
> > device node is streaming, not if it is open ?
>
> Sorry, I did mean VIDIOC_STREAMON!
>
> > > This is purely down to the FW and not the kernel driver.
> > > Given the fragile nature of the FW, I don't want to make any large scale
> > > changes to fix this in FW land.
> >
> > Sometimes I think it could make things easier if the firmware was
> > open-source ;-)
> >
> > More seriously, it seems this could be addressed in kernelspace, by
> > delaying the vchiq_mmal_port_enable() call until the first buffer is
> > queued, which may be as easy as setting vb2_queue.min_buffers_needed to
> > 1. A quick look at videobuf2 doesn't show any immediate ill side
> > effects, but I may have overlooked something. I also understand that
> > changing the kernel would be more complex (as it would need to be synced
> > with merging this series), and thinking more about it, it may be a bit
> > of a hack as it would only work as expect for the startup frame drop.
>
> Indeed, I opted for the userland fix as it does simplify things quite a
> bit.
>
> > > > > for (auto const stream : data->streams_) {
> > > > > + if (data->dropFrameCount_ &&
> > > > > + (stream == &data->isp_[Isp::Output0] ||
> > > > > + stream == &data->isp_[Isp::Output1]))
> > > > > + continue;
> > > > > +
> > > > > ret = stream->dev()->streamOn();
> > > > > if (ret) {
> > > > > stop(camera);
> > > > > @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > > > > if (ret < 0)
> > > > > return ret;
> > > > > } else {
> > > > > + /*
> > > > > + * We don't enable streaming for Output0 and Output1 for
> >
> > Maybe "We haven't enabled streaming yet" to make this clearer ?
>
> Ack.
>
> > > > > + * startup frame drops, so don't queue any buffers.
> > > > > + */
> > > > > + if (stream == &data->isp_[Isp::Output0] ||
> > > > > + stream == &data->isp_[Isp::Output1])
> > > > > + continue;
> >
> > I'd add a blank line here.
> >
> > > > > /*
> > > > > * For external streams, we must queue up a set of internal
> > > > > * buffers to handle the number of drop frames requested by
> > > > > @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()
> > > > > }
> > > > >
> > > > > /*
> > > > > - * Make sure we have three outputs completed in the case of a dropped
> > > > > - * frame.
> > > > > + * Only the ISP statistics output is generated when we are dropping
> > > > > + * frames on startup.
> > > > > */
> > > > > if (state_ == State::IpaComplete &&
> > > > > - ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {
> > > > > + ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {
> > > > > state_ = State::Idle;
> > > > > if (dropFrameCount_) {
> > > > > dropFrameCount_--;
> > > > > LOG(RPI, Debug) << "Dropping frame at the request of the IPA ("
> > > > > << dropFrameCount_ << " left)";
> >
> > Here too.
> >
> > > > > + /*
> > > > > + * If we have finished dropping startup frames, start
> > > > > + * streaming on the ISP Output0 and Output1 nodes for
> > > > > + * normal operation.
> > > > > + */
> > > > > + if (!dropFrameCount_) {
> > > > > + isp_[Isp::Output0].dev()->streamOn();
> > > > > + isp_[Isp::Output1].dev()->streamOn();
> > > > > + }
> >
> > This looks good.
> >
> > I think there's a potential issue though. Given that the output queues
> > are not started in start(), but buffers are still queued in
> > queueRequestDevice(), what will happen if the application calls stop()
> > before the video nodes are started ? Won't the buffers stay queued ?
>
> PipelineHandlerRPi::stopDevice() unconditionally calls V4L2VideoDevice::streamOff()
> for the ISP nodes even if they were not started. I was assuming this would unqueue
> the buffers and clean up correctly. Do you think this might not be correct?
I've had another look at the kernel code and the
V4L2VideoDevice::streamOff() implementation, and I think it's fine. It
would still be nice if you could test it though.
> > > > > }
> > > > > }
> > > > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list