[libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames
Naushir Patuck
naush at raspberrypi.com
Mon Dec 5 12:11:27 CET 2022
Hi Laurent,
On Fri, 2 Dec 2022 at 20:06, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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 ?
>
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.
>
> > > > + * 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?
Regards,
Naush
>
> > > > }
> > > > }
> > > > }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221205/4d442086/attachment.htm>
More information about the libcamera-devel
mailing list