[libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer import count to 32
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 4 13:23:50 CEST 2023
Quoting Naushir Patuck (2023-09-04 10:04:41)
> 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.
No point sending a v3 just for this change I don't think.
You could go direct to a PR with this fixed.
--
Kieran
>
> 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
> > > >
> >
More information about the libcamera-devel
mailing list