[libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Fri, 2 Dec 2022 at 13:44, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> 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
> > +      * 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 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.

Naush



>
> >       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
> > +                      * 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
> > @@ -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)";
> > +                     /*
> > +                      * 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();
> > +                     }
> >               }
> >       }
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221202/8c95b733/attachment.htm>