[{"id":16553,"web_url":"https://patchwork.libcamera.org/comment/16553/","msgid":"","date":"2021-04-26T03:19:57","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Wed, Apr 21, 2021 at 01:51:39PM -0300, Nícolas F. R. A. Prado wrote:\n> Now that the amount of internal buffers allocated is hardcoded by the\n> pipelines, and the amount of buffers allocated by the\n> FrameBufferAllocator helper is passed through\n> FrameBufferAllocator::allocate(), we no longer need to have bufferCount\n> in the StreamConfiguration, so remove it.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> include/libcamera/stream.h | 2 --\n> src/libcamera/pipeline/ipu3/cio2.cpp | 1 -\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +--------------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 --\n> src/libcamera/pipeline/simple/converter.cpp | 7 ++-----\n> src/libcamera/pipeline/simple/converter.h | 5 ++---\n> src/libcamera/pipeline/simple/simple.cpp | 15 ++++-----------\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +----\n> src/libcamera/pipeline/vimc/vimc.cpp | 5 +----\n> src/libcamera/stream.cpp | 9 ++-------\n> src/v4l2/v4l2_camera.cpp | 14 ++++++++++----\n> src/v4l2/v4l2_camera.h | 5 +++--\n> src/v4l2/v4l2_camera_proxy.cpp | 8 +++-----\n> test/camera/buffer_import.cpp | 10 +++++++---\n> test/libtest/buffer_source.cpp | 4 ++--\n> test/libtest/buffer_source.h | 2 +-\n> test/v4l2_videodevice/buffer_cache.cpp | 4 ++--\n\nAn update to src/android/ is missing, which breaks compilation of the\nHAL :-S\n\n> 19 files changed, 45 insertions(+), 86 deletions(-)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index bb47c390f8a1..f36aeffd9540 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -45,8 +45,6 @@ struct StreamConfiguration {\n> \tunsigned int stride;\n> \tunsigned int frameSize;\n> \n> -\tunsigned int bufferCount;\n> -\n> \tStream *stream() const { return stream_; }\n> \tvoid setStream(Stream *stream) { stream_ = stream; }\n> \tconst StreamFormats &formats() const { return formats_; }\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 3cd777d1b742..1e110fe0c189 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -213,7 +213,6 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> \n> \tcfg.size = sensorFormat.size;\n> \tcfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);\n> -\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> \n> \treturn cfg;\n> }\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c8fcc2fda75f..f0a17a553bd3 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -291,7 +291,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> \t\t\tcfg->size = cio2Configuration_.size;\n> \t\t\tcfg->pixelFormat = cio2Configuration_.pixelFormat;\n> -\t\t\tcfg->bufferCount = cio2Configuration_.bufferCount;\n> \t\t\tcfg->stride = info.stride(cfg->size.width, 0, 64);\n> \t\t\tcfg->frameSize = info.frameSize(cfg->size, 64);\n> \t\t\tcfg->setStream(const_cast(&data_->rawStream_));\n> @@ -335,7 +334,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\t\t\t IMGU_OUTPUT_HEIGHT_ALIGN);\n> \n> \t\t\tcfg->pixelFormat = formats::NV12;\n> -\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> \t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> \t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> \n> @@ -403,7 +401,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \tSize sensorResolution = data->cio2_.sensor()->resolution();\n> \tfor (const StreamRole role : roles) {\n> \t\tstd::map> streamFormats;\n> -\t\tunsigned int bufferCount;\n> \t\tPixelFormat pixelFormat;\n> \t\tSize size;\n> \n> @@ -424,7 +421,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\tsize.height = utils::alignDown(size.height - 1,\n> \t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> \t\t\tpixelFormat = formats::NV12;\n> -\t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> \t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> \n> \t\t\tbreak;\n> @@ -434,7 +430,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\t\tdata->cio2_.generateConfiguration(sensorResolution);\n> \t\t\tpixelFormat = cio2Config.pixelFormat;\n> \t\t\tsize = cio2Config.size;\n> -\t\t\tbufferCount = cio2Config.bufferCount;\n> \n> \t\t\tfor (const PixelFormat &format : data->cio2_.formats())\n> \t\t\t\tstreamFormats[format] = data->cio2_.sizes();\n> @@ -453,7 +448,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\t\t\t .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> \t\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_ALIGN);\n> \t\t\tpixelFormat = formats::NV12;\n> -\t\t\tbufferCount = IPU3_BUFFER_COUNT;\n> \t\t\tstreamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };\n> \n> \t\t\tbreak;\n> @@ -470,7 +464,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\tStreamConfiguration cfg(formats);\n> \t\tcfg.size = size;\n> \t\tcfg.pixelFormat = pixelFormat;\n> -\t\tcfg.bufferCount = bufferCount;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> \n> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tImgUDevice *imgu = data->imgu_;\n> -\tunsigned int bufferCount;\n> +\tunsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n> \tint ret;\n> \n> -\tbufferCount = std::max({\n> -\t\tdata->outStream_.configuration().bufferCount,\n> -\t\tdata->vfStream_.configuration().bufferCount,\n> -\t\tdata->rawStream_.configuration().bufferCount,\n> -\t});\n\nI'm not sure properties::QueueDepth is the right value here. This deals\nwith both allocation of stats and params buffers, which are internal,\nand the number of V4L2 buffer slots for the ImgU input and output. For\nthe latter, we probably should overallocate, as underallocation would\nmean trashing dmabuf mappings. For the former, we shouldn't allocate too\nmuch to avoid wasting memory, so we should take into account how long\nthe IPA would need to hold on the params and stats buffers.\n\nJacopo, Jean-Michel, Niklas, any comment ?\n\n> -\n> \tret = imgu->allocateBuffers(bufferCount);\n> \tif (ret < 0)\n> \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 3f35596fe550..44a8a472ae4f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -471,7 +471,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \tRPiCameraData *data = cameraData(camera);\n> \tCameraConfiguration *config = new RPiCameraConfiguration(data);\n> \tV4L2DeviceFormat sensorFormat;\n> -\tunsigned int bufferCount;\n> \tPixelFormat pixelFormat;\n> \tV4L2VideoDevice::Formats fmts;\n> \tSize size;\n> @@ -489,7 +488,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tsensorFormat = findBestMode(fmts, size);\n> \t\t\tpixelFormat = sensorFormat.fourcc.toPixelFormat();\n> \t\t\tASSERT(pixelFormat.isValid());\n> -\t\t\tbufferCount = 2;\n> \t\t\trawCount++;\n> \t\t\tbreak;\n> \n> @@ -498,7 +496,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tpixelFormat = formats::NV12;\n> \t\t\t/* Return the largest sensor resolution. */\n> \t\t\tsize = data->sensor_->resolution();\n> -\t\t\tbufferCount = 1;\n> \t\t\toutCount++;\n> \t\t\tbreak;\n> \n> @@ -514,7 +511,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> \t\t\tpixelFormat = formats::YUV420;\n> \t\t\tsize = { 1920, 1080 };\n> -\t\t\tbufferCount = 4;\n> \t\t\toutCount++;\n> \t\t\tbreak;\n> \n> @@ -522,7 +518,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> \t\t\tpixelFormat = formats::ARGB8888;\n> \t\t\tsize = { 800, 600 };\n> -\t\t\tbufferCount = 4;\n> \t\t\toutCount++;\n> \t\t\tbreak;\n> \n> @@ -552,7 +547,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\tStreamConfiguration cfg(formats);\n> \t\tcfg.size = size;\n> \t\tcfg.pixelFormat = pixelFormat;\n> -\t\tcfg.bufferCount = bufferCount;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> \n> @@ -1142,6 +1136,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> \tint ret;\n> +\tunsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n> \n> \t/*\n> \t * Decide how many internal buffers to allocate. For now, simply look\n> @@ -1152,7 +1147,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \tunsigned int maxBuffers = 0;\n> \tfor (const Stream *s : camera->streams())\n> \t\tif (static_cast(s)->isExternal())\n> -\t\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> +\t\t\tmaxBuffers = std::max(maxBuffers, bufferCount);\n\nThat now looks a bit weird, as bufferCount is constant. This being said,\nthe above comment for the IPU3 applies here. David, Naush, any comment ?\n\n> \n> \tfor (auto const stream : data->streams_) {\n> \t\tret = stream->prepareBuffers(maxBuffers);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 2d95c1ca2a43..73d4ea6ba8f5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -686,16 +686,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> \tunsigned int ipaBufferId = 1;\n> \tint ret;\n> \n> -\tunsigned int maxCount = std::max({\n> -\t\tdata->mainPathStream_.configuration().bufferCount,\n> -\t\tdata->selfPathStream_.configuration().bufferCount,\n> -\t});\n> -\n> -\tret = param_->allocateBuffers(maxCount, ¶mBuffers_);\n> +\tret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers_);\n> \tif (ret < 0)\n> \t\tgoto error;\n> \n> -\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> +\tret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers_);\n\nHere it should be fine.\n\n> \tif (ret < 0)\n> \t\tgoto error;\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 25f482eb8d8e..200e3c2c4cca 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -61,7 +61,6 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)\n> \tStreamConfiguration cfg(formats);\n> \tcfg.pixelFormat = formats::NV12;\n> \tcfg.size = maxResolution;\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> \n> \treturn cfg;\n> }\n> @@ -77,7 +76,6 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)\n> \n> \tcfg->size.boundTo(maxResolution_);\n> \tcfg->size.expandTo(minResolution_);\n> -\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> \n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);\n> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\n> index 68644ef6477f..54e7f1b051f7 100644\n> --- a/src/libcamera/pipeline/simple/converter.cpp\n> +++ b/src/libcamera/pipeline/simple/converter.cpp\n> @@ -86,9 +86,6 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tinputBufferCount_ = inputCfg.bufferCount;\n> -\toutputBufferCount_ = outputCfg.bufferCount;\n> -\n> \treturn 0;\n> }\n> \n> @@ -100,11 +97,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,\n> \n> int SimpleConverter::Stream::start()\n> {\n> -\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n> +\tint ret = m2m_->output()->importBuffers(SIMPLE_QUEUE_DEPTH);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n> +\tret = m2m_->capture()->importBuffers(SIMPLE_QUEUE_DEPTH);\n\nAs above, we should probably overallocate here to avoid trashing the\ndmabuf mappings.\n\n> \tif (ret < 0) {\n> \t\tstop();\n> \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\n> index 480e528d2210..32313748cd24 100644\n> --- a/src/libcamera/pipeline/simple/converter.h\n> +++ b/src/libcamera/pipeline/simple/converter.h\n> @@ -29,6 +29,8 @@ class SizeRange;\n> struct StreamConfiguration;\n> class V4L2M2MDevice;\n> \n> +constexpr unsigned int SIMPLE_QUEUE_DEPTH = 3;\n> +\n> class SimpleConverter\n> {\n> public:\n> @@ -84,9 +86,6 @@ private:\n> \t\tSimpleConverter *converter_;\n> \t\tunsigned int index_;\n> \t\tstd::unique_ptr m2m_;\n> -\n> -\t\tunsigned int inputBufferCount_;\n> -\t\tunsigned int outputBufferCount_;\n> \t};\n> \n> \tstd::string deviceNode_;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index b9f14be6733f..9f28bb66e2e7 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -142,8 +142,6 @@ static const SimplePipelineInfo supportedDevices[] = {\n> \n> } /* namespace */\n> \n> -static constexpr unsigned int kNumInternalBuffers = 3;\n> -\n> class SimpleCameraData : public CameraData\n> {\n> public:\n> @@ -425,7 +423,7 @@ int SimpleCameraData::init()\n> \t}\n> \n> \tproperties_ = sensor_->properties();\n> -\tproperties_.set(properties::QueueDepth, kNumInternalBuffers);\n> +\tproperties_.set(properties::QueueDepth, SIMPLE_QUEUE_DEPTH);\n> \n> \treturn 0;\n> }\n> @@ -616,7 +614,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \t\t cfg.size != pipeConfig_->captureSize)\n> \t\t\tneedConversion_ = true;\n> \n> -\t\t/* Set the stride, frameSize and bufferCount. */\n> +\t\t/* Set the stride and frameSize. */\n> \t\tif (needConversion_) {\n> \t\t\tstd::tie(cfg.stride, cfg.frameSize) =\n> \t\t\t\tconverter->strideAndFrameSize(cfg.pixelFormat, cfg.size);\n> @@ -634,8 +632,6 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \t\t\tcfg.stride = format.planes[0].bpl;\n> \t\t\tcfg.frameSize = format.planes[0].size;\n> \t\t}\n> -\n> -\t\tcfg.bufferCount = 3;\n> \t}\n> \n> \treturn status;\n> @@ -758,7 +754,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \tinputCfg.pixelFormat = pipeConfig->captureFormat;\n> \tinputCfg.size = pipeConfig->captureSize;\n> \tinputCfg.stride = captureFormat.planes[0].bpl;\n> -\tinputCfg.bufferCount = kNumInternalBuffers;\n> \n> \treturn converter_->configure(inputCfg, outputCfgs);\n> }\n> @@ -791,12 +786,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\t * When using the converter allocate a fixed number of internal\n> \t\t * buffers.\n> \t\t */\n> -\t\tret = video->allocateBuffers(kNumInternalBuffers,\n> +\t\tret = video->allocateBuffers(SIMPLE_QUEUE_DEPTH,\n\nThis should definitely not overallocate :-) 3 sounds good.\n\n> \t\t\t\t\t &data->converterBuffers_);\n> \t} else {\n> -\t\t/* Otherwise, prepare for using buffers from the only stream. */\n> -\t\tStream *stream = &data->streams_[0];\n> -\t\tret = video->importBuffers(stream->configuration().bufferCount);\n> +\t\tret = video->importBuffers(SIMPLE_QUEUE_DEPTH);\n\nHere we can overallocate too.\n\n> \t}\n> \tif (ret < 0)\n> \t\treturn ret;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index a148c35f1265..94e6fd9d2d56 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -148,8 +148,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\tcfg.bufferCount = 4;\n> -\n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> \tformat.size = cfg.size;\n> @@ -191,7 +189,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> \n> \tcfg.pixelFormat = formats.pixelformats().front();\n> \tcfg.size = formats.sizes(cfg.pixelFormat).back();\n> -\tcfg.bufferCount = 4;\n> \n> \tconfig->addConfiguration(cfg);\n> \n> @@ -236,7 +233,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,\n> int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> -\tunsigned int count = data->stream_.configuration().bufferCount;\n> +\tunsigned int count = data->properties_.get(properties::QueueDepth);\n> \n> \tint ret = data->video_->importBuffers(count);\n\nSame here, overallocation is best.\n\n> \tif (ret < 0)\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 22d6fdcbb141..891571afb3e5 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -165,8 +165,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\tcfg.bufferCount = 4;\n> -\n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> \tformat.size = cfg.size;\n> @@ -222,7 +220,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> \n> \tcfg.pixelFormat = formats::BGR888;\n> \tcfg.size = { 1920, 1080 };\n> -\tcfg.bufferCount = 4;\n> \n> \tconfig->addConfiguration(cfg);\n> \n> @@ -312,7 +309,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,\n> int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> -\tunsigned int count = data->stream_.configuration().bufferCount;\n> +\tunsigned int count = data->properties_.get(properties::QueueDepth);\n> \n> \tint ret = data->video_->importBuffers(count);\n\nI think you know by now what I would say :-)\n\n> \tif (ret < 0)\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index f7bafcf8fc97..be57abce4eb3 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const\n> * handlers provide StreamFormats.\n> */\n> StreamConfiguration::StreamConfiguration()\n> -\t: pixelFormat(0), stride(0), frameSize(0), bufferCount(0),\n> +\t: pixelFormat(0), stride(0), frameSize(0),\n> \t stream_(nullptr)\n> {\n> }\n> @@ -288,7 +288,7 @@ StreamConfiguration::StreamConfiguration()\n> * \\brief Construct a configuration with stream formats\n> */\n> StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> -\t: pixelFormat(0), stride(0), frameSize(0), bufferCount(0),\n> +\t: pixelFormat(0), stride(0), frameSize(0),\n> \t stream_(nullptr), formats_(formats)\n> {\n> }\n> @@ -323,11 +323,6 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> * validating the configuration with a call to CameraConfiguration::validate().\n> */\n> \n> -/**\n> - * \\var StreamConfiguration::bufferCount\n> - * \\brief Requested number of buffers to allocate for the stream\n> - */\n> -\n> /**\n> * \\fn StreamConfiguration::stream()\n> * \\brief Retrieve the stream associated with the configuration\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 53d97f3e6b86..79bf7aec7782 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> \n> using namespace libcamera;\n> @@ -107,14 +109,12 @@ void V4L2Camera::requestComplete(Request *request)\n> }\n> \n> int V4L2Camera::configure(StreamConfiguration *streamConfigOut,\n> -\t\t\t const Size &size, const PixelFormat &pixelformat,\n> -\t\t\t unsigned int bufferCount)\n> +\t\t\t const Size &size, const PixelFormat &pixelformat)\n> {\n> \tStreamConfiguration &streamConfig = config_->at(0);\n> \tstreamConfig.size.width = size.width;\n> \tstreamConfig.size.height = size.height;\n> \tstreamConfig.pixelFormat = pixelformat;\n> -\tstreamConfig.bufferCount = bufferCount;\n> \t/* \\todo memoryType (interval vs external) */\n> \n> \tCameraConfiguration::Status validation = config_->validate();\n> @@ -146,7 +146,6 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,\n> \tStreamConfiguration &cfg = config->at(0);\n> \tcfg.size = size;\n> \tcfg.pixelFormat = pixelFormat;\n> -\tcfg.bufferCount = 1;\n> \n> \tCameraConfiguration::Status validation = config->validate();\n> \tif (validation == CameraConfiguration::Invalid)\n> @@ -299,3 +298,10 @@ bool V4L2Camera::isRunning()\n> {\n> \treturn isRunning_;\n> }\n> +\n> +int V4L2Camera::getBufCount(int count)\n> +{\n> +\tint min = camera_->properties().get(properties::QueueDepth);\n> +\n> +\treturn std::max(count, min);\n> +}\n\nI'd name this function queueDepth() (we don't usually use a \"get\" prefix\nfor getters) and return the value of the property only. The caller can\nthen decide what to do with it.\n\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index d238046250e3..68df8ad05917 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -45,8 +45,7 @@ public:\n> \tstd::vector completedBuffers();\n> \n> \tint configure(StreamConfiguration *streamConfigOut,\n> -\t\t const Size &size, const PixelFormat &pixelformat,\n> -\t\t unsigned int bufferCount);\n> +\t\t const Size &size, const PixelFormat &pixelformat);\n> \tint validateConfiguration(const PixelFormat &pixelformat,\n> \t\t\t\t const Size &size,\n> \t\t\t\t StreamConfiguration *streamConfigOut);\n> @@ -65,6 +64,8 @@ public:\n> \n> \tbool isRunning();\n> \n> +\tint getBufCount(int count);\n> +\n> private:\n> \tvoid requestComplete(Request *request);\n> \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index f8bfe595e90e..cd32e44a01ad 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -348,8 +348,7 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)\n> \tSize size(arg->fmt.pix.width, arg->fmt.pix.height);\n> \tV4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);\n> \tret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t PixelFormatInfo::info(v4l2Format).format,\n> -\t\t\t bufferCount_);\n> +\t\t\t PixelFormatInfo::info(v4l2Format).format);\n> \tif (ret < 0)\n> \t\treturn -EINVAL;\n> \n> @@ -490,14 +489,13 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf\n> \tSize size(v4l2PixFormat_.width, v4l2PixFormat_.height);\n> \tV4L2PixelFormat v4l2Format = V4L2PixelFormat(v4l2PixFormat_.pixelformat);\n> \tint ret = vcam_->configure(&streamConfig_, size,\n> -\t\t\t\t PixelFormatInfo::info(v4l2Format).format,\n> -\t\t\t\t arg->count);\n> +\t\t\t\t PixelFormatInfo::info(v4l2Format).format);\n> \tif (ret < 0)\n> \t\treturn -EINVAL;\n> \n> \tsetFmtFromConfig(streamConfig_);\n> \n> -\targ->count = streamConfig_.bufferCount;\n> +\targ->count = vcam_->getBufCount(arg->count);\n> \tbufferCount_ = arg->count;\n> \n> \tret = vcam_->allocBuffers(arg->count);\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 61f4eb92ae95..f2c38bfb0b72 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -12,6 +12,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/event_dispatcher.h\"\n> #include \"libcamera/internal/media_device.h\"\n> @@ -91,10 +93,12 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\tunsigned int bufCount = camera_->properties().get(properties::QueueDepth);\n\nLet's name the variable bufferCount or numBuffers for consistency.\n\nThere are a few issues to solve, but this is a really nice change !\n\n> +\n> \t\tStream *stream = cfg.stream();\n> \n> \t\tBufferSource source;\n> -\t\tint ret = source.allocate(cfg);\n> +\t\tint ret = source.allocate(cfg, bufCount);\n> \t\tif (ret != TestPass)\n> \t\t\treturn ret;\n> \n> @@ -138,10 +142,10 @@ protected:\n> \t\twhile (timer.isRunning())\n> \t\t\tdispatcher->processEvents();\n> \n> -\t\tif (completeRequestsCount_ < cfg.bufferCount * 2) {\n> +\t\tif (completeRequestsCount_ < bufCount * 2) {\n> \t\t\tstd::cout << \"Failed to capture enough frames (got \"\n> \t\t\t\t << completeRequestsCount_ << \" expected at least \"\n> -\t\t\t\t << cfg.bufferCount * 2 << \")\" << std::endl;\n> +\t\t\t\t << bufCount * 2 << \")\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp\n> index 73563f2fc39d..c3d5286a2462 100644\n> --- a/test/libtest/buffer_source.cpp\n> +++ b/test/libtest/buffer_source.cpp\n> @@ -24,7 +24,7 @@ BufferSource::~BufferSource()\n> \t\tmedia_->release();\n> }\n> \n> -int BufferSource::allocate(const StreamConfiguration &config)\n> +int BufferSource::allocate(const StreamConfiguration &config, unsigned int count)\n> {\n> \t/* Locate and open the video device. */\n> \tstd::string videoDeviceName = \"vivid-000-vid-out\";\n> @@ -77,7 +77,7 @@ int BufferSource::allocate(const StreamConfiguration &config)\n> \t\treturn TestFail;\n> \t}\n> \n> -\tif (video->allocateBuffers(config.bufferCount, &buffers_) < 0) {\n> +\tif (video->allocateBuffers(count, &buffers_) < 0) {\n> \t\tstd::cout << \"Failed to allocate buffers\" << std::endl;\n> \t\treturn TestFail;\n> \t}\n> diff --git a/test/libtest/buffer_source.h b/test/libtest/buffer_source.h\n> index 14b4770e8d8a..6a18e269a575 100644\n> --- a/test/libtest/buffer_source.h\n> +++ b/test/libtest/buffer_source.h\n> @@ -20,7 +20,7 @@ public:\n> \tBufferSource();\n> \t~BufferSource();\n> \n> -\tint allocate(const StreamConfiguration &config);\n> +\tint allocate(const StreamConfiguration &config, unsigned int count);\n> \tconst std::vector> &buffers();\n> \n> private:\n> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> index b3f2bec11783..07fddfd2617c 100644\n> --- a/test/v4l2_videodevice/buffer_cache.cpp\n> +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \"buffer_source.h\"\n> @@ -145,10 +146,9 @@ public:\n> \t\tStreamConfiguration cfg;\n> \t\tcfg.pixelFormat = formats::YUYV;\n> \t\tcfg.size = Size(600, 800);\n> -\t\tcfg.bufferCount = numBuffers;\n> \n> \t\tBufferSource source;\n> -\t\tint ret = source.allocate(cfg);\n> +\t\tint ret = source.allocate(cfg, numBuffers);\n> \t\tif (ret != TestPass)\n> \t\t\treturn ret;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5C729BDC92\n\tfor ;\n\tMon, 26 Apr 2021 03:20:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A972C68880;\n\tMon, 26 Apr 2021 05:20:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E548605BD\n\tfor ;\n\tMon, 26 Apr 2021 05:20:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B61574FB;\n\tMon, 26 Apr 2021 05:20:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iKphRL6X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619407203;\n\tbh=25HLEyHzxD9ASos7Pb4DbmqxDXq6DJKSypJP3MpZJEE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iKphRL6XN0I8YrecgIor+s25AzvcDfEYfBNT45op7wj+lJB0Q6yhItjVYWScyPDXE\n\tWpanKSG8N/Ny8Tiv4aIRH9BM05VJ/+yWMCJ1SOC+8dqF/2o9BWvYqC2yBoHNrhZgYT\n\tGAhBljICI0WiVPo+BBplp7t5r/OPtmtdCWb9LBpM=","Date":"Mon, 26 Apr 2021 06:19:57 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421165139.318432-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?b?QW5kcsOp?=\n\tAlmeida , kernel@collabora.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16579,"web_url":"https://patchwork.libcamera.org/comment/16579/","msgid":"","date":"2021-04-26T09:28:55","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello,\n\nOn 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:\n\n\n\n> > @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera \n> > *camera)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tImgUDevice *imgu = data->imgu_;\n> > -\tunsigned int bufferCount;\n> > +\tunsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n> > \tint ret;\n> > \n> > -\tbufferCount = std::max({\n> > -\t\tdata->outStream_.configuration().bufferCount,\n> > -\t\tdata->vfStream_.configuration().bufferCount,\n> > -\t\tdata->rawStream_.configuration().bufferCount,\n> > -\t});\n> \n> I'm not sure properties::QueueDepth is the right value here. This deals\n> with both allocation of stats and params buffers, which are internal,\n> and the number of V4L2 buffer slots for the ImgU input and output. For\n> the latter, we probably should overallocate, as underallocation would\n> mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n> much to avoid wasting memory, so we should take into account how long\n> the IPA would need to hold on the params and stats buffers.\n> \n> Jacopo, Jean-Michel, Niklas, any comment ?\n\nI agree that QueueDepth is likely not the best fit here, but neither was \nbufferCount :-) IIRC this was done very early on where bufferCount was \nnot really defined and added as a first steeping stone. I'm very happy \nto see this being thought about.\n\nMaybe the best solution for this series is to add some const numerical \nvalues to the IPU3 pipeline handler to replace the usage of bufferCount \nbut not making it depending on QueueDepth?\n\nMaybe even separating it in two values as Laurent outlines, one for the \noutput and viewfinder slots allocation and one for the raw stream? Maybe \nit make sens to link the raw streams count with the value used for the \nstats and param buffer counts?","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BD9E1BDC9B\n\tfor ;\n\tMon, 26 Apr 2021 09:28:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46FC6688B9;\n\tMon, 26 Apr 2021 11:28:59 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D14D605BE\n\tfor ;\n\tMon, 26 Apr 2021 11:28:57 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id l22so55895942ljc.9\n\tfor ;\n\tMon, 26 Apr 2021 02:28:57 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv17sm1354631lfr.35.2021.04.26.02.28.55\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 26 Apr 2021 02:28:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"UHsvKisd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=a1usc6DBqwK9Bq3aGJs5FP6FkR/JzRXqs7kygRiVr7o=;\n\tb=UHsvKisdyuMg4Wkr8bdCCBg157uPJgP2AcxW8OY04xlNBKNtffjjHvfGMWsxOvKDof\n\tEMJkrDfmf37kvCn4u0facDNNkhvaI9UdSrfzUjmjInhJq4iKsZIPc2wevzrMV2WGcrOj\n\tHQcPRBg/tTHhb8uEJ8gUqFH9gUlu5C9zBCwzgYf75UOIlgayxoNSyMH1hS9yw1dwOoKp\n\tJhX1XsGmUzwy9UJdHxipHg0hWyg37NUbyhyjS70gBbhHHUgXqsacqq5wGOzhEePAyIT2\n\tj4twIogLwlaNJ2CIDgq6t5rFLvJU5Vb1CoWHjs2yaq552oUZm4pfmYtzVXnRvHr+EYmt\n\tzGXA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=a1usc6DBqwK9Bq3aGJs5FP6FkR/JzRXqs7kygRiVr7o=;\n\tb=hL4FGxWpBGuDX6SngJmp+hpiVSoUDDRz/TllnUeLHoUH9ZILFvMYiqLqu5SnYPoCR8\n\tZ2dwPwgryUDKEQSuv1eacJX0PerTmi4cpq2zAOizo0I4SmnsTkp+9fz1kuqHyLD0U5ih\n\tT6AFIVi32hBi6OasX442oyKfz5dYn6QJnDpV89Uwt9q1s8VjvshfNteXB9j33n5MKaGU\n\tr1W632P8cAPNp40IVmVDLF3Rsl5VEkqLQTVCNQsniAqdH0ChYVK27iy7h95Gp6Hf+9pG\n\ta4nq3JtqkcQdtflN6wnDgnBQWuh9iN8QwQjMHPszzOgav9wVGPWTZ43KWtr3DmHOAFmJ\n\tUNYQ==","X-Gm-Message-State":"AOAM531GwVn2DEt3RlBea8RZ9XiKCpPhYaH9NriiTi3RxvOW92cKAuYj\n\tp8UTGYNzxQGJUmbryJZLve02nw==","X-Google-Smtp-Source":"ABdhPJxqE+fRWNOX6AWlfDVQDOIp5+8WN+monSfPsMIf1r7ONFCDW6dfy8TjAy0lxBEcxdnETTZDUg==","X-Received":"by 2002:a2e:8891:: with SMTP id\n\tk17mr12368542lji.11.1619429337064; \n\tMon, 26 Apr 2021 02:28:57 -0700 (PDT)","Date":"Mon, 26 Apr 2021 11:28:55 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, =?iso-8859-1?q?Andr=E9?=\n\tAlmeida , kernel@collabora.com","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16581,"web_url":"https://patchwork.libcamera.org/comment/16581/","msgid":"<411e4814-85cb-f8a9-685b-c34e76c9ebd4@ideasonboard.com>","date":"2021-04-26T10:17:33","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi,\n\nOn 26/04/2021 11:28, Niklas Söderlund wrote:\n> Hello,\n> \n> On 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:\n> \n> \n> \n>>> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera \n>>> *camera)\n>>> {\n>>> \tIPU3CameraData *data = cameraData(camera);\n>>> \tImgUDevice *imgu = data->imgu_;\n>>> -\tunsigned int bufferCount;\n>>> +\tunsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n>>> \tint ret;\n>>> \n>>> -\tbufferCount = std::max({\n>>> -\t\tdata->outStream_.configuration().bufferCount,\n>>> -\t\tdata->vfStream_.configuration().bufferCount,\n>>> -\t\tdata->rawStream_.configuration().bufferCount,\n>>> -\t});\n>>\n>> I'm not sure properties::QueueDepth is the right value here. This deals\n>> with both allocation of stats and params buffers, which are internal,\n>> and the number of V4L2 buffer slots for the ImgU input and output. For\n>> the latter, we probably should overallocate, as underallocation would\n>> mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n>> much to avoid wasting memory, so we should take into account how long\n>> the IPA would need to hold on the params and stats buffers.\n>>\n>> Jacopo, Jean-Michel, Niklas, any comment ?\n> \n> I agree that QueueDepth is likely not the best fit here, but neither was \n> bufferCount :-) IIRC this was done very early on where bufferCount was \n> not really defined and added as a first steeping stone. I'm very happy \n> to see this being thought about.\n> \n> Maybe the best solution for this series is to add some const numerical \n> values to the IPU3 pipeline handler to replace the usage of bufferCount \n> but not making it depending on QueueDepth?\n> \n> Maybe even separating it in two values as Laurent outlines, one for the \n> output and viewfinder slots allocation and one for the raw stream? Maybe \n> it make sens to link the raw streams count with the value used for the \n> stats and param buffer counts?\n\nIndeed IPA may need some buffers, and even now it is not satisfying :-).\nOverallocating is a need, and we should not go lower than the highest\nnumber of frame delay in the delayedControls list. For IPU3, EXPOSURE\nhas a delay of 2, so we should not have less than 3 buffers to be\n\"comfortable\".\nMy 2 cents :-).","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C63B0BDC9C\n\tfor ;\n\tMon, 26 Apr 2021 10:17:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1707F688AF;\n\tMon, 26 Apr 2021 12:17:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A07FA605BE\n\tfor ;\n\tMon, 26 Apr 2021 12:17:34 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:bbde:270e:352b:91bf] (unknown\n\t[IPv6:2a01:e0a:169:7140:bbde:270e:352b:91bf])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CDC14FB;\n\tMon, 26 Apr 2021 12:17:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Bu49DV4z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619432254;\n\tbh=BPf3ZBh/6+z8sQ3k34MdxOCpEZvOqVGuwbtU/F0WrfA=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=Bu49DV4zRYzANOLitGhdGyJCU3POpL6mKb1DRusOJxe0yljDhHNwJvPSdoNkS7+Ul\n\t+Vuj8w9RpCKXOKsLaGgQJJoB98j31atEzb94/iqlNhE0GcLnAd4SLYpMy9a9N9CLb9\n\tCpRnpxFZJNBjW5e5lHt23Y3cY669jloyghIMcaas=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tLaurent Pinchart ","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>\n\t\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<411e4814-85cb-f8a9-685b-c34e76c9ebd4@ideasonboard.com>","Date":"Mon, 26 Apr 2021 12:17:33 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16585,"web_url":"https://patchwork.libcamera.org/comment/16585/","msgid":"","date":"2021-04-26T10:46:33","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nOn Mon, 26 Apr 2021 at 11:17, Jean-Michel Hautbois\n wrote:\n>\n> Hi,\n>\n> On 26/04/2021 11:28, Niklas Söderlund wrote:\n> > Hello,\n> >\n> > On 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:\n> >\n> > \n> >\n> >>> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera\n> >>> *camera)\n> >>> {\n> >>> IPU3CameraData *data = cameraData(camera);\n> >>> ImgUDevice *imgu = data->imgu_;\n> >>> - unsigned int bufferCount;\n> >>> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n> >>> int ret;\n> >>>\n> >>> - bufferCount = std::max({\n> >>> - data->outStream_.configuration().bufferCount,\n> >>> - data->vfStream_.configuration().bufferCount,\n> >>> - data->rawStream_.configuration().bufferCount,\n> >>> - });\n> >>\n> >> I'm not sure properties::QueueDepth is the right value here. This deals\n> >> with both allocation of stats and params buffers, which are internal,\n> >> and the number of V4L2 buffer slots for the ImgU input and output. For\n> >> the latter, we probably should overallocate, as underallocation would\n> >> mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n> >> much to avoid wasting memory, so we should take into account how long\n> >> the IPA would need to hold on the params and stats buffers.\n> >>\n> >> Jacopo, Jean-Michel, Niklas, any comment ?\n> >\n> > I agree that QueueDepth is likely not the best fit here, but neither was\n> > bufferCount :-) IIRC this was done very early on where bufferCount was\n> > not really defined and added as a first steeping stone. I'm very happy\n> > to see this being thought about.\n> >\n> > Maybe the best solution for this series is to add some const numerical\n> > values to the IPU3 pipeline handler to replace the usage of bufferCount\n> > but not making it depending on QueueDepth?\n> >\n> > Maybe even separating it in two values as Laurent outlines, one for the\n> > output and viewfinder slots allocation and one for the raw stream? Maybe\n> > it make sens to link the raw streams count with the value used for the\n> > stats and param buffer counts?\n>\n> Indeed IPA may need some buffers, and even now it is not satisfying :-).\n> Overallocating is a need, and we should not go lower than the highest\n> number of frame delay in the delayedControls list. For IPU3, EXPOSURE\n> has a delay of 2, so we should not have less than 3 buffers to be\n> \"comfortable\".\n> My 2 cents :-).\n\nI've been following this discussion and there seem to be a lot of\nqueues and delays flying around! Maybe someone can help me to\nunderstand - at the moment I believe there is:\n\n* A notion of how many buffers need to be allocated for each\nstream.The more you have, the more resilient processing tends to be to\nlatencies and delays.\n\n* This number can be different for different streams. You could\nimagine an application requiring fewer raw stream buffers, for\nexample.\n\n* There's a minimum number of buffers that are required for the\npipeline to work. For the Raspberry Pi this is 1.\n\n* I don't understand how delayed controls are related to this. They're\npurely to cope with the fact that the sensor has delays before\nsettings that have been applied take effect, and that this delay may\nvary between controls. They also vary with the sensor, they're not\nfixed for a particular pipeline handler.\n\nBest regards\nDavid\n\n>\n> --\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 77CF4BDC9A\n\tfor ;\n\tMon, 26 Apr 2021 10:46:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E641C6885A;\n\tMon, 26 Apr 2021 12:46:46 +0200 (CEST)","from mail-wr1-x433.google.com (mail-wr1-x433.google.com\n\t[IPv6:2a00:1450:4864:20::433])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46318605BE\n\tfor ;\n\tMon, 26 Apr 2021 12:46:45 +0200 (CEST)","by mail-wr1-x433.google.com with SMTP id h4so46308678wrt.12\n\tfor ;\n\tMon, 26 Apr 2021 03:46:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"gK78oqFj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=CvtAca38bTthI0e7CTDdGe7XVIWr+1HLFRjSqgwXOFA=;\n\tb=gK78oqFjmnilFgP53uHFKoCxgVbZ67vsG1He3Ey4WoebetrM9X4+izJhlqgm4Eh9R8\n\trZBbp53oZRAapY4VrEsdHyzN6fcTNIKrPVtvbbrKKLzPHAIdLz0Ov8kVeTtsOGvgbAp3\n\tiRXNv47hWJ9QnlTMZ7ZhtaKxtLS4YxXRoRk5IcHl3OlL3ndYBEmzPotphZoQCwQxPSLi\n\t/1t4iPGWztTDkBYGX3mAObXiq9+LeS2CkuzPo/Lrx+gojH3fnRndzNs176cqKXtdwc1W\n\trFmcH3k/6pRr5mCsXmkzropMkfD2PWBR5dM3DFY9n6iRiNrj5OZZuzUOnspKv6BC+ooK\n\tbRZQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=CvtAca38bTthI0e7CTDdGe7XVIWr+1HLFRjSqgwXOFA=;\n\tb=Y7HhjI1RiG0ZFfZjPcZ/j44TgmjE4/L5iBAE8Kost8rV/pG6E0C89ZlwHwlVVGX5+e\n\tExPIiih0WeOOmkmwPL8oqohP70h3mYFIu7+pPC/irIuLfIXQ1y6z3cfO0iL8RuXufUlH\n\tAZqVMvn3dU7xp4IPbWqeOiIiiqmsw1bY3uaV0LDjS9fqG6Tn/wcmfWhnuRRcavpM+hM1\n\tPzrK73yiRsgzzNw3cmwc701Bz5F8KJ1++3iPQtG8bgKpbo6kqbRzdVWcEVIq5IXKbOQl\n\tz/yBMQzkdDAf6zcMG/DiuI3kIglYDKZqMAGX58rGlTqq5qBxLe/TXifR/SimZGwjSl/h\n\tTcZw==","X-Gm-Message-State":"AOAM5327tRSUzEdUiFWy5SV9Ka5ihU6ZuVFIEDKGcJz1YaZSu5tuJAqq\n\t7BM0e5FAWHevO6rzseg7+HA38hZfqnYgdoUBt/Kqa46P9m0=","X-Google-Smtp-Source":"ABdhPJzDOjWvTaZs2+wnIE5RwsUGiMGD2XSam5BBxhCFr6g/CRXoSB5zSXSC15zTqua11fP3sTPEdGWwK4jgjDVqKaI=","X-Received":"by 2002:adf:f186:: with SMTP id h6mr21510931wro.89.1619434004982;\n\tMon, 26 Apr 2021 03:46:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>\n\t\n\t\n\t<411e4814-85cb-f8a9-685b-c34e76c9ebd4@ideasonboard.com>","In-Reply-To":"<411e4814-85cb-f8a9-685b-c34e76c9ebd4@ideasonboard.com>","From":"David Plowman ","Date":"Mon, 26 Apr 2021 11:46:33 +0100","Message-ID":"","To":"Jean-Michel Hautbois ","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ,\n\tkernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16586,"web_url":"https://patchwork.libcamera.org/comment/16586/","msgid":"<4b9f4c36-f8e6-7fa7-42c9-0a852f7da9cb@ideasonboard.com>","date":"2021-04-26T10:52:46","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi David,\n\nOn 26/04/2021 12:46, David Plowman wrote:\n> Hi everyone\n> \n> On Mon, 26 Apr 2021 at 11:17, Jean-Michel Hautbois\n> wrote:\n>>\n>> Hi,\n>>\n>> On 26/04/2021 11:28, Niklas Söderlund wrote:\n>>> Hello,\n>>>\n>>> On 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:\n>>>\n>>> \n>>>\n>>>>> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera\n>>>>> *camera)\n>>>>> {\n>>>>> IPU3CameraData *data = cameraData(camera);\n>>>>> ImgUDevice *imgu = data->imgu_;\n>>>>> - unsigned int bufferCount;\n>>>>> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n>>>>> int ret;\n>>>>>\n>>>>> - bufferCount = std::max({\n>>>>> - data->outStream_.configuration().bufferCount,\n>>>>> - data->vfStream_.configuration().bufferCount,\n>>>>> - data->rawStream_.configuration().bufferCount,\n>>>>> - });\n>>>>\n>>>> I'm not sure properties::QueueDepth is the right value here. This deals\n>>>> with both allocation of stats and params buffers, which are internal,\n>>>> and the number of V4L2 buffer slots for the ImgU input and output. For\n>>>> the latter, we probably should overallocate, as underallocation would\n>>>> mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n>>>> much to avoid wasting memory, so we should take into account how long\n>>>> the IPA would need to hold on the params and stats buffers.\n>>>>\n>>>> Jacopo, Jean-Michel, Niklas, any comment ?\n>>>\n>>> I agree that QueueDepth is likely not the best fit here, but neither was\n>>> bufferCount :-) IIRC this was done very early on where bufferCount was\n>>> not really defined and added as a first steeping stone. I'm very happy\n>>> to see this being thought about.\n>>>\n>>> Maybe the best solution for this series is to add some const numerical\n>>> values to the IPU3 pipeline handler to replace the usage of bufferCount\n>>> but not making it depending on QueueDepth?\n>>>\n>>> Maybe even separating it in two values as Laurent outlines, one for the\n>>> output and viewfinder slots allocation and one for the raw stream? Maybe\n>>> it make sens to link the raw streams count with the value used for the\n>>> stats and param buffer counts?\n>>\n>> Indeed IPA may need some buffers, and even now it is not satisfying :-).\n>> Overallocating is a need, and we should not go lower than the highest\n>> number of frame delay in the delayedControls list. For IPU3, EXPOSURE\n>> has a delay of 2, so we should not have less than 3 buffers to be\n>> \"comfortable\".\n>> My 2 cents :-).\n> \n> I've been following this discussion and there seem to be a lot of\n> queues and delays flying around! Maybe someone can help me to\n> understand - at the moment I believe there is:\n> \n> * A notion of how many buffers need to be allocated for each\n> stream.The more you have, the more resilient processing tends to be to\n> latencies and delays.\n> \n> * This number can be different for different streams. You could\n> imagine an application requiring fewer raw stream buffers, for\n> example.\n> \n> * There's a minimum number of buffers that are required for the\n> pipeline to work. For the Raspberry Pi this is 1.\n> \n> * I don't understand how delayed controls are related to this. They're\n> purely to cope with the fact that the sensor has delays before\n> settings that have been applied take effect, and that this delay may\n> vary between controls. They also vary with the sensor, they're not\n> fixed for a particular pipeline handler.\n\nTrue, I read too fast sorry for having disturbed you :-/.\nThere is no relation with delayedControls.\n\nJM","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4D6F4BDC9C\n\tfor ;\n\tMon, 26 Apr 2021 10:52:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAA9E68882;\n\tMon, 26 Apr 2021 12:52:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C7C1605BE\n\tfor ;\n\tMon, 26 Apr 2021 12:52:47 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:bbde:270e:352b:91bf] (unknown\n\t[IPv6:2a01:e0a:169:7140:bbde:270e:352b:91bf])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 164904FB;\n\tMon, 26 Apr 2021 12:52:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dsu1CNX6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619434367;\n\tbh=fuXoaciRsR+kxMj7JlUMm8mNfHLwtkapDcPhmrMT+34=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dsu1CNX6r8kxy4vs1ouQG3X0Z0chdN2GdnrplyxPS0tltebU8YkVxKDpUNwCO7MwB\n\tU43iqDd5XvPHMEV8CLKPjE4ZugvH37QUugI0zFZ2I8Q+uq1bPzHy8aG9qvshdueipE\n\tihoRXdx0XPg1Cf47cz6gKdz9oaHsZoGYAZ8LoI5Y=","To":"David Plowman ,\n\tJean-Michel Hautbois ","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>\n\t\n\t\n\t<411e4814-85cb-f8a9-685b-c34e76c9ebd4@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<4b9f4c36-f8e6-7fa7-42c9-0a852f7da9cb@ideasonboard.com>","Date":"Mon, 26 Apr 2021 12:52:46 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ,\n\tkernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16590,"web_url":"https://patchwork.libcamera.org/comment/16590/","msgid":"","date":"2021-04-26T11:56:35","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent, and Nicolas,\n\nOn Mon, 26 Apr 2021 at 04:20, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Nícolas,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 21, 2021 at 01:51:39PM -0300, Nícolas F. R. A. Prado wrote:\n> > Now that the amount of internal buffers allocated is hardcoded by the\n> > pipelines, and the amount of buffers allocated by the\n> > FrameBufferAllocator helper is passed through\n> > FrameBufferAllocator::allocate(), we no longer need to have bufferCount\n> > in the StreamConfiguration, so remove it.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > include/libcamera/stream.h | 2 --\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 1 -\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +--------------\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++-------\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 --\n> > src/libcamera/pipeline/simple/converter.cpp | 7 ++-----\n> > src/libcamera/pipeline/simple/converter.h | 5 ++---\n> > src/libcamera/pipeline/simple/simple.cpp | 15 ++++-----------\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +----\n> > src/libcamera/pipeline/vimc/vimc.cpp | 5 +----\n> > src/libcamera/stream.cpp | 9 ++-------\n> > src/v4l2/v4l2_camera.cpp | 14 ++++++++++----\n> > src/v4l2/v4l2_camera.h | 5 +++--\n> > src/v4l2/v4l2_camera_proxy.cpp | 8 +++-----\n> > test/camera/buffer_import.cpp | 10 +++++++---\n> > test/libtest/buffer_source.cpp | 4 ++--\n> > test/libtest/buffer_source.h | 2 +-\n> > test/v4l2_videodevice/buffer_cache.cpp | 4 ++--\n>\n> An update to src/android/ is missing, which breaks compilation of the\n> HAL :-S\n>\n> > 19 files changed, 45 insertions(+), 86 deletions(-)\n> >\n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index bb47c390f8a1..f36aeffd9540 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -45,8 +45,6 @@ struct StreamConfiguration {\n> > unsigned int stride;\n> > unsigned int frameSize;\n> >\n> > - unsigned int bufferCount;\n> > -\n> > Stream *stream() const { return stream_; }\n> > void setStream(Stream *stream) { stream_ = stream; }\n> > const StreamFormats &formats() const { return formats_; }\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp\n> b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 3cd777d1b742..1e110fe0c189 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -213,7 +213,6 @@ StreamConfiguration\n> CIO2Device::generateConfiguration(Size size) const\n> >\n> > cfg.size = sensorFormat.size;\n> > cfg.pixelFormat =\n> mbusCodesToPixelFormat.at(sensorFormat.mbus_code);\n> > - cfg.bufferCount = CIO2_BUFFER_COUNT;\n> >\n> > return cfg;\n> > }\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c8fcc2fda75f..f0a17a553bd3 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -291,7 +291,6 @@ CameraConfiguration::Status\n> IPU3CameraConfiguration::validate()\n> > /* Initialize the RAW stream with the CIO2\n> configuration. */\n> > cfg->size = cio2Configuration_.size;\n> > cfg->pixelFormat = cio2Configuration_.pixelFormat;\n> > - cfg->bufferCount = cio2Configuration_.bufferCount;\n> > cfg->stride = info.stride(cfg->size.width, 0, 64);\n> > cfg->frameSize = info.frameSize(cfg->size, 64);\n> > cfg->setStream(const_cast *>(&data_->rawStream_));\n> > @@ -335,7 +334,6 @@ CameraConfiguration::Status\n> IPU3CameraConfiguration::validate()\n> > IMGU_OUTPUT_HEIGHT_ALIGN);\n> >\n> > cfg->pixelFormat = formats::NV12;\n> > - cfg->bufferCount = IPU3_BUFFER_COUNT;\n> > cfg->stride = info.stride(cfg->size.width, 0, 1);\n> > cfg->frameSize = info.frameSize(cfg->size, 1);\n> >\n> > @@ -403,7 +401,6 @@ CameraConfiguration\n> *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > Size sensorResolution = data->cio2_.sensor()->resolution();\n> > for (const StreamRole role : roles) {\n> > std::map>\n> streamFormats;\n> > - unsigned int bufferCount;\n> > PixelFormat pixelFormat;\n> > Size size;\n> >\n> > @@ -424,7 +421,6 @@ CameraConfiguration\n> *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > size.height = utils::alignDown(size.height - 1,\n> >\n> IMGU_OUTPUT_HEIGHT_MARGIN);\n> > pixelFormat = formats::NV12;\n> > - bufferCount = IPU3_BUFFER_COUNT;\n> > streamFormats[pixelFormat] = { {\n> IMGU_OUTPUT_MIN_SIZE, size } };\n> >\n> > break;\n> > @@ -434,7 +430,6 @@ CameraConfiguration\n> *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >\n> data->cio2_.generateConfiguration(sensorResolution);\n> > pixelFormat = cio2Config.pixelFormat;\n> > size = cio2Config.size;\n> > - bufferCount = cio2Config.bufferCount;\n> >\n> > for (const PixelFormat &format :\n> data->cio2_.formats())\n> > streamFormats[format] =\n> data->cio2_.sizes();\n> > @@ -453,7 +448,6 @@ CameraConfiguration\n> *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >\n> .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> >\n> IMGU_OUTPUT_HEIGHT_ALIGN);\n> > pixelFormat = formats::NV12;\n> > - bufferCount = IPU3_BUFFER_COUNT;\n> > streamFormats[pixelFormat] = { {\n> IMGU_OUTPUT_MIN_SIZE, size } };\n> >\n> > break;\n> > @@ -470,7 +464,6 @@ CameraConfiguration\n> *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > StreamConfiguration cfg(formats);\n> > cfg.size = size;\n> > cfg.pixelFormat = pixelFormat;\n> > - cfg.bufferCount = bufferCount;\n> > config->addConfiguration(cfg);\n> > }\n> >\n> > @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera\n> *camera)\n> > {\n> > IPU3CameraData *data = cameraData(camera);\n> > ImgUDevice *imgu = data->imgu_;\n> > - unsigned int bufferCount;\n> > + unsigned int bufferCount =\n> data->properties_.get(properties::QueueDepth);\n> > int ret;\n> >\n> > - bufferCount = std::max({\n> > - data->outStream_.configuration().bufferCount,\n> > - data->vfStream_.configuration().bufferCount,\n> > - data->rawStream_.configuration().bufferCount,\n> > - });\n>\n> I'm not sure properties::QueueDepth is the right value here. This deals\n> with both allocation of stats and params buffers, which are internal,\n> and the number of V4L2 buffer slots for the ImgU input and output. For\n> the latter, we probably should overallocate, as underallocation would\n> mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n> much to avoid wasting memory, so we should take into account how long\n> the IPA would need to hold on the params and stats buffers.\n>\n> Jacopo, Jean-Michel, Niklas, any comment ?\n>\n> > -\n> > ret = imgu->allocateBuffers(bufferCount);\n> > if (ret < 0)\n> > return ret;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 3f35596fe550..44a8a472ae4f 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -471,7 +471,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > RPiCameraData *data = cameraData(camera);\n> > CameraConfiguration *config = new RPiCameraConfiguration(data);\n> > V4L2DeviceFormat sensorFormat;\n> > - unsigned int bufferCount;\n> > PixelFormat pixelFormat;\n> > V4L2VideoDevice::Formats fmts;\n> > Size size;\n> > @@ -489,7 +488,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > sensorFormat = findBestMode(fmts, size);\n> > pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> > ASSERT(pixelFormat.isValid());\n> > - bufferCount = 2;\n> > rawCount++;\n> > break;\n> >\n> > @@ -498,7 +496,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > pixelFormat = formats::NV12;\n> > /* Return the largest sensor resolution. */\n> > size = data->sensor_->resolution();\n> > - bufferCount = 1;\n> > outCount++;\n> > break;\n> >\n> > @@ -514,7 +511,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > fmts = data->isp_[Isp::Output0].dev()->formats();\n> > pixelFormat = formats::YUV420;\n> > size = { 1920, 1080 };\n> > - bufferCount = 4;\n> > outCount++;\n> > break;\n> >\n> > @@ -522,7 +518,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > fmts = data->isp_[Isp::Output0].dev()->formats();\n> > pixelFormat = formats::ARGB8888;\n> > size = { 800, 600 };\n> > - bufferCount = 4;\n> > outCount++;\n> > break;\n> >\n> > @@ -552,7 +547,6 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > StreamConfiguration cfg(formats);\n> > cfg.size = size;\n> > cfg.pixelFormat = pixelFormat;\n> > - cfg.bufferCount = bufferCount;\n> > config->addConfiguration(cfg);\n> > }\n> >\n> > @@ -1142,6 +1136,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > int ret;\n> > + unsigned int bufferCount =\n> data->properties_.get(properties::QueueDepth);\n> >\n> > /*\n> > * Decide how many internal buffers to allocate. For now, simply\n> look\n> > @@ -1152,7 +1147,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > unsigned int maxBuffers = 0;\n> > for (const Stream *s : camera->streams())\n> > if (static_cast(s)->isExternal())\n> > - maxBuffers = std::max(maxBuffers,\n> s->configuration().bufferCount);\n> > + maxBuffers = std::max(maxBuffers, bufferCount);\n>\n> That now looks a bit weird, as bufferCount is constant. This being said,\n> the above comment for the IPU3 applies here. David, Naush, any comment ?\n>\n\nSimilar to what others have mentioned, I don't think properties::QueueDepth\nshould be\nused over here. This bit of code may need reworking after these changes\nhave gone in,\nbut for now, a const value for bufferCount might be more suitable.\n\nNaush\n\n\n\n> >\n> > for (auto const stream : data->streams_) {\n> > ret = stream->prepareBuffers(maxBuffers);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 2d95c1ca2a43..73d4ea6ba8f5 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -686,16 +686,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera\n> *camera)\n> > unsigned int ipaBufferId = 1;\n> > int ret;\n> >\n> > - unsigned int maxCount = std::max({\n> > - data->mainPathStream_.configuration().bufferCount,\n> > - data->selfPathStream_.configuration().bufferCount,\n> > - });\n> > -\n> > - ret = param_->allocateBuffers(maxCount, ¶mBuffers_);\n> > + ret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers_);\n> > if (ret < 0)\n> > goto error;\n> >\n> > - ret = stat_->allocateBuffers(maxCount, &statBuffers_);\n> > + ret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers_);\n>\n> Here it should be fine.\n>\n> > if (ret < 0)\n> > goto error;\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 25f482eb8d8e..200e3c2c4cca 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -61,7 +61,6 @@ StreamConfiguration\n> RkISP1Path::generateConfiguration(const Size &resolution)\n> > StreamConfiguration cfg(formats);\n> > cfg.pixelFormat = formats::NV12;\n> > cfg.size = maxResolution;\n> > - cfg.bufferCount = RKISP1_BUFFER_COUNT;\n> >\n> > return cfg;\n> > }\n> > @@ -77,7 +76,6 @@ CameraConfiguration::Status\n> RkISP1Path::validate(StreamConfiguration *cfg)\n> >\n> > cfg->size.boundTo(maxResolution_);\n> > cfg->size.expandTo(minResolution_);\n> > - cfg->bufferCount = RKISP1_BUFFER_COUNT;\n> >\n> > V4L2DeviceFormat format;\n> > format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);\n> > diff --git a/src/libcamera/pipeline/simple/converter.cpp\n> b/src/libcamera/pipeline/simple/converter.cpp\n> > index 68644ef6477f..54e7f1b051f7 100644\n> > --- a/src/libcamera/pipeline/simple/converter.cpp\n> > +++ b/src/libcamera/pipeline/simple/converter.cpp\n> > @@ -86,9 +86,6 @@ int SimpleConverter::Stream::configure(const\n> StreamConfiguration &inputCfg,\n> > return -EINVAL;\n> > }\n> >\n> > - inputBufferCount_ = inputCfg.bufferCount;\n> > - outputBufferCount_ = outputCfg.bufferCount;\n> > -\n> > return 0;\n> > }\n> >\n> > @@ -100,11 +97,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned\n> int count,\n> >\n> > int SimpleConverter::Stream::start()\n> > {\n> > - int ret = m2m_->output()->importBuffers(inputBufferCount_);\n> > + int ret = m2m_->output()->importBuffers(SIMPLE_QUEUE_DEPTH);\n> > if (ret < 0)\n> > return ret;\n> >\n> > - ret = m2m_->capture()->importBuffers(outputBufferCount_);\n> > + ret = m2m_->capture()->importBuffers(SIMPLE_QUEUE_DEPTH);\n>\n> As above, we should probably overallocate here to avoid trashing the\n> dmabuf mappings.\n>\n> > if (ret < 0) {\n> > stop();\n> > return ret;\n> > diff --git a/src/libcamera/pipeline/simple/converter.h\n> b/src/libcamera/pipeline/simple/converter.h\n> > index 480e528d2210..32313748cd24 100644\n> > --- a/src/libcamera/pipeline/simple/converter.h\n> > +++ b/src/libcamera/pipeline/simple/converter.h\n> > @@ -29,6 +29,8 @@ class SizeRange;\n> > struct StreamConfiguration;\n> > class V4L2M2MDevice;\n> >\n> > +constexpr unsigned int SIMPLE_QUEUE_DEPTH = 3;\n> > +\n> > class SimpleConverter\n> > {\n> > public:\n> > @@ -84,9 +86,6 @@ private:\n> > SimpleConverter *converter_;\n> > unsigned int index_;\n> > std::unique_ptr m2m_;\n> > -\n> > - unsigned int inputBufferCount_;\n> > - unsigned int outputBufferCount_;\n> > };\n> >\n> > std::string deviceNode_;\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp\n> b/src/libcamera/pipeline/simple/simple.cpp\n> > index b9f14be6733f..9f28bb66e2e7 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -142,8 +142,6 @@ static const SimplePipelineInfo supportedDevices[] =\n> {\n> >\n> > } /* namespace */\n> >\n> > -static constexpr unsigned int kNumInternalBuffers = 3;\n> > -\n> > class SimpleCameraData : public CameraData\n> > {\n> > public:\n> > @@ -425,7 +423,7 @@ int SimpleCameraData::init()\n> > }\n> >\n> > properties_ = sensor_->properties();\n> > - properties_.set(properties::QueueDepth, kNumInternalBuffers);\n> > + properties_.set(properties::QueueDepth, SIMPLE_QUEUE_DEPTH);\n> >\n> > return 0;\n> > }\n> > @@ -616,7 +614,7 @@ CameraConfiguration::Status\n> SimpleCameraConfiguration::validate()\n> > cfg.size != pipeConfig_->captureSize)\n> > needConversion_ = true;\n> >\n> > - /* Set the stride, frameSize and bufferCount. */\n> > + /* Set the stride and frameSize. */\n> > if (needConversion_) {\n> > std::tie(cfg.stride, cfg.frameSize) =\n> >\n> converter->strideAndFrameSize(cfg.pixelFormat, cfg.size);\n> > @@ -634,8 +632,6 @@ CameraConfiguration::Status\n> SimpleCameraConfiguration::validate()\n> > cfg.stride = format.planes[0].bpl;\n> > cfg.frameSize = format.planes[0].size;\n> > }\n> > -\n> > - cfg.bufferCount = 3;\n> > }\n> >\n> > return status;\n> > @@ -758,7 +754,6 @@ int SimplePipelineHandler::configure(Camera *camera,\n> CameraConfiguration *c)\n> > inputCfg.pixelFormat = pipeConfig->captureFormat;\n> > inputCfg.size = pipeConfig->captureSize;\n> > inputCfg.stride = captureFormat.planes[0].bpl;\n> > - inputCfg.bufferCount = kNumInternalBuffers;\n> >\n> > return converter_->configure(inputCfg, outputCfgs);\n> > }\n> > @@ -791,12 +786,10 @@ int SimplePipelineHandler::start(Camera *camera,\n> [[maybe_unused]] const ControlL\n> > * When using the converter allocate a fixed number of\n> internal\n> > * buffers.\n> > */\n> > - ret = video->allocateBuffers(kNumInternalBuffers,\n> > + ret = video->allocateBuffers(SIMPLE_QUEUE_DEPTH,\n>\n> This should definitely not overallocate :-) 3 sounds good.\n>\n> > &data->converterBuffers_);\n> > } else {\n> > - /* Otherwise, prepare for using buffers from the only\n> stream. */\n> > - Stream *stream = &data->streams_[0];\n> > - ret =\n> video->importBuffers(stream->configuration().bufferCount);\n> > + ret = video->importBuffers(SIMPLE_QUEUE_DEPTH);\n>\n> Here we can overallocate too.\n>\n> > }\n> > if (ret < 0)\n> > return ret;\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index a148c35f1265..94e6fd9d2d56 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -148,8 +148,6 @@ CameraConfiguration::Status\n> UVCCameraConfiguration::validate()\n> > status = Adjusted;\n> > }\n> >\n> > - cfg.bufferCount = 4;\n> > -\n> > V4L2DeviceFormat format;\n> > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> > format.size = cfg.size;\n> > @@ -191,7 +189,6 @@ CameraConfiguration\n> *PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> >\n> > cfg.pixelFormat = formats.pixelformats().front();\n> > cfg.size = formats.sizes(cfg.pixelFormat).back();\n> > - cfg.bufferCount = 4;\n> >\n> > config->addConfiguration(cfg);\n> >\n> > @@ -236,7 +233,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera\n> *camera,\n> > int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const\n> ControlList *controls)\n> > {\n> > UVCCameraData *data = cameraData(camera);\n> > - unsigned int count = data->stream_.configuration().bufferCount;\n> > + unsigned int count = data->properties_.get(properties::QueueDepth);\n> >\n> > int ret = data->video_->importBuffers(count);\n>\n> Same here, overallocation is best.\n>\n> > if (ret < 0)\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp\n> b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 22d6fdcbb141..891571afb3e5 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -165,8 +165,6 @@ CameraConfiguration::Status\n> VimcCameraConfiguration::validate()\n> > status = Adjusted;\n> > }\n> >\n> > - cfg.bufferCount = 4;\n> > -\n> > V4L2DeviceFormat format;\n> > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> > format.size = cfg.size;\n> > @@ -222,7 +220,6 @@ CameraConfiguration\n> *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >\n> > cfg.pixelFormat = formats::BGR888;\n> > cfg.size = { 1920, 1080 };\n> > - cfg.bufferCount = 4;\n> >\n> > config->addConfiguration(cfg);\n> >\n> > @@ -312,7 +309,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera\n> *camera,\n> > int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const\n> ControlList *controls)\n> > {\n> > VimcCameraData *data = cameraData(camera);\n> > - unsigned int count = data->stream_.configuration().bufferCount;\n> > + unsigned int count = data->properties_.get(properties::QueueDepth);\n> >\n> > int ret = data->video_->importBuffers(count);\n>\n> I think you know by now what I would say :-)\n>\n> > if (ret < 0)\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index f7bafcf8fc97..be57abce4eb3 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(const PixelFormat\n> &pixelformat) const\n> > * handlers provide StreamFormats.\n> > */\n> > StreamConfiguration::StreamConfiguration()\n> > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),\n> > + : pixelFormat(0), stride(0), frameSize(0),\n> > stream_(nullptr)\n> > {\n> > }\n> > @@ -288,7 +288,7 @@ StreamConfiguration::StreamConfiguration()\n> > * \\brief Construct a configuration with stream formats\n> > */\n> > StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),\n> > + : pixelFormat(0), stride(0), frameSize(0),\n> > stream_(nullptr), formats_(formats)\n> > {\n> > }\n> > @@ -323,11 +323,6 @@ StreamConfiguration::StreamConfiguration(const\n> StreamFormats &formats)\n> > * validating the configuration with a call to\n> CameraConfiguration::validate().\n> > */\n> >\n> > -/**\n> > - * \\var StreamConfiguration::bufferCount\n> > - * \\brief Requested number of buffers to allocate for the stream\n> > - */\n> > -\n> > /**\n> > * \\fn StreamConfiguration::stream()\n> > * \\brief Retrieve the stream associated with the configuration\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 53d97f3e6b86..79bf7aec7782 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -10,6 +10,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> >\n> > using namespace libcamera;\n> > @@ -107,14 +109,12 @@ void V4L2Camera::requestComplete(Request *request)\n> > }\n> >\n> > int V4L2Camera::configure(StreamConfiguration *streamConfigOut,\n> > - const Size &size, const PixelFormat &pixelformat,\n> > - unsigned int bufferCount)\n> > + const Size &size, const PixelFormat &pixelformat)\n> > {\n> > StreamConfiguration &streamConfig = config_->at(0);\n> > streamConfig.size.width = size.width;\n> > streamConfig.size.height = size.height;\n> > streamConfig.pixelFormat = pixelformat;\n> > - streamConfig.bufferCount = bufferCount;\n> > /* \\todo memoryType (interval vs external) */\n> >\n> > CameraConfiguration::Status validation = config_->validate();\n> > @@ -146,7 +146,6 @@ int V4L2Camera::validateConfiguration(const\n> PixelFormat &pixelFormat,\n> > StreamConfiguration &cfg = config->at(0);\n> > cfg.size = size;\n> > cfg.pixelFormat = pixelFormat;\n> > - cfg.bufferCount = 1;\n> >\n> > CameraConfiguration::Status validation = config->validate();\n> > if (validation == CameraConfiguration::Invalid)\n> > @@ -299,3 +298,10 @@ bool V4L2Camera::isRunning()\n> > {\n> > return isRunning_;\n> > }\n> > +\n> > +int V4L2Camera::getBufCount(int count)\n> > +{\n> > + int min = camera_->properties().get(properties::QueueDepth);\n> > +\n> > + return std::max(count, min);\n> > +}\n>\n> I'd name this function queueDepth() (we don't usually use a \"get\" prefix\n> for getters) and return the value of the property only. The caller can\n> then decide what to do with it.\n>\n> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > index d238046250e3..68df8ad05917 100644\n> > --- a/src/v4l2/v4l2_camera.h\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -45,8 +45,7 @@ public:\n> > std::vector completedBuffers();\n> >\n> > int configure(StreamConfiguration *streamConfigOut,\n> > - const Size &size, const PixelFormat &pixelformat,\n> > - unsigned int bufferCount);\n> > + const Size &size, const PixelFormat &pixelformat);\n> > int validateConfiguration(const PixelFormat &pixelformat,\n> > const Size &size,\n> > StreamConfiguration *streamConfigOut);\n> > @@ -65,6 +64,8 @@ public:\n> >\n> > bool isRunning();\n> >\n> > + int getBufCount(int count);\n> > +\n> > private:\n> > void requestComplete(Request *request);\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp\n> b/src/v4l2/v4l2_camera_proxy.cpp\n> > index f8bfe595e90e..cd32e44a01ad 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -348,8 +348,7 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile\n> *file, struct v4l2_format *arg)\n> > Size size(arg->fmt.pix.width, arg->fmt.pix.height);\n> > V4L2PixelFormat v4l2Format =\n> V4L2PixelFormat(arg->fmt.pix.pixelformat);\n> > ret = vcam_->configure(&streamConfig_, size,\n> > - PixelFormatInfo::info(v4l2Format).format,\n> > - bufferCount_);\n> > + PixelFormatInfo::info(v4l2Format).format);\n> > if (ret < 0)\n> > return -EINVAL;\n> >\n> > @@ -490,14 +489,13 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile\n> *file, struct v4l2_requestbuf\n> > Size size(v4l2PixFormat_.width, v4l2PixFormat_.height);\n> > V4L2PixelFormat v4l2Format =\n> V4L2PixelFormat(v4l2PixFormat_.pixelformat);\n> > int ret = vcam_->configure(&streamConfig_, size,\n> > -\n> PixelFormatInfo::info(v4l2Format).format,\n> > - arg->count);\n> > +\n> PixelFormatInfo::info(v4l2Format).format);\n> > if (ret < 0)\n> > return -EINVAL;\n> >\n> > setFmtFromConfig(streamConfig_);\n> >\n> > - arg->count = streamConfig_.bufferCount;\n> > + arg->count = vcam_->getBufCount(arg->count);\n> > bufferCount_ = arg->count;\n> >\n> > ret = vcam_->allocBuffers(arg->count);\n> > diff --git a/test/camera/buffer_import.cpp\n> b/test/camera/buffer_import.cpp\n> > index 61f4eb92ae95..f2c38bfb0b72 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -12,6 +12,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > #include \"libcamera/internal/event_dispatcher.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > @@ -91,10 +93,12 @@ protected:\n> > return TestFail;\n> > }\n> >\n> > + unsigned int bufCount =\n> camera_->properties().get(properties::QueueDepth);\n>\n> Let's name the variable bufferCount or numBuffers for consistency.\n>\n> There are a few issues to solve, but this is a really nice change !\n>\n> > +\n> > Stream *stream = cfg.stream();\n> >\n> > BufferSource source;\n> > - int ret = source.allocate(cfg);\n> > + int ret = source.allocate(cfg, bufCount);\n> > if (ret != TestPass)\n> > return ret;\n> >\n> > @@ -138,10 +142,10 @@ protected:\n> > while (timer.isRunning())\n> > dispatcher->processEvents();\n> >\n> > - if (completeRequestsCount_ < cfg.bufferCount * 2) {\n> > + if (completeRequestsCount_ < bufCount * 2) {\n> > std::cout << \"Failed to capture enough frames (got\n> \"\n> > << completeRequestsCount_ << \" expected\n> at least \"\n> > - << cfg.bufferCount * 2 << \")\" <<\n> std::endl;\n> > + << bufCount * 2 << \")\" << std::endl;\n> > return TestFail;\n> > }\n> >\n> > diff --git a/test/libtest/buffer_source.cpp\n> b/test/libtest/buffer_source.cpp\n> > index 73563f2fc39d..c3d5286a2462 100644\n> > --- a/test/libtest/buffer_source.cpp\n> > +++ b/test/libtest/buffer_source.cpp\n> > @@ -24,7 +24,7 @@ BufferSource::~BufferSource()\n> > media_->release();\n> > }\n> >\n> > -int BufferSource::allocate(const StreamConfiguration &config)\n> > +int BufferSource::allocate(const StreamConfiguration &config, unsigned\n> int count)\n> > {\n> > /* Locate and open the video device. */\n> > std::string videoDeviceName = \"vivid-000-vid-out\";\n> > @@ -77,7 +77,7 @@ int BufferSource::allocate(const StreamConfiguration\n> &config)\n> > return TestFail;\n> > }\n> >\n> > - if (video->allocateBuffers(config.bufferCount, &buffers_) < 0) {\n> > + if (video->allocateBuffers(count, &buffers_) < 0) {\n> > std::cout << \"Failed to allocate buffers\" << std::endl;\n> > return TestFail;\n> > }\n> > diff --git a/test/libtest/buffer_source.h b/test/libtest/buffer_source.h\n> > index 14b4770e8d8a..6a18e269a575 100644\n> > --- a/test/libtest/buffer_source.h\n> > +++ b/test/libtest/buffer_source.h\n> > @@ -20,7 +20,7 @@ public:\n> > BufferSource();\n> > ~BufferSource();\n> >\n> > - int allocate(const StreamConfiguration &config);\n> > + int allocate(const StreamConfiguration &config, unsigned int\n> count);\n> > const std::vector> &buffers();\n> >\n> > private:\n> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp\n> b/test/v4l2_videodevice/buffer_cache.cpp\n> > index b3f2bec11783..07fddfd2617c 100644\n> > --- a/test/v4l2_videodevice/buffer_cache.cpp\n> > +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"buffer_source.h\"\n> > @@ -145,10 +146,9 @@ public:\n> > StreamConfiguration cfg;\n> > cfg.pixelFormat = formats::YUYV;\n> > cfg.size = Size(600, 800);\n> > - cfg.bufferCount = numBuffers;\n> >\n> > BufferSource source;\n> > - int ret = source.allocate(cfg);\n> > + int ret = source.allocate(cfg, numBuffers);\n> > if (ret != TestPass)\n> > return ret;\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BECC3BDC9D\n\tfor ;\n\tMon, 26 Apr 2021 11:56:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F61F688B1;\n\tMon, 26 Apr 2021 13:56:54 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28556605BE\n\tfor ;\n\tMon, 26 Apr 2021 13:56:52 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id r128so60863100lff.4\n\tfor ;\n\tMon, 26 Apr 2021 04:56:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"IlVpMBkR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=NlYrZhvbuKlnmNDfwf09u1b1dzi0SkmZ98PUiPbiSiM=;\n\tb=IlVpMBkRdeeVzyJ8YspA/JekJym9WSni9fE/lcbsTe2EcimJHvXBK5BQsrdNHmACjZ\n\tf1qwMl7jrh84g9CfUwpDmn+OfQrpYJ5JLujfzaYnoAghugaYBPlyDQqDj00audrrG56U\n\tu+Zkon+mFeJiSqAnbuUd9+6wr0D9k6m0kFcDx4A5+JwfPU4n+41qJSIWfRO+TsdHwX64\n\ttmWEmC9yPcv5ALEs0akzTGnOVFf3/eYt4adgcgvKh3oRsYjePSPC9i30YY6z0UQB6yuk\n\tnj3kdC2fkHHHefKQCAqU0f92saZFObcJK2Wh91A2etNvnY7sviiAI3MAU7fNAJitwmCj\n\t9gMQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NlYrZhvbuKlnmNDfwf09u1b1dzi0SkmZ98PUiPbiSiM=;\n\tb=BVNRDDnhOiEIk4IxrMclBfjHkOgnFVbQq55XdMntYc/fJLHgEHXFBVVcr+vHQWkWL0\n\tei+TfSmKI8osgtyeW+VqwHtzST3KxmgxfZV8uFY/zOHI4FGZIIhRnnFmwi25ukbsU0ZV\n\tnyVqdvxVpnwWE9zbkozWgO3djr1ZmmmhqQkMYpcDZEExDH46kmRl1tNf4oKEOUirq9Kp\n\t4zCanaxqLPcTp0gRs9ZqhBYNkPlpIhpCtBpDYr9e4TgVAOmdpI5on/+igIQoLEc7dqnH\n\t3k9RU4fgHzpPuVXfmuVzewi/WDxe0Bov9/1c+WpUUmwckk7ltwdaRDCOyHtp6t313i2Q\n\tv1Uw==","X-Gm-Message-State":"AOAM532ImsKlyXsrQfYPpwj/ZssmwzTHEFdAwy/8bJcycHMIZqL+Ua+d\n\tGT2xCZPhA5qWuI0iRuLqLCDBeXQRckdAdgHnESTQuLTD8rM=","X-Google-Smtp-Source":"ABdhPJyRRlwSOkveVLDHppVcIwMJzsx/3b9mVl9J4FwNQSkA5QQSIkHsMuuAWPQX6RtqzRUh+bmxn2/nmfw1vx1pers=","X-Received":"by 2002:a05:6512:1322:: with SMTP id\n\tx34mr12502323lfu.133.1619438211429; \n\tMon, 26 Apr 2021 04:56:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-5-nfraprado@collabora.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 26 Apr 2021 12:56:35 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel , =?utf-8?q?Andr?=\n\t=?utf-8?q?=C3=A9_Almeida?= ,\n\tkernel@collabora.com","Content-Type":"multipart/mixed;\n\tboundary=\"===============3849885759555205084==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16660,"web_url":"https://patchwork.libcamera.org/comment/16660/","msgid":"","date":"2021-04-27T16:56:07","subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi,\n\nEm 2021-04-26 06:28, Niklas Söderlund escreveu:\n\n> Hello,\n>\n> On 2021-04-26 06:19:57 +0300, Laurent Pinchart wrote:\n>\n> \n>\n> > > @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera \n> > > *camera)\n> > > {\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > \tImgUDevice *imgu = data->imgu_;\n> > > -\tunsigned int bufferCount;\n> > > +\tunsigned int bufferCount = data->properties_.get(properties::QueueDepth);\n> > > \tint ret;\n> > > \n> > > -\tbufferCount = std::max({\n> > > -\t\tdata->outStream_.configuration().bufferCount,\n> > > -\t\tdata->vfStream_.configuration().bufferCount,\n> > > -\t\tdata->rawStream_.configuration().bufferCount,\n> > > -\t});\n> > \n> > I'm not sure properties::QueueDepth is the right value here. This deals\n> > with both allocation of stats and params buffers, which are internal,\n> > and the number of V4L2 buffer slots for the ImgU input and output. For\n> > the latter, we probably should overallocate, as underallocation would\n> > mean trashing dmabuf mappings. For the former, we shouldn't allocate too\n> > much to avoid wasting memory, so we should take into account how long\n> > the IPA would need to hold on the params and stats buffers.\n> > \n> > Jacopo, Jean-Michel, Niklas, any comment ?\n>\n> I agree that QueueDepth is likely not the best fit here, but neither was\n> bufferCount :-) IIRC this was done very early on where bufferCount was\n> not really defined and added as a first steeping stone. I'm very happy\n> to see this being thought about.\n>\n> Maybe the best solution for this series is to add some const numerical\n> values to the IPU3 pipeline handler to replace the usage of bufferCount\n> but not making it depending on QueueDepth?\n>\n> Maybe even separating it in two values as Laurent outlines, one for the\n> output and viewfinder slots allocation and one for the raw stream? Maybe\n> it make sens to link the raw streams count with the value used for the\n> stats and param buffer counts?\n\nOkay, this makes perfect sense. So if I'm understanding it correctly, in general\nwe don't want to use QueueDepth in importBuffers() and allocateBuffers().\nimportBuffers() should be on the bigger side, since having it less than the\namount of requests queued decreases performance. allocateBuffers() on the other\nhand is just for internal buffers, so it doesn't need to scale with the amount\nof requests sent, and how many are needed concerns only the IPA.\n\nNow, my first question is: in the ideal world, importBuffers() should\nalways \"overallocate\", meaning that however many requests are sent to the\ncamera simultaneousy, there are always more buffer slots, so performance is\nunaffected, but in reality there are limited resources so we should create a\nfixed amount of buffer slots, and by \"overallocate\" you just meant keep it on\nthe bigger side, right Laurent?\n\nSo I guess my doubt is: how many are we talking about? Does importBuffers(5) and\nallocateBuffers(3) sound reasonable? Or maybe we could keep allocateBuffers at\n4, for now, since I'm not sure if the IPA would work with less, and 4 was the\namount used all along so probably a safer bet.\n\nThanks,\nNícolas","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5DA0EBDE3D\n\tfor ;\n\tTue, 27 Apr 2021 16:57:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B524668866;\n\tTue, 27 Apr 2021 18:57:34 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CF8B602C2\n\tfor ;\n\tTue, 27 Apr 2021 18:57:33 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:c5f3:918d:a027:70d9])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id C6B3D1F419EB;\n\tTue, 27 Apr 2021 17:57:29 +0100 (BST)"],"Mime-Version":"1.0","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\t\"Laurent Pinchart\" ","Date":"Tue, 27 Apr 2021 13:56:07 -0300","Message-Id":"","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove\n\tbufferCount","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t, kernel@collabora.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]