| Message ID | 20230721093759.27700-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Fri, Jul 21, 2023 at 10:37:56AM +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@raspberrypi.com> > --- > .../pipeline/rpi/common/rpi_stream.cpp | 35 +++++-------------- > 1 file changed, 9 insertions(+), 26 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index c158843cb0ed..1d05c5acc0d9 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -94,35 +94,18 @@ 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(); > - } > + if (!(flags_ & StreamFlag::ImportOnly) && count) { Is there a case where this functions is called with a 0 'count' ? > + /* 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); > } > > int Stream::queueBuffer(FrameBuffer *buffer) > -- > 2.34.1 >
Hi Jacopo, On Mon, 24 Jul 2023 at 08:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Jul 21, 2023 at 10:37:56AM +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@raspberrypi.com> > > --- > > .../pipeline/rpi/common/rpi_stream.cpp | 35 +++++-------------- > > 1 file changed, 9 insertions(+), 26 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > index c158843cb0ed..1d05c5acc0d9 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > @@ -94,35 +94,18 @@ 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(); > > - } > > + if (!(flags_ & StreamFlag::ImportOnly) && count) { > > Is there a case where this functions is called with a 0 'count' ? I *think* count will always be non-zero. So the test above is probably redundant! Regards, Naush > > > + /* 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); > > } > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > -- > > 2.34.1 > >
diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp index c158843cb0ed..1d05c5acc0d9 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp @@ -94,35 +94,18 @@ 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(); - } + if (!(flags_ & StreamFlag::ImportOnly) && count) { + /* 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); } int Stream::queueBuffer(FrameBuffer *buffer)
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@raspberrypi.com> --- .../pipeline/rpi/common/rpi_stream.cpp | 35 +++++-------------- 1 file changed, 9 insertions(+), 26 deletions(-)