[libcamera-devel,v3,08/13] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames

17951 diff mbox series

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 improved latency, and more crucially does not require internal
buffers to be allocated to deal with this condition.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)

Hi,

Unfortunately this patch has introduced a tiny regression.  It turns out
that
calling stream on for an ISP node takes some hundreds of milliseconds to
complete.
Most of this time is communicating and setup within the firmware layer.  In
video encode use cases, this will cause a frame drop if we do not have
enough
buffers to smooth out the glitch.

Not wanting to spend too much time and effort optimising this in the
firmware
(particularly because I will likely break something else!), I will remove
this
patch from the series, and we accept that we have to allocate a single
buffer if
we have to drop frames for AE convergence.  This is not disastrous, as
typically
this will be a viewfinder size buffer.

I will post an updated series shortly.

Thanks,
Naush


On Tue, 6 Dec 2022 at 13:55, Naushir Patuck <naush@raspberrypi.com> 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 improved latency, and more crucially does not require
> internal
> buffers to be allocated to deal with this condition.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++++++++---
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9a316eda6fea..302279ac5e01 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1088,8 +1088,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
> +        * and avoids the need for allocating an Output0 buffer only to
> handle
> +        * startup drop frame conditions.
> +        */
>         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);
> @@ -1425,6 +1435,14 @@ int PipelineHandlerRPi::queueAllBuffers(Camera
> *camera)
>                         if (ret < 0)
>                                 return ret;
>                 } else {
> +                       /*
> +                        * We haven't enabled streaming yet for Output0
> and Output1
> +                        * for startup frame drops, so don't queue any
> buffers.
> +                        */
> +                       if (stream == &data->isp_[Isp::Output0] ||
> +                           stream == &data->isp_[Isp::Output1])
> +                               continue;
> +
>                         /*
>                          * For external streams, we must queue up a set of
> internal
>                          * buffers to handle the number of drop frames
> requested by
> @@ -2158,16 +2176,26 @@ 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)";
> +
> +                       /*
> +                        * 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();
> +                       }
>                 }
>         }
>  }
> --
> 2.25.1
>
>