[libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer import count to 32

[Naushir Patuck]

Hi Kieran,

Thanks for the review.

On Mon, 4 Sept 2023 at 09:44, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Jacopo Mondi via libcamera-devel (2023-07-27 10:31:28)
> > Hi Naush
> >
> > On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > > Hardcode the maximum number of buffers imported to the V4L2 video
> device
> > > to 32. This only has a minor disadvantage of over-allocating cache
> slots
> > > and V4L2 buffer indexes, but does allow more headroom for using dma
> > > buffers allocated from outside of libcamera.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  .../pipeline/rpi/common/rpi_stream.cpp        | 33 +++++--------------
> > >  1 file changed, 8 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > > index c158843cb0ed..e9ad1e6f0848 100644
> > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > > @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)
> > >       int ret;
> > >
> > >       if (!(flags_ & StreamFlag::ImportOnly)) {
> > > -             if (count) {
> > > -                     /* Export some frame buffers for internal use. */
> > > -                     ret = dev_->exportBuffers(count,
> &internalBuffers_);
> > > -                     if (ret < 0)
> > > -                             return ret;
> > > -
> > > -                     /* Add these exported buffers to the
> internal/external buffer list. */
> > > -                     setExportedBuffers(&internalBuffers_);
> > > -                     resetBuffers();
> > > -             }
> > > +             /* Export some frame buffers for internal use. */
> > > +             ret = dev_->exportBuffers(count, &internalBuffers_);
> > > +             if (ret < 0)
> > > +                     return ret;
> > >
> > > -             /* We must import all internal/external exported
> buffers. */
> > > -             count = bufferMap_.size();
> > > +             /* Add these exported buffers to the internal/external
> buffer list. */
> > > +             setExportedBuffers(&internalBuffers_);
> > > +             resetBuffers();
> > >       }
> > >
> > > -     /*
> > > -      * If this is an external stream, we must allocate slots for
> buffers that
> > > -      * might be externally allocated. We have no indication of how
> many buffers
> > > -      * may be used, so this might overallocate slots in the buffer
> cache.
> > > -      * Similarly, if this stream is only importing buffers, we do
> the same.
> > > -      *
> > > -      * \todo Find a better heuristic, or, even better, an exact
> solution to
> > > -      * this issue.
> > > -      */
> > > -     if ((flags_ & StreamFlag::External) || (flags_ &
> StreamFlag::ImportOnly))
> > > -             count = count * 2;
> > > -
> > > -     return dev_->importBuffers(count);
> > > +     return dev_->importBuffers(32);
> >
> > Might be good to define 32 somewhere as constexpr to avoid magic
> > numbers, but apart from this
>
> Agreed here. Something like
>
> /** The maximum buffer allocation possible with V4L2 */
> constexpr maximumV4L2Buffers = 32;
>

Ack, I'll make this change for v3.

Naush


>
> to express that this is the maximum possible with V4L2....
>
> But otherwise
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> >   j
> >
> > >  }
> > >
> > >  int Stream::queueBuffer(FrameBuffer *buffer)
> > > --
> > > 2.34.1
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230904/524dc961/attachment.htm>