[libcamera-devel] [PATCH v5 08/12] pipeline: raspberrypi: Handle OptionalStream hints for ISP Output0

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Sun, 22 Jan 2023 at 22:45, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jan 18, 2023 at 08:59:49AM +0000, Naushir Patuck via libcamera-devel wrote:
> > Look for OptionalStream flag in the hints field of the ISP Output0
> > StreamConfiguration structure. If this flag is not set, it guarantees that the
> > application will provide buffers for the ISP, do not allocate any internal
> > buffers for the device.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 29 ++++++++++++++-----
> >  1 file changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 2b396f1db9b6..13d0ab4c4968 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1453,7 +1453,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >       RPiCameraData *data = cameraData(camera);
> >       unsigned int minUnicamBuffers = data->config_.minUnicamBuffers,
> >                    minTotalUnicamBuffers = data->config_.minTotalUnicamBuffers,
> > -                  numRawBuffers = 0;
> > +                  numRawBuffers = 0, minIspBuffers = 1;
>
> One variable per line.

Ack.

>
> >       int ret;
> >
> >       for (Stream *s : camera->streams()) {
> > @@ -1477,8 +1477,21 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                                       minTotalUnicamBuffers = 0;
> >                               }
> >                       }
> > -
> > -                     break;
> > +             } else if (s == &data->isp_[Isp::Output0]) {
> > +                     /*
> > +                      * Since the ISP runs synchronous with the IPA and requests,
> > +                      * we only ever need a maximum of one internal buffer. Any
> > +                      * buffers the application wants to hold onto will already
> > +                      * be exported through PipelineHandlerRPi::exportFrameBuffers().
> > +                      *
> > +                      * However, as above, if the application provides a guarantee
> > +                      * that the buffer will always be provided for the ISP Output0
> > +                      * stream in a Request, we don't need any internal buffers
> > +                      * allocated.
> > +                      */
> > +                     if (!(s->configuration().hints & StreamConfiguration::Hint::OptionalStream) &&
> > +                         !data->dropFrameCount_)
> > +                             minIspBuffers = 0;
> >               }
> >       }
> >
> > @@ -1514,12 +1527,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                        * so allocate the minimum required to avoid frame drops.
> >                        */
> >                       numBuffers = data->config_.minTotalUnicamBuffers;
> > +             } else if (stream == &data->isp_[Isp::Output0]) {
> > +                     /* Buffer count for this is handled in the earlier loop above. */
> > +                     numBuffers = minIspBuffers;
> >               } else {
> >                       /*
> > -                      * Since the ISP runs synchronous with the IPA and requests,
> > -                      * we only ever need one set of internal buffers. Any buffers
> > -                      * the application wants to hold onto will already be exported
> > -                      * through PipelineHandlerRPi::exportFrameBuffers().
> > +                      * Same reasoning as for ISP Output 0, we only ever need
> > +                      * a maximum of one internal buffer for Output1 (required
> > +                      * for colour denoise) and ISP statistics.
> >                        */
> >                       numBuffers = 1;
>
> Do you plan to also optimize the buffer allocation based on hints for
> Output1 ?

No, I'll leave Output1 as-is.  It's only ever one extra (much lower resolution)
frame buffer.  It is also best to keep it around as we use it for colour denoise
analysis internally.

Regards,
Naush

>
> >               }
>
> --
> Regards,
>
> Laurent Pinchart