[libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 2 21:06:36 CET 2022
Hi Naush,
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 ?
> > > + * 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 ?
> 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.
> > > 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 ?
> > > + * 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 ?
> > > }
> > > }
> > > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list