[libcamera-devel] [PATCH v2 03/10] pipeline: raspberrypi: Split out ISP Output0 buffer allocation

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback!

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

> Hi Naush,
>
> Thank you for the patch.
>
> On Tue, Nov 29, 2022 at 01:45:27PM +0000, Naushir Patuck via
> libcamera-devel wrote:
> > Add a new config parameter numOutput0Buffers specifying the number of
> internally
> > allocated ISP Output0 buffers. This parameter defaults to 1.
> >
> > Split out the buffer allocation logic so that the buffer count for ISP
> Output0
> > can be different from the ISP Output1 and Statistics streams.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 +++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 4486d31ea78d..f2b695af2399 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -305,6 +305,11 @@ public:
> >                * the Unicam Image steram.
> >                */
> >               unsigned int minTotalUnicamBuffers;
> > +             /*
> > +              * The number of internal buffers allocated for the ISP
> Output0
> > +              * stream.
>
> I'd comment here that the value can only be 0 or 1.
>

Ack.


>
> > +              */
> > +             unsigned int numOutput0Buffers;
> >       };
> >
> >       Config config_;
> > @@ -1418,6 +1423,7 @@ int
> PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)
> >       config = {
> >               .minUnicamBuffers = 2,
> >               .minTotalUnicamBuffers = 4,
> > +             .numOutput0Buffers = 1,
> >       };
> >
> >       return 0;
> > @@ -1502,12 +1508,19 @@ int PipelineHandlerRPi::prepareBuffers(Camera
> *camera)
> >                        * so allocate the minimum required to avoid frame
> drops.
> >                        */
> >                       numBuffers = minBuffers;
> > -             } else {
> > +             } else if (stream == &data->isp_[Isp::Output0]) {
> >                       /*
> >                        * 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().
> > +                      * 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().
> > +                      */
> > +                     numBuffers = std::min(1u,
> data->config_.numOutput0Buffers);
>
> Is it OK to call Stream::prepareBuffers() below if numBuffers is 0 ?
>

Yes it is. The count param tells the stream how many (if any) buffers to
allocate for internal use. Stream::prepareBuffers will call
V4L2VideoDevice::importBuffers
for externally/internally allocated buffers.

Regards,
Naush


>
> > +             } else {
> > +                     /*
> > +                      * Same reasoning as above, we only ever need a
> maximum
> > +                      * of one internal buffer for Output1 (required
> for colour
> > +                      * denoise) and ISP statistics.
> >                        */
> >                       numBuffers = 1;
> >               }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221202/449c529a/attachment.htm>