<div dir="ltr"><div dir="ltr">Hi Laurent, and Nicolas,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 26 Apr 2021 at 04:20, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Nícolas,<br>
<br>
Thank you for the patch.<br>
<br>
On Wed, Apr 21, 2021 at 01:51:39PM -0300, Nícolas F. R. A. Prado wrote:<br>
> Now that the amount of internal buffers allocated is hardcoded by the<br>
> pipelines, and the amount of buffers allocated by the<br>
> FrameBufferAllocator helper is passed through<br>
> FrameBufferAllocator::allocate(), we no longer need to have bufferCount<br>
> in the StreamConfiguration, so remove it.<br>
> <br>
> Signed-off-by: Nícolas F. R. A. Prado <<a href="mailto:nfraprado@collabora.com" target="_blank">nfraprado@collabora.com</a>><br>
> ---<br>
> include/libcamera/stream.h | 2 --<br>
> src/libcamera/pipeline/ipu3/cio2.cpp | 1 -<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +--------------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++-------<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------<br>
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 --<br>
> src/libcamera/pipeline/simple/converter.cpp | 7 ++-----<br>
> src/libcamera/pipeline/simple/converter.h | 5 ++---<br>
> src/libcamera/pipeline/simple/simple.cpp | 15 ++++-----------<br>
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +----<br>
> src/libcamera/pipeline/vimc/vimc.cpp | 5 +----<br>
> src/libcamera/stream.cpp | 9 ++-------<br>
> src/v4l2/v4l2_camera.cpp | 14 ++++++++++----<br>
> src/v4l2/v4l2_camera.h | 5 +++--<br>
> src/v4l2/v4l2_camera_proxy.cpp | 8 +++-----<br>
> test/camera/buffer_import.cpp | 10 +++++++---<br>
> test/libtest/buffer_source.cpp | 4 ++--<br>
> test/libtest/buffer_source.h | 2 +-<br>
> test/v4l2_videodevice/buffer_cache.cpp | 4 ++--<br>
<br>
An update to src/android/ is missing, which breaks compilation of the<br>
HAL :-S<br>
<br>
> 19 files changed, 45 insertions(+), 86 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h<br>
> index bb47c390f8a1..f36aeffd9540 100644<br>
> --- a/include/libcamera/stream.h<br>
> +++ b/include/libcamera/stream.h<br>
> @@ -45,8 +45,6 @@ struct StreamConfiguration {<br>
> unsigned int stride;<br>
> unsigned int frameSize;<br>
> <br>
> - unsigned int bufferCount;<br>
> -<br>
> Stream *stream() const { return stream_; }<br>
> void setStream(Stream *stream) { stream_ = stream; }<br>
> const StreamFormats &formats() const { return formats_; }<br>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp<br>
> index 3cd777d1b742..1e110fe0c189 100644<br>
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp<br>
> @@ -213,7 +213,6 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const<br>
> <br>
> cfg.size = sensorFormat.size;<br>
> cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);<br>
> - cfg.bufferCount = CIO2_BUFFER_COUNT;<br>
> <br>
> return cfg;<br>
> }<br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index c8fcc2fda75f..f0a17a553bd3 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -291,7 +291,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()<br>
> /* Initialize the RAW stream with the CIO2 configuration. */<br>
> cfg->size = cio2Configuration_.size;<br>
> cfg->pixelFormat = cio2Configuration_.pixelFormat;<br>
> - cfg->bufferCount = cio2Configuration_.bufferCount;<br>
> cfg->stride = info.stride(cfg->size.width, 0, 64);<br>
> cfg->frameSize = info.frameSize(cfg->size, 64);<br>
> cfg->setStream(const_cast<Stream *>(&data_->rawStream_));<br>
> @@ -335,7 +334,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()<br>
> IMGU_OUTPUT_HEIGHT_ALIGN);<br>
> <br>
> cfg->pixelFormat = formats::NV12;<br>
> - cfg->bufferCount = IPU3_BUFFER_COUNT;<br>
> cfg->stride = info.stride(cfg->size.width, 0, 1);<br>
> cfg->frameSize = info.frameSize(cfg->size, 1);<br>
> <br>
> @@ -403,7 +401,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,<br>
> Size sensorResolution = data->cio2_.sensor()->resolution();<br>
> for (const StreamRole role : roles) {<br>
> std::map<PixelFormat, std::vector<SizeRange>> streamFormats;<br>
> - unsigned int bufferCount;<br>
> PixelFormat pixelFormat;<br>
> Size size;<br>
> <br>
> @@ -424,7 +421,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,<br>
> size.height = utils::alignDown(size.height - 1,<br>
> IMGU_OUTPUT_HEIGHT_MARGIN);<br>
> pixelFormat = formats::NV12;<br>
> - bufferCount = IPU3_BUFFER_COUNT;<br>
> streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };<br>
> <br>
> break;<br>
> @@ -434,7 +430,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,<br>
> data->cio2_.generateConfiguration(sensorResolution);<br>
> pixelFormat = cio2Config.pixelFormat;<br>
> size = cio2Config.size;<br>
> - bufferCount = cio2Config.bufferCount;<br>
> <br>
> for (const PixelFormat &format : data->cio2_.formats())<br>
> streamFormats[format] = data->cio2_.sizes();<br>
> @@ -453,7 +448,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,<br>
> .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,<br>
> IMGU_OUTPUT_HEIGHT_ALIGN);<br>
> pixelFormat = formats::NV12;<br>
> - bufferCount = IPU3_BUFFER_COUNT;<br>
> streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };<br>
> <br>
> break;<br>
> @@ -470,7 +464,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,<br>
> StreamConfiguration cfg(formats);<br>
> cfg.size = size;<br>
> cfg.pixelFormat = pixelFormat;<br>
> - cfg.bufferCount = bufferCount;<br>
> config->addConfiguration(cfg);<br>
> }<br>
> <br>
> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)<br>
> {<br>
> IPU3CameraData *data = cameraData(camera);<br>
> ImgUDevice *imgu = data->imgu_;<br>
> - unsigned int bufferCount;<br>
> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);<br>
> int ret;<br>
> <br>
> - bufferCount = std::max({<br>
> - data->outStream_.configuration().bufferCount,<br>
> - data->vfStream_.configuration().bufferCount,<br>
> - data->rawStream_.configuration().bufferCount,<br>
> - });<br>
<br>
I'm not sure properties::QueueDepth is the right value here. This deals<br>
with both allocation of stats and params buffers, which are internal,<br>
and the number of V4L2 buffer slots for the ImgU input and output. For<br>
the latter, we probably should overallocate, as underallocation would<br>
mean trashing dmabuf mappings. For the former, we shouldn't allocate too<br>
much to avoid wasting memory, so we should take into account how long<br>
the IPA would need to hold on the params and stats buffers.<br>
<br>
Jacopo, Jean-Michel, Niklas, any comment ?<br>
<br>
> -<br>
> ret = imgu->allocateBuffers(bufferCount);<br>
> if (ret < 0)<br>
> return ret;<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 3f35596fe550..44a8a472ae4f 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -471,7 +471,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> RPiCameraData *data = cameraData(camera);<br>
> CameraConfiguration *config = new RPiCameraConfiguration(data);<br>
> V4L2DeviceFormat sensorFormat;<br>
> - unsigned int bufferCount;<br>
> PixelFormat pixelFormat;<br>
> V4L2VideoDevice::Formats fmts;<br>
> Size size;<br>
> @@ -489,7 +488,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> sensorFormat = findBestMode(fmts, size);<br>
> pixelFormat = sensorFormat.fourcc.toPixelFormat();<br>
> ASSERT(pixelFormat.isValid());<br>
> - bufferCount = 2;<br>
> rawCount++;<br>
> break;<br>
> <br>
> @@ -498,7 +496,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> pixelFormat = formats::NV12;<br>
> /* Return the largest sensor resolution. */<br>
> size = data->sensor_->resolution();<br>
> - bufferCount = 1;<br>
> outCount++;<br>
> break;<br>
> <br>
> @@ -514,7 +511,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> fmts = data->isp_[Isp::Output0].dev()->formats();<br>
> pixelFormat = formats::YUV420;<br>
> size = { 1920, 1080 };<br>
> - bufferCount = 4;<br>
> outCount++;<br>
> break;<br>
> <br>
> @@ -522,7 +518,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> fmts = data->isp_[Isp::Output0].dev()->formats();<br>
> pixelFormat = formats::ARGB8888;<br>
> size = { 800, 600 };<br>
> - bufferCount = 4;<br>
> outCount++;<br>
> break;<br>
> <br>
> @@ -552,7 +547,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,<br>
> StreamConfiguration cfg(formats);<br>
> cfg.size = size;<br>
> cfg.pixelFormat = pixelFormat;<br>
> - cfg.bufferCount = bufferCount;<br>
> config->addConfiguration(cfg);<br>
> }<br>
> <br>
> @@ -1142,6 +1136,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> {<br>
> RPiCameraData *data = cameraData(camera);<br>
> int ret;<br>
> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);<br>
> <br>
> /*<br>
> * Decide how many internal buffers to allocate. For now, simply look<br>
> @@ -1152,7 +1147,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> unsigned int maxBuffers = 0;<br>
> for (const Stream *s : camera->streams())<br>
> if (static_cast<const RPi::Stream *>(s)->isExternal())<br>
> - maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);<br>
> + maxBuffers = std::max(maxBuffers, bufferCount);<br>
<br>
That now looks a bit weird, as bufferCount is constant. This being said,<br>
the above comment for the IPU3 applies here. David, Naush, any comment ?<br></blockquote><div><br></div><div>Similar to what others have mentioned, I don't think properties::QueueDepth should be</div><div>used over here. This bit of code may need reworking after these changes have gone in,</div><div>but for now, a const value for bufferCount might be more suitable.</div><div><br></div><div>Naush</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> for (auto const stream : data->streams_) {<br>
> ret = stream->prepareBuffers(maxBuffers);<br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> index 2d95c1ca2a43..73d4ea6ba8f5 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> @@ -686,16 +686,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)<br>
> unsigned int ipaBufferId = 1;<br>
> int ret;<br>
> <br>
> - unsigned int maxCount = std::max({<br>
> - data->mainPathStream_.configuration().bufferCount,<br>
> - data->selfPathStream_.configuration().bufferCount,<br>
> - });<br>
> -<br>
> - ret = param_->allocateBuffers(maxCount, ¶mBuffers_);<br>
> + ret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers_);<br>
> if (ret < 0)<br>
> goto error;<br>
> <br>
> - ret = stat_->allocateBuffers(maxCount, &statBuffers_);<br>
> + ret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers_);<br>
<br>
Here it should be fine.<br>
<br>
> if (ret < 0)<br>
> goto error;<br>
> <br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> index 25f482eb8d8e..200e3c2c4cca 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> @@ -61,7 +61,6 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)<br>
> StreamConfiguration cfg(formats);<br>
> cfg.pixelFormat = formats::NV12;<br>
> cfg.size = maxResolution;<br>
> - cfg.bufferCount = RKISP1_BUFFER_COUNT;<br>
> <br>
> return cfg;<br>
> }<br>
> @@ -77,7 +76,6 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)<br>
> <br>
> cfg->size.boundTo(maxResolution_);<br>
> cfg->size.expandTo(minResolution_);<br>
> - cfg->bufferCount = RKISP1_BUFFER_COUNT;<br>
> <br>
> V4L2DeviceFormat format;<br>
> format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);<br>
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp<br>
> index 68644ef6477f..54e7f1b051f7 100644<br>
> --- a/src/libcamera/pipeline/simple/converter.cpp<br>
> +++ b/src/libcamera/pipeline/simple/converter.cpp<br>
> @@ -86,9 +86,6 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,<br>
> return -EINVAL;<br>
> }<br>
> <br>
> - inputBufferCount_ = inputCfg.bufferCount;<br>
> - outputBufferCount_ = outputCfg.bufferCount;<br>
> -<br>
> return 0;<br>
> }<br>
> <br>
> @@ -100,11 +97,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,<br>
> <br>
> int SimpleConverter::Stream::start()<br>
> {<br>
> - int ret = m2m_->output()->importBuffers(inputBufferCount_);<br>
> + int ret = m2m_->output()->importBuffers(SIMPLE_QUEUE_DEPTH);<br>
> if (ret < 0)<br>
> return ret;<br>
> <br>
> - ret = m2m_->capture()->importBuffers(outputBufferCount_);<br>
> + ret = m2m_->capture()->importBuffers(SIMPLE_QUEUE_DEPTH);<br>
<br>
As above, we should probably overallocate here to avoid trashing the<br>
dmabuf mappings.<br>
<br>
> if (ret < 0) {<br>
> stop();<br>
> return ret;<br>
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h<br>
> index 480e528d2210..32313748cd24 100644<br>
> --- a/src/libcamera/pipeline/simple/converter.h<br>
> +++ b/src/libcamera/pipeline/simple/converter.h<br>
> @@ -29,6 +29,8 @@ class SizeRange;<br>
> struct StreamConfiguration;<br>
> class V4L2M2MDevice;<br>
> <br>
> +constexpr unsigned int SIMPLE_QUEUE_DEPTH = 3;<br>
> +<br>
> class SimpleConverter<br>
> {<br>
> public:<br>
> @@ -84,9 +86,6 @@ private:<br>
> SimpleConverter *converter_;<br>
> unsigned int index_;<br>
> std::unique_ptr<V4L2M2MDevice> m2m_;<br>
> -<br>
> - unsigned int inputBufferCount_;<br>
> - unsigned int outputBufferCount_;<br>
> };<br>
> <br>
> std::string deviceNode_;<br>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp<br>
> index b9f14be6733f..9f28bb66e2e7 100644<br>
> --- a/src/libcamera/pipeline/simple/simple.cpp<br>
> +++ b/src/libcamera/pipeline/simple/simple.cpp<br>
> @@ -142,8 +142,6 @@ static const SimplePipelineInfo supportedDevices[] = {<br>
> <br>
> } /* namespace */<br>
> <br>
> -static constexpr unsigned int kNumInternalBuffers = 3;<br>
> -<br>
> class SimpleCameraData : public CameraData<br>
> {<br>
> public:<br>
> @@ -425,7 +423,7 @@ int SimpleCameraData::init()<br>
> }<br>
> <br>
> properties_ = sensor_->properties();<br>
> - properties_.set(properties::QueueDepth, kNumInternalBuffers);<br>
> + properties_.set(properties::QueueDepth, SIMPLE_QUEUE_DEPTH);<br>
> <br>
> return 0;<br>
> }<br>
> @@ -616,7 +614,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()<br>
> cfg.size != pipeConfig_->captureSize)<br>
> needConversion_ = true;<br>
> <br>
> - /* Set the stride, frameSize and bufferCount. */<br>
> + /* Set the stride and frameSize. */<br>
> if (needConversion_) {<br>
> std::tie(cfg.stride, cfg.frameSize) =<br>
> converter->strideAndFrameSize(cfg.pixelFormat, cfg.size);<br>
> @@ -634,8 +632,6 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()<br>
> cfg.stride = format.planes[0].bpl;<br>
> cfg.frameSize = format.planes[0].size;<br>
> }<br>
> -<br>
> - cfg.bufferCount = 3;<br>
> }<br>
> <br>
> return status;<br>
> @@ -758,7 +754,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)<br>
> inputCfg.pixelFormat = pipeConfig->captureFormat;<br>
> inputCfg.size = pipeConfig->captureSize;<br>
> inputCfg.stride = captureFormat.planes[0].bpl;<br>
> - inputCfg.bufferCount = kNumInternalBuffers;<br>
> <br>
> return converter_->configure(inputCfg, outputCfgs);<br>
> }<br>
> @@ -791,12 +786,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL<br>
> * When using the converter allocate a fixed number of internal<br>
> * buffers.<br>
> */<br>
> - ret = video->allocateBuffers(kNumInternalBuffers,<br>
> + ret = video->allocateBuffers(SIMPLE_QUEUE_DEPTH,<br>
<br>
This should definitely not overallocate :-) 3 sounds good.<br>
<br>
> &data->converterBuffers_);<br>
> } else {<br>
> - /* Otherwise, prepare for using buffers from the only stream. */<br>
> - Stream *stream = &data->streams_[0];<br>
> - ret = video->importBuffers(stream->configuration().bufferCount);<br>
> + ret = video->importBuffers(SIMPLE_QUEUE_DEPTH);<br>
<br>
Here we can overallocate too.<br>
<br>
> }<br>
> if (ret < 0)<br>
> return ret;<br>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> index a148c35f1265..94e6fd9d2d56 100644<br>
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
> @@ -148,8 +148,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()<br>
> status = Adjusted;<br>
> }<br>
> <br>
> - cfg.bufferCount = 4;<br>
> -<br>
> V4L2DeviceFormat format;<br>
> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);<br>
> format.size = cfg.size;<br>
> @@ -191,7 +189,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,<br>
> <br>
> cfg.pixelFormat = formats.pixelformats().front();<br>
> cfg.size = formats.sizes(cfg.pixelFormat).back();<br>
> - cfg.bufferCount = 4;<br>
> <br>
> config->addConfiguration(cfg);<br>
> <br>
> @@ -236,7 +233,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,<br>
> int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>
> {<br>
> UVCCameraData *data = cameraData(camera);<br>
> - unsigned int count = data->stream_.configuration().bufferCount;<br>
> + unsigned int count = data->properties_.get(properties::QueueDepth);<br>
> <br>
> int ret = data->video_->importBuffers(count);<br>
<br>
Same here, overallocation is best.<br>
<br>
> if (ret < 0)<br>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> index 22d6fdcbb141..891571afb3e5 100644<br>
> --- a/src/libcamera/pipeline/vimc/vimc.cpp<br>
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> @@ -165,8 +165,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()<br>
> status = Adjusted;<br>
> }<br>
> <br>
> - cfg.bufferCount = 4;<br>
> -<br>
> V4L2DeviceFormat format;<br>
> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);<br>
> format.size = cfg.size;<br>
> @@ -222,7 +220,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,<br>
> <br>
> cfg.pixelFormat = formats::BGR888;<br>
> cfg.size = { 1920, 1080 };<br>
> - cfg.bufferCount = 4;<br>
> <br>
> config->addConfiguration(cfg);<br>
> <br>
> @@ -312,7 +309,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,<br>
> int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)<br>
> {<br>
> VimcCameraData *data = cameraData(camera);<br>
> - unsigned int count = data->stream_.configuration().bufferCount;<br>
> + unsigned int count = data->properties_.get(properties::QueueDepth);<br>
> <br>
> int ret = data->video_->importBuffers(count);<br>
<br>
I think you know by now what I would say :-)<br>
<br>
> if (ret < 0)<br>
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp<br>
> index f7bafcf8fc97..be57abce4eb3 100644<br>
> --- a/src/libcamera/stream.cpp<br>
> +++ b/src/libcamera/stream.cpp<br>
> @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const<br>
> * handlers provide StreamFormats.<br>
> */<br>
> StreamConfiguration::StreamConfiguration()<br>
> - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),<br>
> + : pixelFormat(0), stride(0), frameSize(0),<br>
> stream_(nullptr)<br>
> {<br>
> }<br>
> @@ -288,7 +288,7 @@ StreamConfiguration::StreamConfiguration()<br>
> * \brief Construct a configuration with stream formats<br>
> */<br>
> StreamConfiguration::StreamConfiguration(const StreamFormats &formats)<br>
> - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),<br>
> + : pixelFormat(0), stride(0), frameSize(0),<br>
> stream_(nullptr), formats_(formats)<br>
> {<br>
> }<br>
> @@ -323,11 +323,6 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)<br>
> * validating the configuration with a call to CameraConfiguration::validate().<br>
> */<br>
> <br>
> -/**<br>
> - * \var StreamConfiguration::bufferCount<br>
> - * \brief Requested number of buffers to allocate for the stream<br>
> - */<br>
> -<br>
> /**<br>
> * \fn StreamConfiguration::stream()<br>
> * \brief Retrieve the stream associated with the configuration<br>
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp<br>
> index 53d97f3e6b86..79bf7aec7782 100644<br>
> --- a/src/v4l2/v4l2_camera.cpp<br>
> +++ b/src/v4l2/v4l2_camera.cpp<br>
> @@ -10,6 +10,8 @@<br>
> #include <errno.h><br>
> #include <unistd.h><br>
> <br>
> +#include <libcamera/property_ids.h><br>
> +<br>
> #include "libcamera/internal/log.h"<br>
> <br>
> using namespace libcamera;<br>
> @@ -107,14 +109,12 @@ void V4L2Camera::requestComplete(Request *request)<br>
> }<br>
> <br>
> int V4L2Camera::configure(StreamConfiguration *streamConfigOut,<br>
> - const Size &size, const PixelFormat &pixelformat,<br>
> - unsigned int bufferCount)<br>
> + const Size &size, const PixelFormat &pixelformat)<br>
> {<br>
> StreamConfiguration &streamConfig = config_->at(0);<br>
> streamConfig.size.width = size.width;<br>
> streamConfig.size.height = size.height;<br>
> streamConfig.pixelFormat = pixelformat;<br>
> - streamConfig.bufferCount = bufferCount;<br>
> /* \todo memoryType (interval vs external) */<br>
> <br>
> CameraConfiguration::Status validation = config_->validate();<br>
> @@ -146,7 +146,6 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,<br>
> StreamConfiguration &cfg = config->at(0);<br>
> cfg.size = size;<br>
> cfg.pixelFormat = pixelFormat;<br>
> - cfg.bufferCount = 1;<br>
> <br>
> CameraConfiguration::Status validation = config->validate();<br>
> if (validation == CameraConfiguration::Invalid)<br>
> @@ -299,3 +298,10 @@ bool V4L2Camera::isRunning()<br>
> {<br>
> return isRunning_;<br>
> }<br>
> +<br>
> +int V4L2Camera::getBufCount(int count)<br>
> +{<br>
> + int min = camera_->properties().get(properties::QueueDepth);<br>
> +<br>
> + return std::max(count, min);<br>
> +}<br>
<br>
I'd name this function queueDepth() (we don't usually use a "get" prefix<br>
for getters) and return the value of the property only. The caller can<br>
then decide what to do with it.<br>
<br>
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h<br>
> index d238046250e3..68df8ad05917 100644<br>
> --- a/src/v4l2/v4l2_camera.h<br>
> +++ b/src/v4l2/v4l2_camera.h<br>
> @@ -45,8 +45,7 @@ public:<br>
> std::vector<Buffer> completedBuffers();<br>
> <br>
> int configure(StreamConfiguration *streamConfigOut,<br>
> - const Size &size, const PixelFormat &pixelformat,<br>
> - unsigned int bufferCount);<br>
> + const Size &size, const PixelFormat &pixelformat);<br>
> int validateConfiguration(const PixelFormat &pixelformat,<br>
> const Size &size,<br>
> StreamConfiguration *streamConfigOut);<br>
> @@ -65,6 +64,8 @@ public:<br>
> <br>
> bool isRunning();<br>
> <br>
> + int getBufCount(int count);<br>
> +<br>
> private:<br>
> void requestComplete(Request *request);<br>
> <br>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp<br>
> index f8bfe595e90e..cd32e44a01ad 100644<br>
> --- a/src/v4l2/v4l2_camera_proxy.cpp<br>
> +++ b/src/v4l2/v4l2_camera_proxy.cpp<br>
> @@ -348,8 +348,7 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)<br>
> Size size(arg->fmt.pix.width, arg->fmt.pix.height);<br>
> V4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);<br>
> ret = vcam_->configure(&streamConfig_, size,<br>
> - PixelFormatInfo::info(v4l2Format).format,<br>
> - bufferCount_);<br>
> + PixelFormatInfo::info(v4l2Format).format);<br>
> if (ret < 0)<br>
> return -EINVAL;<br>
> <br>
> @@ -490,14 +489,13 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf<br>
> Size size(v4l2PixFormat_.width, v4l2PixFormat_.height);<br>
> V4L2PixelFormat v4l2Format = V4L2PixelFormat(v4l2PixFormat_.pixelformat);<br>
> int ret = vcam_->configure(&streamConfig_, size,<br>
> - PixelFormatInfo::info(v4l2Format).format,<br>
> - arg->count);<br>
> + PixelFormatInfo::info(v4l2Format).format);<br>
> if (ret < 0)<br>
> return -EINVAL;<br>
> <br>
> setFmtFromConfig(streamConfig_);<br>
> <br>
> - arg->count = streamConfig_.bufferCount;<br>
> + arg->count = vcam_->getBufCount(arg->count);<br>
> bufferCount_ = arg->count;<br>
> <br>
> ret = vcam_->allocBuffers(arg->count);<br>
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp<br>
> index 61f4eb92ae95..f2c38bfb0b72 100644<br>
> --- a/test/camera/buffer_import.cpp<br>
> +++ b/test/camera/buffer_import.cpp<br>
> @@ -12,6 +12,8 @@<br>
> #include <numeric><br>
> #include <vector><br>
> <br>
> +#include <libcamera/property_ids.h><br>
> +<br>
> #include "libcamera/internal/device_enumerator.h"<br>
> #include "libcamera/internal/event_dispatcher.h"<br>
> #include "libcamera/internal/media_device.h"<br>
> @@ -91,10 +93,12 @@ protected:<br>
> return TestFail;<br>
> }<br>
> <br>
> + unsigned int bufCount = camera_->properties().get(properties::QueueDepth);<br>
<br>
Let's name the variable bufferCount or numBuffers for consistency.<br>
<br>
There are a few issues to solve, but this is a really nice change !<br>
<br>
> +<br>
> Stream *stream = cfg.stream();<br>
> <br>
> BufferSource source;<br>
> - int ret = source.allocate(cfg);<br>
> + int ret = source.allocate(cfg, bufCount);<br>
> if (ret != TestPass)<br>
> return ret;<br>
> <br>
> @@ -138,10 +142,10 @@ protected:<br>
> while (timer.isRunning())<br>
> dispatcher->processEvents();<br>
> <br>
> - if (completeRequestsCount_ < cfg.bufferCount * 2) {<br>
> + if (completeRequestsCount_ < bufCount * 2) {<br>
> std::cout << "Failed to capture enough frames (got "<br>
> << completeRequestsCount_ << " expected at least "<br>
> - << cfg.bufferCount * 2 << ")" << std::endl;<br>
> + << bufCount * 2 << ")" << std::endl;<br>
> return TestFail;<br>
> }<br>
> <br>
> diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp<br>
> index 73563f2fc39d..c3d5286a2462 100644<br>
> --- a/test/libtest/buffer_source.cpp<br>
> +++ b/test/libtest/buffer_source.cpp<br>
> @@ -24,7 +24,7 @@ BufferSource::~BufferSource()<br>
> media_->release();<br>
> }<br>
> <br>
> -int BufferSource::allocate(const StreamConfiguration &config)<br>
> +int BufferSource::allocate(const StreamConfiguration &config, unsigned int count)<br>
> {<br>
> /* Locate and open the video device. */<br>
> std::string videoDeviceName = "vivid-000-vid-out";<br>
> @@ -77,7 +77,7 @@ int BufferSource::allocate(const StreamConfiguration &config)<br>
> return TestFail;<br>
> }<br>
> <br>
> - if (video->allocateBuffers(config.bufferCount, &buffers_) < 0) {<br>
> + if (video->allocateBuffers(count, &buffers_) < 0) {<br>
> std::cout << "Failed to allocate buffers" << std::endl;<br>
> return TestFail;<br>
> }<br>
> diff --git a/test/libtest/buffer_source.h b/test/libtest/buffer_source.h<br>
> index 14b4770e8d8a..6a18e269a575 100644<br>
> --- a/test/libtest/buffer_source.h<br>
> +++ b/test/libtest/buffer_source.h<br>
> @@ -20,7 +20,7 @@ public:<br>
> BufferSource();<br>
> ~BufferSource();<br>
> <br>
> - int allocate(const StreamConfiguration &config);<br>
> + int allocate(const StreamConfiguration &config, unsigned int count);<br>
> const std::vector<std::unique_ptr<FrameBuffer>> &buffers();<br>
> <br>
> private:<br>
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp<br>
> index b3f2bec11783..07fddfd2617c 100644<br>
> --- a/test/v4l2_videodevice/buffer_cache.cpp<br>
> +++ b/test/v4l2_videodevice/buffer_cache.cpp<br>
> @@ -10,6 +10,7 @@<br>
> #include <vector><br>
> <br>
> #include <libcamera/formats.h><br>
> +#include <libcamera/property_ids.h><br>
> #include <libcamera/stream.h><br>
> <br>
> #include "buffer_source.h"<br>
> @@ -145,10 +146,9 @@ public:<br>
> StreamConfiguration cfg;<br>
> cfg.pixelFormat = formats::YUYV;<br>
> cfg.size = Size(600, 800);<br>
> - cfg.bufferCount = numBuffers;<br>
> <br>
> BufferSource source;<br>
> - int ret = source.allocate(cfg);<br>
> + int ret = source.allocate(cfg, numBuffers);<br>
> if (ret != TestPass)<br>
> return ret;<br>
> <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>